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

feat: Move the data bucket into the GuCDK stack #622

Merged
merged 2 commits into from
Jun 25, 2021
Merged

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented Jun 24, 2021

Builds on #598.

What does this change?

Migrates the AmigoDataBucket resource into the GuCDK version of the stack.

In order to do this, we need to also move any resource that references AmigoDataBucket. There's only one resource AmigoAppPolicy.

Whilst we could migrate AmigoAppPolicy in this change too, it's preferable not to as the change set is pretty big and difficult to reason about.

I couldn't work out how to mutate a YAML defined resource from within CDK, so instead an additional policy is being added. I plan to migrate the remaining policies defined in AmigoAppPolicy to it in the next PR.

One thing to note is the bucket name is "changing" from a Fn::Sub to a mapping lookup, it still resolves to the same value. This causes cdk diff (via ./script/diff) to suggest this is a replacement operation:

[~] AWS::S3::Bucket AmigoDataBucket AmigoDataBucket replace
 ├─ [~] BucketName (requires replacement)
 │   ├─ [+] Added: .Fn::FindInMap
 │   └─ [-] Removed: .Fn::Sub

However it really isn't, as seen in the JSON change set below - there are only AWS::IAM::Policy changes. (Technically, the gu:cdk:version tag also updates as the library version has been bumped, however I applied that separately as it's a no-op).

Change set JSON from CODE
[
  {
    "resourceChange": {
      "logicalResourceId": "AmigoAppPolicy",
      "action": "Modify",
      "physicalResourceId": "amigo-Amig-1E4GKDN7I64ZN",
      "resourceType": "AWS::IAM::Policy",
      "replacement": "False",
      "moduleInfo": null,
      "details": [
        {
          "target": {
            "name": "PolicyDocument",
            "requiresRecreation": "Never",
            "attribute": "Properties"
          },
          "causingEntity": null,
          "evaluation": "Static",
          "changeSource": "DirectModification"
        }
      ],
      "changeSetId": null,
      "scope": [
        "Properties"
      ]
    },
    "hookInvocationCount": null,
    "type": "Resource"
  },
  {
    "resourceChange": {
      "logicalResourceId": "AppPolicyF941AEC5",
      "action": "Add",
      "physicalResourceId": null,
      "resourceType": "AWS::IAM::Policy",
      "replacement": null,
      "moduleInfo": null,
      "details": [],
      "changeSetId": null,
      "scope": []
    },
    "hookInvocationCount": null,
    "type": "Resource"
  }
]

I think cdk simply follows the Cloudformation spec:

BucketName
Update requires: Replacement

Lastly @guardian/cdk gets updated to 19.3.0 to get the new GuS3Bucket construct (guardian/cdk#635).

How to test

Deploy this branch to CODE, we should still be able to list the packages installed during a bake.

How can we measure success?

We move closer to a CDK only template.

@akash1810 akash1810 merged commit 3b55adc into main Jun 25, 2021
@akash1810 akash1810 deleted the aa-data-bucket branch June 25, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants