feat: allow multiple environments per VPC via new service discovery namespace #2515

bvtujo · 2021-06-24T21:57:29Z

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.

In the image above, I have deployed the same service to 3 environments.
legacy was created at environment version 1.4.0. and upgraded to 1.5.0.
new was created at version 1.5.0
shared-with-legacy was created at 1.5.0 and shares a vpc and subnets with legacy.

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.

Updated integ tests to reflect new endpoints. Manually tested the above by running copilot svc exec -n fe -e $ENV for each of the three environments, then running curl fe.$COPILOT_SVC_DISCOVERY_ENDPOINT from within the container. Each time, I received the expected HTML content.

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.

iamhopaul123

Looks good to me! Would you add e2e and maybe update the doc accordingly (and merge conflict again sorry)? Thank you!

internal/pkg/cli/job_package.go

efekarakus

Looks good to me! Mostly just a few questions

efekarakus · 2021-06-28T23:21:34Z

internal/pkg/describe/env.go

+		// If the service discovery param is set (meaning the stack is not upgraded), use the provided value
+		if v != "" {


Wouldn't this mean that the stack is actually upgraded? i.e. we are at least on v1.5.0

efekarakus · 2021-06-28T23:22:32Z

internal/pkg/describe/env.go

+		if v != "" {
+			return v, nil
+		}
+		// if the param exists but is empty, use the legacy endpoint.


When can this happen? (not against the code or anything) but it might be worthwhile writing it.

My understanding is that when upgrading we always set a value for this parameter.

We don't set a value when upgrading. If the stack is created at version 1.5.0 or higher, it's set to env.app.local. If the stack is upgraded from before that version, it's set to empty, which causes the template to use app.local.

site/content/docs/developing/service-discovery.en.md

efekarakus · 2021-06-28T23:35:02Z

site/content/docs/developing/service-discovery.en.md

+
+Prior to Copilot v1.8.0 and environment version 1.5.0, the service discovery namespace used the format _{app name}.local_, without including the environment. This limitation made it impossible to deploy multiple environments in the same VPC. Any environments created with Copilot v1.8.0 and newer can share a VPC with any other environment.
+
+When your environments are upgraded, Copilot will honor the service discovery namespace that the environment was created with. That means that the endpoint your services are reachable at will not change. Any new environments created with Copilot v1.8.0 and above will use the _{env name}.{app name}.local_ format for service discovery, and can share VPCs with older environments. 


Can we create an issue and pin that in our repository so that folks are aware of this change.
Ideally if they use only the env variable then we are good. However, if folks have app.local hardcoded in their code and deploy to a new env then their code will fail mysteriously. (Which I think is ok since we provide the env variable)

templates/environment/versions/cf-v1.5.0.yml

…t param value

efekarakus · 2021-07-07T20:27:41Z

internal/pkg/cli/svc_deploy.go

@@ -262,6 +263,14 @@ func (o *deploySvcOpts) configureClients() error {
 	// CF client against env account profile AND target environment region
 	o.svcCFN = cloudformation.New(envSession)

+	o.endpointGetter, err = describe.NewEnvDescriber(describe.NewEnvDescriberConfig{


Do we need to update cli/deploy.go as well? I couldn't find it in the list of files.

No, since there are no flag changes and deploy under the hood just invokes svc or job deploy.

The endpointGetter func is initialized during Execute for both svc init and job init by the configureClients() method on the initSvcProps and initJobProps structs.

internal/pkg/describe/env.go

internal/pkg/deploy/cloudformation/stack/env.go

templates/environment/versions/cf-v1.5.0.yml

Co-authored-by: Efe Karakus <efekarakus@gmail.com>

bvtujo · 2021-07-12T19:06:11Z

Success. Tested that the given endpoint is reachable from each environment by using `copilot svc exec`.

efekarakus

Woohooo 🚀

iamhopaul123

Awesome 🚢

…amespace (aws#2515) 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. <img width="761" alt="Screen Shot 2021-06-24 at 3 56 02 PM" src="https://user-images.githubusercontent.com/25392995/123342814-50146a00-d505-11eb-8749-41e409150999.png"> In the image above, I have deployed the same service to 3 environments. legacy was created at environment version 1.4.0. and upgraded to 1.5.0. new was created at version 1.5.0 shared-with-legacy was created at 1.5.0 and shares a vpc and subnets with legacy. 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. Updated integ tests to reflect new endpoints. Manually tested the above by running `copilot svc exec -n fe -e $ENV` for each of the three environments, then running `curl fe.$COPILOT_SVC_DISCOVERY_ENDPOINT` from within the container. Each time, I received the expected HTML content. Related aws#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 added 3 commits June 24, 2021 21:50

initial commit

ab9e358

feat: allow multiple environments in vpcs

a7a2c89

chore: fix mocks

a0881b9

bvtujo requested a review from a team as a code owner June 24, 2021 21:57

bvtujo requested review from huanjani and removed request for a team June 24, 2021 21:57

amazon-ecs-cli-v2-pr-manager requested a review from iamhopaul123 June 24, 2021 21:57

amazon-ecs-cli-v2-pr-manager added this to In review in Sprint 🏃‍♀️ Jun 24, 2021

chore: update env version to latest

926a3c2

iamhopaul123 mentioned this pull request Jun 25, 2021

copilot 1.8 - Alias did not update route53 #2469

Open

iamhopaul123 reviewed Jun 25, 2021

View reviewed changes

internal/pkg/cli/job_package.go Show resolved Hide resolved

docs: add documentation for multiple envs

be44f92

bvtujo requested a review from toricls as a code owner June 28, 2021 18:05

bvtujo added 2 commits June 28, 2021 23:22

chore: output different discoveries in svc show

6b563dc

test: add e2e test for customized env

a0191c7

efekarakus reviewed Jun 28, 2021

View reviewed changes

iamhopaul123 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 29, 2021

bvtujo added 5 commits July 6, 2021 16:48

Merge branch 'mainline' into multiple-envs

c19104f

chore: default to app.local as svc discovery param for upgraded envs

a0c3547

chore: remove conditional from env template in favor of better defaul…

57994cb

…t param value

Merge branch 'mainline' into multiple-envs

7025414

chore: go mod tidy

58c4db2

efekarakus reviewed Jul 7, 2021

View reviewed changes

iamhopaul123 reviewed Jul 7, 2021

View reviewed changes

templates/environment/versions/cf-v1.5.0.yml Outdated Show resolved Hide resolved

Merge branch 'mainline' into multiple-envs

b11683d

iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 8, 2021

bvtujo added 2 commits July 9, 2021 22:59

Merge branch 'mainline' into multiple-envs

871685e

add endpoint to integ tests

2d3dc35

efekarakus changed the title ~~fix: allow multiple environments per VPC via new service discovery namespace~~ feat: allow multiple environments per VPC via new service discovery namespace Jul 9, 2021

Update site/content/docs/developing/service-discovery.en.md

b6ac367

Co-authored-by: Efe Karakus <efekarakus@gmail.com>

bvtujo added 4 commits July 12, 2021 19:09

chore: update comment in Describe

c040aea

chore: update version in docs

7f63d18

test(e2e): fix init test to look for right svc discovery endpoint

f7828f9

chore: fix typo in customized env e2e

03f68a8

efekarakus approved these changes Jul 12, 2021

View reviewed changes

iamhopaul123 approved these changes Jul 12, 2021

View reviewed changes

Merge branch 'mainline' into svc-discovery-env

0af2b59

mergify bot merged commit 64920a0 into aws:mainline Jul 12, 2021

Sprint 🏃‍♀️ automation moved this from In review to Pending release Jul 12, 2021

efekarakus mentioned this pull request Jul 13, 2021

Support for multi application load balancer on Load Balanced Web Service #2604

Closed

efekarakus mentioned this pull request Dec 1, 2021

Copilot is not spinning a new environment #2377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow multiple environments per VPC via new service discovery namespace #2515

feat: allow multiple environments per VPC via new service discovery namespace #2515

bvtujo commented Jun 24, 2021 •

edited

iamhopaul123 left a comment •

edited

efekarakus left a comment

efekarakus Jun 28, 2021

efekarakus Jun 28, 2021

bvtujo Jul 6, 2021

efekarakus Jun 28, 2021

efekarakus Jul 7, 2021

bvtujo Jul 12, 2021

bvtujo Jul 12, 2021

bvtujo commented Jul 12, 2021

efekarakus left a comment

iamhopaul123 left a comment

		// If the service discovery param is set (meaning the stack is not upgraded), use the provided value
		if v != "" {


		Prior to Copilot v1.8.0 and environment version 1.5.0, the service discovery namespace used the format _{app name}.local_, without including the environment. This limitation made it impossible to deploy multiple environments in the same VPC. Any environments created with Copilot v1.8.0 and newer can share a VPC with any other environment.

		When your environments are upgraded, Copilot will honor the service discovery namespace that the environment was created with. That means that the endpoint your services are reachable at will not change. Any new environments created with Copilot v1.8.0 and above will use the _{env name}.{app name}.local_ format for service discovery, and can share VPCs with older environments.

feat: allow multiple environments per VPC via new service discovery namespace #2515

feat: allow multiple environments per VPC via new service discovery namespace #2515

Conversation

bvtujo commented Jun 24, 2021 • edited

iamhopaul123 left a comment • edited

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

efekarakus Jun 28, 2021

Choose a reason for hiding this comment

efekarakus Jun 28, 2021

Choose a reason for hiding this comment

bvtujo Jul 6, 2021

Choose a reason for hiding this comment

efekarakus Jun 28, 2021

Choose a reason for hiding this comment

efekarakus Jul 7, 2021

Choose a reason for hiding this comment

bvtujo Jul 12, 2021

Choose a reason for hiding this comment

bvtujo Jul 12, 2021

Choose a reason for hiding this comment

bvtujo commented Jul 12, 2021

efekarakus left a comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

bvtujo commented Jun 24, 2021 •

edited

iamhopaul123 left a comment •

edited