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

fix(riff-raff.yaml): Loosen restrictions on GuStacks in an App #1695

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented Jan 19, 2023

Background

When we added riff-raff.yaml generation in #1372, we had a validation step that:

  1. Discovered all the GuStacks in an App
  2. Discovered the set of regions
  3. Discovered the set of stackss
  4. Discovered the set of stages
  5. For each combination of the above, expected to find a GuStack that matched.

This meant, for example, for the following:

class MyDatabaseStack extends GuStack {}
class MyAppStack extends GuStack {}

new MyDatabaseStack({ env: { region: 'eu-west-1' }, stack: 'frontend', stage: 'CODE' });
new MyAppStack({ env: { region: 'eu-west-1' }, stack: 'frontend', stage: 'CODE' });
new MyAppStack({ env: { region: 'eu-west-1' }, stack: 'frontend', stage: 'PROD' });

We'd get validation errors as there is no PROD MyDatabaseStack definition.

The aim of this validation is to only produce riff-raff.yaml that works.

With MyDatabaseStack of PROD missing, when we deploy PROD, Riff-Raff would fail to locate the CloudFormation template file, as it simply doesn't exist, and the deployment would fail.

This behaviour is due to allowedStages (correctly) being a property at the root of the riff-raff.yaml file, rather than a property per deployment.

However, there are some niche scenarios where this 4 part validation is too strict.

What does this change?

In this change, we support the, rather niche, use-case of deploying different GuStacks to multiple AWS accounts (aka Riff-Raff stacks), and regions. For example:

class PrimaryWaf extends GuStack {}
class TeamcityWaf extends GuStack {}

new TeamcityWaf({ env: { region: 'eu-west-1' }, stack: 'deploy', stage: 'INFRA' });
new PrimaryWaf({ env: { region: 'eu-west-1' }, stack: 'deploy', stage: 'INFRA' });
new PrimaryWaf({ env: { region: 'eu-west-1' }, stack: 'security', stage: 'INFRA' });
new PrimaryWaf({ env: { region: 'us-east-1' }, stack: 'security', stage: 'INFRA' });

To get this to work, the validation step changes (see changes to isCdkStackPresent).

Previously the requirement was that every combination of GuStack, stack, stage, and region was present in an App, however this is unnecessarily restrictive for the above use-case.

In this change, the validation becomes every combination of stack, and stage are present in an App.

How to test

See the added tests, also https://github.com/guardian/waf/pull/21.

How can we measure success?

We can auto-generate the riff-raff.yaml file for more services, such as https://github.com/guardian/waf.

Have we considered potential risks?

N/A - the current tests continue to pass, so existing uses of this feature will continue to work.

Checklist

  • [ ] I have listed any breaking changes, along with a migration path 1
  • I have updated the documentation as required for the described changes 2

Footnotes

  1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs.

  2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid?

@akash1810 akash1810 changed the title test(riff-raff.yaml): Add a failing test for riff-raff.yaml generation fix(riff-raff.yaml): Loosen restrictions on GuStacks in an App Jan 19, 2023
@akash1810 akash1810 force-pushed the riff-raff-waf branch 3 times, most recently from 9e2b212 to bd365eb Compare January 21, 2023 13:08
@@ -33,7 +176,7 @@ describe("The RiffRaffYamlFileExperimental class", () => {

expect(() => {
new RiffRaffYamlFileExperimental(app);
}).toThrowError("Unable to produce a working riff-raff.yaml file; missing 4 definitions");
}).toThrowError("Unable to produce a working riff-raff.yaml file; missing 1 definitions"); // Stack of media-service has no CODE stage
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this was missing 4 definitions:

