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

fix: allow multiple environments per VPC #2394

Closed
wants to merge 21 commits into from

Conversation

bvtujo
Copy link
Contributor

@bvtujo bvtujo commented May 28, 2021

This PR modifies the default service discovery namespace for new environments to the form {env}.{app}.local. This change allows customers to place multiple environments in the same VPC.

The PR also creates an upgrade path for older environments. Those upgraded environments will retain the default app.local namespace to avoid a backward breaking change for services deployed there, but new environments will use env.app.local.

Screen Shot 2021-05-28 at 9 19 04 AM

In the image above, I have deployed the same service to 5 environments.
test and beta were created at env version 1.4.0 and upgraded to 1.5.0.
delta-shared-vpc was created at version 1.5.0 shares its vpc and subnets with beta.
epsilon-shared-test was created at 1.5.0 and shares its vpc and subnets with test.
gamma is a new environment.

There currently is not an upgrade path for existing environments to switch to new service discovery. This should not be an issue in most cases, however, as the environment variable points to the correct namespace in all cases and new+old environments can coexist in the same VPC.

This change does not disrupt any existing environments. If we eventually add an "environment v2.0.0," we can force migration to the new discovery service by removing the UseLegacyServiceDiscoveryIfBlank parameter and updating our workloads to remove the LegacyServiceDiscovery bool from the template package.

Related #2377

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bvtujo bvtujo requested a review from a team as a code owner May 28, 2021 16:34
@bvtujo bvtujo requested a review from efekarakus May 28, 2021 16:34
@bvtujo
Copy link
Contributor Author

bvtujo commented May 28, 2021

For easy comparison, an annotated diff between v1.4.0 and 1.5.0

5c5
<   Version: 'v1.4.0'
---
>   Version: 'v1.5.0'
30a31,33
>   UseLegacyServiceDiscoveryIfBlank:
>     Type: String
>     Default: ""
44a48,49
>   UseLegacyServiceDiscovery:
>     !Equals [ !Ref UseLegacyServiceDiscoveryIfBlank, "" ]

New parameter and new condition. If the environment is upgraded, this is left blank. If the environment is new, this parameter's value is set to "doNotCreate". The condition is used to change the value of a stack output.

51a57
>   # This is a legacy construct and only used in environments created before v1.5.0.

Change to comment on old svc discovery namespace.

60a67,77
>   # Creates a service discovery namespace with the form:
>   # {svc}.{env}.{appname}.local
>   ServiceDiscoveryNamespaceWithEnv:
>     Type: AWS::ServiceDiscovery::PrivateDnsNamespace
>     Properties:
>       Name: !Sub ${EnvironmentName}.${AppName}.local
> {{- if .ImportVPC}}
>       Vpc: {{.ImportVPC.ID}}
> {{- else}}
>       Vpc: !Ref VPC
> {{- end}}

New namespace.

304a323
>     Condition: UseLegacyServiceDiscovery

Add a condition to the old service discovery output

307a325,332
>   ServiceDiscoveryNamespaceIDWithEnv:
>     Value: !GetAtt ServiceDiscoveryNamespaceWithEnv.Id
>     Export:
>       Name: !Sub ${AWS::StackName}-ServiceDiscoveryNamespaceIDWithEnv
>   UseLegacyServiceDiscovery:
>     Value: !If [ UseLegacyServiceDiscovery, true, false ]
>     Export:
>       Name: !Sub ${AWS::StackName}-UseLegacyServiceDiscovery

Create a new output with the new namespace, and an output to use to tell svc deploy whether to use the legacy namespace or the new one.

@bvtujo bvtujo added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label May 28, 2021
@bvtujo
Copy link
Contributor Author

bvtujo commented May 28, 2021

Just realized this leaves the env template in a state with modes--applying the do-not-merge label while I fix that.

Ed: the environment template is now non-modal. Both namespaces are always created, and it's up to svc deploy to choose which namespace to register a service with based on whether the environment's UseLegacyServiceDiscovery output is "true" or empty.

ed: non-modal behavior breaks the ability of environments to coexist. Kept the check on the output of the stack, but only create the legacy namespace when the input parameter is empty.

