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

(aws-ssm): Support StringParameter default value #26795

Closed
2 tasks
luxaritas opened this issue Aug 17, 2023 · 8 comments
Closed
2 tasks

(aws-ssm): Support StringParameter default value #26795

luxaritas opened this issue Aug 17, 2023 · 8 comments
Labels
@aws-cdk/aws-ssm Related to AWS Systems Manager feature-request A feature should be added or improved.

Comments

@luxaritas
Copy link
Contributor

luxaritas commented Aug 17, 2023

Describe the feature

Instead of providing a value that will always be set on redeploy (or if the parameter changes), I'd like to be able to set a default value for the parameter, such that it will only be set if there is not a value for that parameter already

Use Case

I have a Fargate service defined in CDK and an ECS deployment in CodePipeline which updates the image in its task definition. By default I set the image to be an empty nginx image (see #14765 (comment)), but this means that if I ever update some other part of the task definition, the image is "reset" to the empty image instead of the one set by the codepipeline. My intent is to initialize a parameter with the nginx image and change it at the same time I update the task in codepipeline so that if I redeploy the task definition from the CDK app, it can get the latest published image

Proposed Solution

Change StringParameter to require EITHER a value or defaultValue, and if a defaultValue is provided, query the parameter store first to use as a preferred value

Other Information

As a workaround, I intend to use the approach defined here (defining the parameter name myself): #7051 (comment) - a significant downfall of this though is that when I destroy the stack, it won't remove the parameter, so on recreating the stack it won't be reset to the default value

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.67.0

Environment details (OS name and version, etc.)

NA

@luxaritas luxaritas added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 17, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ssm Related to AWS Systems Manager label Aug 17, 2023
@luxaritas
Copy link
Contributor Author

Actually the workaround doesn't appear to work, as it ends up throwing SSM parameter not available in account when not present. I guess I'll have to use the SSM SDK. Also going to retract my note that I may be able to implement this, as it's not something available in cfn and I wouldn't know how to manage needing the parameter name ahead of time to get the current value when the name may not be auto-generated until later...

@luxaritas
Copy link
Contributor Author

In retrospect, I guess since CloudFormation doesn't automatically reconcile drift, this is already the behavior? If my pipeline has changed the parameter value, redeploying the stack shouldn't change it unless I've modified the parameter itself, right?

@peterwoodworth
Copy link
Contributor

I'm not sure how StringParameter comes into play here, are you referring to the import methods or the constructor? If you're referring to the import method, isn't this a duplicate of #7051? Or do you intend to create the StringParameter if it doesn't already exist?

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 17, 2023
@luxaritas
Copy link
Contributor Author

luxaritas commented Aug 17, 2023

Or do you intend to create the StringParameter if it doesn't already exist?

Yeah, that's the idea - that way, if/when the stack is destroyed, the parameter also goes away (so that if/when the stack is recreated, it goes back to the default value). Am I correct that "defaulting" is the behavior StringParameter would have anyways?

@peterwoodworth
Copy link
Contributor

You can't both try to import it if it exists, and create it if it doesn't already without implementing your own logic. That's not how CloudFormation works, and CDK best practice is to not make decisions at runtime based on asynchronous calls like this, since CDK apps should be deterministic. See commit cdk.context.json for more information.

If you are initially intending to define the StringParameter through CDK, you should only be updating that parameter through your CDK app, (probably by passing in a Parameter which does support default values, or by running a script before cdk deploy as described in the best practices docs) and everything that consumes this StringParameter value should be structured in such a way that it can depend on knowing that the value exists.

I think the feature request #7051 is reasonable, but there's not much else we can provide without going against our own recommendations

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Aug 17, 2023
@luxaritas
Copy link
Contributor Author

If you are initially intending to define the StringParameter through CDK, you should only be updating that parameter through your CDK app

The problem with this, per the use case above, is that the parameter value needs to change every time the pipeline for the Fargate service is run. Sure, I could pull down the latest parameter values before running a cdk deploy, but then there's additional latency between the time the parameter is fetched and the time its updated, which means more chance that the pipeline changed in that between time and the value is reverted to something out-of-date. Also, seems silly when the parameter is... already there, and filled out how I need it.

I suppose the "real" problem in this case is not wanting to have to fill out the container image in the task before the first pipeline run (and I don't want to insert the extra complexity of a codepipeline-deployed Cloudformation stack just for that). Or, secondarily, that the ECR CodeDeploy action breaks the CDK best practice of "if its specified by the CDK, nothing else modifies it" (though again, I fail to see a way around that which doesn't involve a whole bunch of extra complexity). Though this is presumably a completely separate issue.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 18, 2023
@peterwoodworth
Copy link
Contributor

Thanks for your reasoning @luxaritas, we are bound to CloudFormation however. If you have an idea for a custom construct which can make this work, we would suggest you build that 🙂 The request can't work with the current solution, and we don't tend to branch away from CloudFormation if we can help it. Like I mentioned before, #7051 is reasonable to add to our context provider, but an overhaul to the construct / adding a custom one for this use case wouldn't be in scope. I'm going to close this in favor of #7051, thanks for the discussion

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ssm Related to AWS Systems Manager feature-request A feature should be added or improved.
Projects
None yet
Development

No branches or pull requests

2 participants