Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for cross service communication via CloudFormation outputs #3575

Merged
merged 2 commits into from
May 9, 2017

Conversation

eahefnawy
Copy link
Member

What did you implement:

Closes #3442

This PR adds support for cross service/stack communication by utilising CloudFormation outputs as a new source for the variable system => ${cf:stackName.outputKey}

How did you implement it:

Added a new CloudFormation source to the variable system that allows you reference any output values in any stack you specify. This way a user can define CF outputs in one service, and reference them in another.

How can we verify it:

You'll need two services to test communication:

service: service-a

provider:
  name: aws
  memorySize: 512

functions:
  hello:
      handler: handler.hello

resources:
  Outputs:
    memorySize:
      Value: ${self:provider.memorySize}
      Export:
        Name: memorySize
service: service-b

provider:
  name: aws
  memorySize: ${cf:service-a-dev.memorySize}

functions:
  hello:
      handler: handler.hello

deploy service-a then service-b, you'll notice that the memory size of the hello function in service-b is now set to 512 instead of the default 1024, following service-a config.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant 👏 !!! Love the simplicity!

Works like a charm 🎉 :shipit:

@pmuens pmuens merged commit c868e63 into master May 9, 2017
@pmuens pmuens deleted the cross-service-communication branch May 9, 2017 13:00
@nicka
Copy link
Member

nicka commented May 9, 2017

May I ask why we're not going with Fn::ImportValue?

Since it has the following features (yes I find them features 😉 ):

  • You can't delete a stack if another stack references one of its outputs.
  • You can't modify or remove an output value that is referenced by another stack.

@erikerikson
Copy link
Member

We have enjoyed those features too.

That said, we had also written a plugin for doing just this in a case where we were using CF outputs for a build process.

Thanks!

@HyperBrain
Copy link
Member

We also rely on the integrity provided by CF when using Fn::ImportValue. E.g. our VPC and network stack configurations are stored ina separate stack and referenced by the other SLS stacks. It would imply capital damage if the stack references were not protected and someone just deletes the network stack.
Every losely coupled stack dependency imposes quite a risk.

@eahefnawy
Copy link
Member Author

eahefnawy commented May 9, 2017

@nicka the main reason is that Fn::ImportValue wouldn't really work when used outside the context of a CF stack, for example if used in the custom property for generic service config that would never be included in the CF template. Fn::ImportValue isn't really a value itself but rather a ref to a value, which doesn't fit well with the variable system.

We may still go with Fn::ImportValue in the future though for the features you mentioned, but I think we'd still keep the cf source along with it.

Just my two cents 😊

@nicka
Copy link
Member

nicka commented May 9, 2017

@eahefnawy Thanks for the response, and it does make sense! In any case it's a nice addition and we're all still free to use Fn:::ImportValue when needed.

Off topic, we should really try to fix all area's where it's not possible to specify Fn:::ImportValue's within the serverless.yml. Not sure if it's already fixed, but we did hit it for sns events.

@eahefnawy
Copy link
Member Author

@nicka for sure! 👍 ... is there an issue open where we can track this? 😊

@arabold
Copy link

arabold commented May 9, 2017

I must admit I'm not the biggest fan of ${cf:foo.bar} for reasons given by @nicka and @HyperBrain. However, I can see why it was added and the code is incredibly simple and straight forward. You might also want to take a look at the Export-Env Plugin I've wrote a few weeks back. I ran into issues with Fn::ImportValue and Ref: as those would only be resolved during deployment, not when testing Serverless locally using sls invoke local or sls offline, or when running local mocha tests. The plugin solves that by injecting the resolved variables from the CloudFormation stack during runtime. This allows you to use both Ref: and Fn:ImportValue e.g. for environment variables. Without the plugin sls invoke local would only resolve those to [Object object], not an actual value.

I think both problems are related. ${cf:foo.bar} resolves the variables the same way as the plugin (which much nicer code though 😆 ). The difference is that this core sls change does it before package/deploy, the plugin does it locally during runtime. I'm wondering if it would make sense to improve support for Ref: and Fn::ImportValue in core Serverless in a similar way as the plugin, e.g. automatically resolve the values during local execution to avoid the [Object object] issue described above.

@nitely
Copy link

nitely commented Jun 5, 2017

@nicka

Off topic, we should really try to fix all area's where it's not possible to specify Fn:::ImportValue's within the serverless.yml. Not sure if it's already fixed, but we did hit it for sns events.

That's available since forever. Pretty much everywhere where you can use intrinsic functions.

functions:
  fufu:
    handler: fufu.handler
    events:
      - sns:
          arn:
            Fn::ImportValue: ${cf:service-a-dev.fufuTopicArn}
          topicName: fufuTopic

@pmuens
Copy link
Contributor

pmuens commented Jun 5, 2017

That's available since forever. Pretty much everywhere where you can use intrinsic functions.

@nitely thanks for mentioning that. AFAIK some event definitions are still lacking behind because we had to implement this over and over again for every event in the past.

Referencing #3212 here where we're discussing a global arn parser.

@tommedema
Copy link

tommedema commented Dec 15, 2017

@eahefnawy @HyperBrain I've been playing around with a proof of concept on how to create large, domain/service separated serverless applications. The need for cross stack references is clear.

There remains one big issue, no matter if you use cf: or Fn::ImportValue. The issue is in orchestration: cloudformation is great because it automatically determines in which order your resources should be created when they depend on each other. However, we lack this functionality when stacks are depending on each other (instead of resources depending on each other). This means that we need to re-implement such dependency orchestration ourselves, and so far this has not been done.