PASS src/experimental/riff-raff-yaml-file/index.test.ts
  ● Console

    console.log
      Unable to produce a working riff-raff.yaml file; missing 4 definitions (details below)

      at RiffRaffYamlFileExperimental.validateStacksInApp (src/experimental/riff-raff-yaml-file/index.ts:195:15)

    console.log
      For the class: MyApplicationStack

      at src/experimental/riff-raff-yaml-file/index.ts:197:17
          at Array.forEach (<anonymous>)

    console.log
      ┌───────────────┬────────────────────────────┐
      │    (index)    │         eu-west-1          │
      ├───────────────┼────────────────────────────┤
      │    deploy     │ { CODE: '✅', PROD: '✅' } │
      │ media-service │ { CODE: '❌', PROD: '✅' } │
      └───────────────┴────────────────────────────┘

      at src/experimental/riff-raff-yaml-file/index.ts:198:17
          at Array.forEach (<anonymous>)

    console.log
      For the class: MyDatabaseStack

      at src/experimental/riff-raff-yaml-file/index.ts:197:17
          at Array.forEach (<anonymous>)

    console.log
      ┌───────────────┬────────────────────────────┐
      │    (index)    │         eu-west-1          │
      ├───────────────┼────────────────────────────┤
      │    deploy     │ { CODE: '❌', PROD: '✅' } │
      │ media-service │ { CODE: '❌', PROD: '❌' } │
      └───────────────┴────────────────────────────┘

      at src/experimental/riff-raff-yaml-file/index.ts:198:17

The changes in this PR means only 1 definition is missing:

PASS src/experimental/riff-raff-yaml-file/index.test.ts
  ● Console

    console.log
      Unable to produce a working riff-raff.yaml file; missing 1 definitions (details below)

      at RiffRaffYamlFileExperimental.validateStacksInApp (src/experimental/riff-raff-yaml-file/index.ts:165:15)

    console.log
      ┌───────────────┬──────┬──────┐
      │    (index)    │ CODE │ PROD │
      ├───────────────┼──────┼──────┤
      │    deploy     │ '✅' │ '✅' │
      │ media-service │ '❌' │ '✅' │
      └───────────────┴──────┴──────┘

      at RiffRaffYamlFileExperimental.validateStacksInApp (src/experimental/riff-raff-yaml-file/index.ts:166:15)

@akash1810 akash1810 marked this pull request as ready for review January 21, 2023 15:15
@akash1810 akash1810 requested a review from a team as a code owner January 21, 2023 15:15
akash1810 and others added 3 commits January 26, 2023 09:10
Adds the test case for different GuStacks across multiple AWS accounts.

Co-authored-by: Jacob <jacobwinch@users.noreply.github.com>
Co-authored-by: Thalia <tjsilver@users.noreply.github.com>
Support the use-case where an `App` includes multiple types of `GuStack`.
For example:

```ts
class ServiceRunningInDeployTools extends GuStack {}
class ServiceRunningInSecurity extends GuStack {}

new ServiceRunningInDeployTools(app, "App-CODE-deploy", {
  stack: "deploy",
  stage: "CODE",
});

new ServiceRunningInSecurity(app, "App-CODE-security", {
  stack: "security",
  stage: "CODE",
});
```

It's not clear why this validation was originally added.
The class name is used for the `app` property in a `cloud-formation`
deployment type. This functionality remains, as demonstrated by the tests.

Co-authored-by: Jacob <jacobwinch@users.noreply.github.com>
Co-authored-by: Thalia <tjsilver@users.noreply.github.com>
Supports the case of deploying to multiple stacks, but not necessarily the same region.
* ├───────────────┼──────┼──────┤
* │ deploy │ '✅' │ '✅' │
* │ media-service │ '❌' │ '✅' │
* └───────────────┴──────┴──────┘
* ```
*
* @private
*/
private validateStacksInApp(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to validate and describe the logic in this function as a unit test apart from the higher level behaviour, not blocking for merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

@akash1810 akash1810 merged commit 0af2832 into main Jan 26, 2023
@akash1810 akash1810 deleted the riff-raff-waf branch January 26, 2023 14:15
@github-actions
Copy link
Contributor

🎉 This PR is included in version 49.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants