guardian / cdk

Generic Guardian flavoured AWS CDK components
https://guardian.github.io/cdk/
22 stars 5 forks source link

Do we really need GuSSMParameter? #1032

Closed nicl closed 2 years ago

nicl commented 2 years ago

As part of some JSII testing, I was looking at the GuSSMParameter and also the original PR:

https://github.com/guardian/cdk/pull/336

GuSSMParameter is a custom resource that allows using SSM parameters within Cloudformation templates. To recap the motivations given at the time:

However, I can't see any usage of GuSSMParameter in our codebase.

Is GuSSMParameter really an improvement over SSM parameters or should we just delete it?

To take some of the motivations above:

Defaults are tricky, but it's not obvious why data that needs to be in Parameter Store would have a sensible default value within @guardian/cdk itself; typically parameter store is used for application-specific toggles or secrets. Indeed, the lack of usage seems to confirm this.

And, the 'con' of increased Cfn parameters seems to me to actually be a win. It is much clearer what the inputs to a stack are if they are actual parameters. The stack then becomes closer to a function conceptually, rather than magically relying on all kinds of global state deep in the stack body (which is what this custom resource seems to achieve).

So, to summarise, it isn't clear that this feature meets the problems described any better than native SSM Cfn parameters and, in some respects, is actually worse.

@guardian/cdk is already pretty large. Let's delete this complex and low-value feature.

nicl commented 2 years ago

Update: okay we've agreed to remove this. In general, turns out there has been a bit of a reversal on custom resources anyway due to the random lambdas they create in accounts.

akash1810 commented 2 years ago

Fixed in #1157 .