fix: preserve existing service discovery endpoint#3949
Merged
mergify[bot] merged 6 commits intoaws:mainlinefrom Aug 29, 2022
Merged
fix: preserve existing service discovery endpoint#3949mergify[bot] merged 6 commits intoaws:mainlinefrom
mergify[bot] merged 6 commits intoaws:mainlinefrom
Conversation
Lou1415926
force-pushed
the
fix/service-discovery-endpoint
branch
from
241bc3d to
61e39b2
Compare
| if old == nil { // The ParamKey doesn't exist in the old stack, use the new value. | ||
| return &new | ||
| // transformEnvControllerParameters transforms an env-controller managed parameter. | ||
| // If the parameter exists in the old template, it uses its previous value. Otherwise, it returns its new default value. |
Contributor
There was a problem hiding this comment.
Suggested change
| // If the parameter exists in the old template, it uses its previous value. Otherwise, it returns its new default value. | |
| // If the parameter exists in the old template, it uses its previous value. Otherwise, it returns its new value. |
huanjani
reviewed
Aug 25, 2022
| // The parameter`transformFunc` are functions that transform a parameter, given its value in the new template and the old template. | ||
| // Each transform functions should keep the following in mind: | ||
| // 1. It should return `nil` if the parameter should be removed. | ||
| // 2. The transform functions are applied in a convolutional manner. |
Contributor
There was a problem hiding this comment.
Sorry-- what does this mean?
Contributor
Author
There was a problem hiding this comment.
that the second filter is applied on the result of the first filter, and the third filter is applied on the result of the second filter, etc...
any suggestion on better wordings?
Contributor
There was a problem hiding this comment.
🤔 I dunno...maybe "sequential"?
huanjani
reviewed
Aug 25, 2022
| if old == nil { | ||
| return &cloudformation.Parameter{ | ||
| ParameterKey: new.ParameterKey, | ||
| ParameterValue: aws.String(fmt.Sprintf(`%s.local`, e.in.App.Name)), |
Contributor
There was a problem hiding this comment.
wait, isn't the new default env.app.local?
Contributor
Author
There was a problem hiding this comment.
Yes but we want to use the old default in this case. Otherwise we'd be trying to update app.local to env.app.local which is the bug in the first place
Contributor
There was a problem hiding this comment.
Ah, gotcha gotcha-- I missed the second part of "the fix."
dannyrandall
approved these changes
Aug 26, 2022
huanjani
added
the
do-not-merge
huanjani
approved these changes
Aug 29, 2022
Contributor
huanjani
left a comment
huanjani
left a comment
There was a problem hiding this comment.
Looks good! Adding label in case you want to address nits on comments.
Lou1415926
removed
the
do-not-merge
mergify Bot
pushed a commit
that referenced
this pull request
Aug 30, 2022
…k with bootstrap stack (#3966) Previously, we have two fixes: 1. #3956 which patches the S3 permissions to the environment manager role if it's needed. 2. #3949 which preserves service discovery endpoint namespace from a previously deployed env stack. Fix 1 performs an environment template version check to infer whether the necessary permissions are present. This version check fails to take into account the `"bootstrap"` version, which is present when the environment is only bootstrapped (by running `env init` and not having run `env deploy`) Fix 2 grabs the old parameter from a deployed environment stack. It fails to take into consideration that, while an env stack that's on `"bootstrap"` version is not considered a "deployed environment" in Copilot, it is a deployed stack in CFN. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Back in v1.9.0, we introduced “multiple environment per VPC”. This feature is shipped with upgrading the service discovery endpoint, from app.local to env.app.local. Since during env upgrade, we preserve all the old parameters, app.local has always been preserved for old environments.
However, env deploy didn’t account for this. It will try to use env.app.local for all environments, old or new. When env deploy is executed against an environment that was created pre-v1.9.0, it results in an attempt to update app.local to env.app.local. This will be breaking for environments that already have services in it, because the service discovery endpoint is exported and imported by the service stack, thus not updatable.
The fix
The fix simply preserves the value of the he existing
ServiceDiscoveryEndpointparameter. It also accounts for the scenario whereServiceDiscoveryEndpointis not yet in the parameter (pre-v1.9.0 environment that has never been upgraded), and useapp.localfor those environment.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.