@bvtujo bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label May 28, 2021
Added the conditions back to the export and the legacy service discovery namespace.
Turns out the condition was necessary in order for environments to coexist in the
same VPC.
@bvtujo bvtujo requested a review from toricls as a code owner May 28, 2021 19:53
@huanjani
Copy link
Contributor

Wow, what a comprehensive PR-- you have all the things, even including Japanese docs!
Quick question: do we need to worry about the 1024 limit for namespace names? Is it worth building in a check/truncation?

@bvtujo
Copy link
Contributor Author

bvtujo commented Jun 2, 2021

@huanjani I don't believe so; we cap service, app, and environment names at 255 characters so we'll never hit that limit!

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

Can't believe this will be fixed so quickly. Thank you!

}
stackOutputs := make(map[string]string)
for _, output := range descr.Outputs {
stackOutputs[*output.OutputKey] = *output.OutputValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use aws.StringValue() for consistency?

Value: !GetAtt ServiceDiscoveryNamespaceWithEnv.Id
Export:
Name: !Sub ${AWS::StackName}-ServiceDiscoveryNamespaceIDWithEnv
UseLegacyServiceDiscovery:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this output? Can't we check if UseLegacyServiceDiscoveryIfBlank this param is blank to tell?

@@ -13,7 +13,7 @@ const (
// LegacyEnvTemplateVersion is the version associated with the environment template before we started versioning.
LegacyEnvTemplateVersion = "v0.0.0"
// LatestEnvTemplateVersion is the latest version number available for environment templates.
LatestEnvTemplateVersion = "v1.4.0"
LatestEnvTemplateVersion = "v1.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reminder if #2413 is merged earlier we need to include changes in v1.5.0

Comment on lines +19 to +23
{{- if .LegacyServiceDiscovery}}
!Sub '${AppName}-${EnvName}-ServiceDiscoveryNamespaceID'
{{- else}}
!Sub '${AppName}-${EnvName}-ServiceDiscoveryNamespaceIDWithEnv'
{{- end}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to include both if they recently upgraded their environment? since in the env template if they were using legacy sd we also add new sd for them. Or we don't create new sd if they are using legacy at all.

AdditionalTags: tags.Merge(o.targetApp.Tags, o.resourceTags),
AddonsTemplateURL: addonsURL,
AdditionalTags: tags.Merge(o.targetApp.Tags, o.resourceTags),
LegacyServiceDiscovery: svcDiscovery,
Copy link
Contributor

Choose a reason for hiding this comment

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

does copilot job use service discovery 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We inject the endpoint as an environment variable, so we still have to update this here.

Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

Sorry I haven't had the time to read the PR completely! Can you explain to me the sequence of events for this fix? I think it would be nice to have a 1-2 page short doc/issue explaining how you got this resolved to the team as it's on the critical path.

Can we do a flow that's like this?

1. If running `env init`, then create service discovery with new format
2. If running `env upgrade`:
   2.1. Keep old service discovery namespace
   2.2. Add the new service discovery namespace
3. In svc templates only use the new service discovery namespace

Instead of passing CFN parameters, I wonder if we can use Go templates to decide if we want to keep the old svc discovery

AdditionalTags: tags.Merge(o.targetApp.Tags, o.resourceTags),
AddonsTemplateURL: addonsURL,
AdditionalTags: tags.Merge(o.targetApp.Tags, o.resourceTags),
LegacyServiceDiscovery: svcDiscovery,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this fits under RuntimeConfig. The original idea behind this struct was to capture additional configuration passed by the client when calling deploy: 1) the url for addons/ template 2) if they use the deploy --resource-tags flag.

Maybe we can push down the LegacyServiceDiscovery one level below into the stack pkg? so that the clients have to do less work

internal/pkg/cli/svc_package.go Show resolved Hide resolved
Comment on lines +91 to +94
output, ok := stackOutputs[stack.EnvOutputLegacyServiceDiscovery]
if !ok || output != "false" {
return true, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the UseLegacyServiceDiscovery output? Can we instead check if we have the new output ServiceDiscoveryNamespaceIDWithEnv in the template?

@efekarakus efekarakus self-assigned this Jun 7, 2021
@efekarakus efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 16, 2021
@bvtujo
Copy link
Contributor Author

bvtujo commented Jun 24, 2021

Closing and reopening due to merge conflicts.

@bvtujo bvtujo closed this Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge Pull requests that mergify shouldn't merge until the requester allows it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants