common-fate / glide

Automate permissions to your cloud and critical applications.
https://docs.commonfate.io/common-fate/introduction
GNU Affero General Public License v3.0
237 stars 21 forks source link

Fix cloudformation vpc parameter names #652

Closed IvanVan closed 8 months ago

IvanVan commented 9 months ago

What changed?

This PR changes parameters that cloudformation is run with.

Why?

We need to match variables in gdeploy config and in cloudformation. Otherwise you get error like this:

[✘] failed to load config with error: yaml: unmarshal errors:
  line 35: field SubnetIds not found in type deploy.Parameters
  line 36: field SecurityGroups not found in type deploy.Parameters

How did you test it?

By building gdeploy from the branch and later running gdeploy update against https://releases.commonfate.io/gdeploy/v0.15.13/gdeploy_0.15.13_darwin_x86_64.tar.gz

Potential risks

Changes only touch deployment phase, so all bugs should be scoped to this.

Is patch release candidate?

Link to relevant docs PRs

https://github.com/common-fate/glide/pull/635

IvanVan commented 9 months ago

Hi @chrnorm @shwethaumashanker, I'd like to discuss with you how we can prevent this kind of bugs in the future. The problem is that now we have a few sources of truth for cloud formation parameters. For devel env we propagate parameters to CF via context - for example https://github.com/common-fate/glide/pull/635/files#diff-1fbd407f0d924995f7432e04bec68576cdf219c6a4b1605159c48f0f224ea33fR97. For prod env we propagate parameters via gdeploy here - https://github.com/common-fate/glide/pull/635/files#diff-c93215984b406264bec244ff073e6d18c7a799bc7fd19d9aec872c03ad948de0R549. Therefore, just because things work for development environment does not necessarily mean that they will work for production. To mitigate this issue, I suggest adding a test to the CI process. This test would generate the output of cdk synth to a file and then run gdeploy on it in silent mode to ensure that all the parameters are valid. What do you think?