You might feel like cf: etc. is working, but in most projects, it would fail on the first deployment because there is a large chance that the stack it is referencing would not yet have been created created at this point. It would only start working on subsequent deployments, and after manually changing the order of deployment. And even so, in subsequent deploys, it would probably actually reference to the value of the instance of a stack just before the current deployment, while the intention is obviously to refer to the value after the current deployment.

Is my explanation clear enough? From my perspective this is a huge issue that needs to be resolved for serverless to become an acceptable paradigm in larger projects.

If you like, I could invite you to a private repo in which I am playing around with this concept.

@nitely
Copy link

nitely commented Dec 15, 2017

Hey @tommedema , I'm a heavy user of serverless and I've used it to create the kind of deployment you are referring to.

The issue is in orchestration: cloudformation is great because it automatically determines in which order your resources should be created when they depend on each other.

It does not. You must specify dependsOn on every resource that depends on a second one. Even if it's the same stack, CF does not guarantee the creation order (without dependsOn). It's actually pretty random, sometimes it'll work as you may expect and sometimes it won't. It has no automatic dependency resolution of any kind.

dependsOn does not work across stacks. So, no way to define a resource depends on a second stack that way. You have to use Fn::ImportValue for that.

Circular dependencies are the worst. If there is a resource A depending on C in one stack and a resource B depending on C, then C must be deployed on a third stack before A and B, then you can use Fn::ImportValue as normal. There are other sceneries where this is just not possible, then you have to check if there is another way to solve that. There usually is, but it's case specific.

@tommedema
Copy link

tommedema commented Dec 15, 2017

@nitely thanks for your answer. You don't have to specify dependsOn; this is implied automatically whenever you refer (i.e. depend) on another resource. E.g. my hosted zone has a Ref: to a cloudfront distribution, then CF will ensure that the cloudfront distribution is deployed prior to the hosted zone. That's dependency resolution, right? ;)

There is indeed no way to define dependencies across-stack. This is why I'm advocating that this should be built into Serverless Framework, because I don't see large projects being maintainable if you have to wait for remote dependency resolution errors to happen. Especially since resolving them is a nightmare.

Consider this example of a circular dependency that's very real (it happened just 10 minutes ago). I have three services:

  1. a web service that provides my s3 buckets and cloudfront distributions for hosting my web app, it also contains the source files and builds them
  2. a domains service that creates the route53 hosted zone and record sets and connects it to the cloudfront distribution
  3. a ssl service that automatically retrieves, validates, and renews certificates

All of these services have so much domain-specific business logic that their serverless.yml files exceed ~150-200 lines. Therefore it's good to keep them separated.

The circular dependency is here:

  1. the web service's cloudfront distribution depends on the ssl service's certificate; therefore the ssl service would have to be deployed first (otherwise the certificate stack output value would not yet exist when the web service would request it)

ssl --> web

  1. the ssl service depends on the domain's service's route53 hosted zone, because it needs to create new record sets and assign them to the domain's exported hosted zone ID. So far there is no direct circular dependency yet, but there is an indirect one: the domain's hosted zone in turn depends on the web service's cloudfront distribution. This means that the web service has to be deployed before domain service. At the same time the ssl service would have to be deployed before the web service (see point 1), but this is not possible. There is an indirect circular dependency.

web --> domain --> ssl

Of course, circular dependencies cannot really be resolved, but the orchestration or order of deployment can. Currently this last part is done for references within stacks, but not between stacks.

@nitely
Copy link

nitely commented Dec 15, 2017

You don't have to specify dependsOn; this is implied automatically whenever you refer (i.e. depend) on another resource.

For policies you definitely have to. I don't know about ref, but for the sake of this conversation let's say you are right.

because I don't see large projects being maintainable if you have to wait and see for remote errors dependency resolution errors to happen. Especially since resolving them is a nightmare.

AFAIK, everyone has a stage just for deployment testing. No need for resolving them as you can just delete and create the whole thing gain for a second try. I know this is not ideal, but CF does not gives lots of tools for this.

There has been some discussions about automatically splitting a serverless stack, but IMHO that's too ambitious.

@tommedema
Copy link

tommedema commented Dec 15, 2017

I think we have a different perception on what kind of developer experience would be acceptable for large projects. In my opinion, deploying to a review environment is supposed to be for manual and functional testing of the application, and for product owners to confirm expectations. It should not be for testing if deployment works. Especially considering how slow a serverless deployment is, this would be much worse than just not ideal. In my case for example, I run a tech department with a small team of about 20 developers and really want to make a shift to serverless in the future -- but this is currently holding us back. I imagine that for a larger company this is even more so the case.

@nitely
Copy link

nitely commented Dec 15, 2017

I think we have a different perception on what kind of developer experience would be acceptable for large projects.

I think you have a different perception than AWS engineers, PMs or whoever make the calls. I wish CF had better recovering/rollback for failed deployments but it's not the case and avoiding them is not very realistic. Providing this as a third party is a way harder task. IMHO, automatic deployment far outweighs the cons of CF.

alexvpickering added a commit to alexvpickering/appsync-serverless-notes-app that referenced this pull request Mar 20, 2018
Having seperate stacks for each resource fixes issues with re-deploying buckets/etc that already exist.
It also splits up code and increases potential for re-using stacks.
added 'deploymentBucket' property so that all stacks deployed to same bucket (created manually).

References:
https://forum.serverless.com/t/cross-stack-references/1402/2 (rationale for multiple stacks)
serverless/serverless#3575 (PR showing how-to cross-stack-references)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants