Add KmsKeyId to LogGroup #27

benbridts · 2020-03-25T16:59:28Z

~~This is a Work in Progress, if that's better as an issue, let me know.~~

I tried to tackle the difficult parts first, which for me besides the whole of Java is writing test. I haven't started writing code yet. Assistance with both parts is very welcome.

I'm opening this PR mainly to get feedback on the approach. Currently I have the following questions related to the work already in here:

Spec is step 1, so that can already get a decent review :)
Is this the way to update the tests, or do we prefer completely new tests that only deal with the KmsKeyId

I also have question for the work that isn't in here yet:

There are three related methods in the API: "CreateLogGroup" with the "kmsKeyId" parameter, "AssociateKmsKey" and "DisassociateKmsKey"
This allows to approaches in the CreateHandler. Doing it in two calls (CreateLogGroup without kmsKeyId and AssociateKmsKey if it's not Null on the model), which leaves the LogGroup without encryption for some time (and in that time some messages might appear). Or doing it in one call, which uses a different flow for the createHandler and the updateHandler.
In the updateHandler we have to do it in two steps, which is something that we either shouldn't support (only allowing going from encrypted to plaintext and vice versa) or should highlight in the documentation. The risk of log lines arriving while this change is in progress is even higher than with the CreateHandler.

We should probably decide on the best approach before writing the code.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

rjlohan · 2020-03-25T17:05:32Z

Not super important, but you can do 'Draft' PRs to signal WIP. We can review them entirely, but they're not merge-able.

rjlohan

I'm inclined to use the inline kmsKeyId argument for CreateLogGroup in the CreateHandler and then use Associate/Disassociate in the Update handler.

aws-logs-loggroup/aws-logs-loggroup.json

rjlohan · 2020-03-25T17:13:45Z

aws-logs-loggroup/aws-logs-loggroup.json

+            "description": "The Amazon Resource Name (ARN) of the CMK to use when encrypting log data.",
+            "type": "string",
+            "maxLength": 256,
+            "pattern": "^arn:[a-z0-9-]+:kms:[a-z0-9-]+:\\d{12}:(key|alias)/.+\\Z"


Will want to come back to this so we can $ref an AWS::KMS::Key resource's Arn property when that's available.

benbridts · 2020-03-29T22:16:56Z

Current blocked from (easily/comfortably) writing the implementation by #25 (and a little bit by #28, but I got that working locally by pulling in #29 )

rjlohan · 2020-03-30T15:57:03Z

If you merge from master, you'll get a Travis build for this PR from now on. 👍

Also, to be consistent with other repos, I updated the space indenting for JSON files from 4-->2 so you'll need to run pre-commit run --all to fix the schema file after rebasing/merging.

rjlohan · 2020-04-16T00:04:50Z

@ikben how's this PR going?

benbridts · 2020-04-17T19:30:58Z

I didn't get to pulling in the recent changes yet, hopefully I will have some time to look at it in the next few days

Does not play nice with precommit

- Add missing PutRetentionPolicy to permissions - Remove extra space from LogGroupName description - Make formatting consitent with surrounding code

benbridts · 2020-04-25T21:59:26Z

I think this is what the implementation would look like. Please note:

contract tests still fail, because of LogGroup: Not returning ARN on create and update #25 additionally they now also do not return the KmsKeyId (for the same reason). The ARN is the bigger issue in my opinion, until there is a decision made about haw to tackle that, it does not make a lot of sense to fix that here.
7596883 and ff7dc2d are useful even without adding the KmsKeyId, so if it turns out that there is still a lot more work on this, they can be cherry-picked.
I wanted to add the docs (similar to how resource-role.yaml is included), but that file does not have a newline at the end.

rjlohan · 2020-05-08T03:08:51Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

@@ -28,6 +30,15 @@
            putRetentionPolicy(proxy, request, logger);
        }

+        // It can take up to five minutes for the (dis)associate operation to take effect
+        // It's unclear from the documentation if that state can be checked via the API.
+        // https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/encrypt-log-data-kms.html


Are we emitting ProgressEvents with IN_PROGRESS to check on the status or it seems like that's not actually discoverable?

Where does the 5 mins come from? As far as I see there is no such status in the log group. It may take some time for data plane to take effect, but from control plane perspective, it's done once API call successful.

The docs say "After you associate or disassociate a CMK from a log group, it can take up to five minutes for the operation to take effect." To me that sounds like the data plane needing some time, like Wenbing described,and what I assumed when writing the code. There is no separate API call to check on the status and it's hard to test because "it is immediately present when I do a read call", still falls into the "up to 5 minutes" bucket.

wbingli · 2020-05-08T04:03:52Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

@@ -28,6 +30,15 @@
            putRetentionPolicy(proxy, request, logger);
        }

+        // It can take up to five minutes for the (dis)associate operation to take effect
+        // It's unclear from the documentation if that state can be checked via the API.
+        // https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/encrypt-log-data-kms.html


Where does the 5 mins come from? As far as I see there is no such status in the log group. It may take some time for data plane to take effect, but from control plane perspective, it's done once API call successful.

aws-logs-loggroup/README.md

miparnisari · 2020-05-08T19:53:46Z

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/Translator.java

        return ResourceModel.builder()
                .arn(logGroupArn)
                .logGroupName(logGroupName)
                .retentionInDays(retentionInDays)
+                .kmsKeyId(kmsKeyId)


From the contract: "A read request SHOULD NOT return properties that have null values.".

True, I followed the code above (lines 69-73), that also breaks that rule

Actually, I might need some java help here. I'd assume the rpdk would take care of that, since all the properties are properties of the ResourceModel class

Hmm. I don't think the rpdk is checking this. @rjlohan do you have any recommendations here? Other than checking for null before building the model's properties?

@rjlohan Do you have any feedback on this?

The real reason for this is to disambiguate these scenarios;

property not set/present

property explicitly set to null/empty

It's not a strict requirement, hence it's a SHOULD NOT rather than a MUST NOT. I'm ok to proceed as is, as it's not strictly a relevant distinction in this case.

aws-logs-loggroup/src/main/java/software/amazon/logs/loggroup/UpdateHandler.java

aws-logs-loggroup/README.md

Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

aws-logs-loggroup/README.md

(suggestion from code review) Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

benbridts · 2020-06-14T22:11:35Z

I rewrote a few if statements and added a bunch of tests to get the required coverage.

The issue about having model properties that can be null is still open, but I'll wait on feedback for the best approach there

Lukas-Franz · 2020-08-03T08:03:52Z

Thanks for the implementation! We've just tested this out and it seems not to be in production as of yet.

A bit of topic, but important nonethelss: Is there a way to get notified or keep track of changes like these as they make their way through quality assurance and to production? As far as I'm aware, the only way right now is trial and error, which is quite frustrating.

benbridts · 2020-08-03T18:54:45Z

@OfficialTermi

The best way to see that should be the CloudFormation Coverage Roadmap Issue. Unfortunately, that issue hasn't been moved to the roadmap yet (for the AWS-folks here, is the coverage roadmap updated if the PR is coming from outside of AWS?)
That does not mean you have to do it with trial and error though. You can look at:
- The CloudFormation Release History (it also has an RSS feed)
- The CloudFormation Resource Specification
- The API, using the CLI to describe the type definition and check the Schema for "KmsKeyId": aws cloudformation describe-type --type RESOURCE --type-name AWS::Logs::LogGroup | grep KmsKeyId

miparnisari · 2020-09-04T20:41:20Z

aws-logs-loggroup/dev-resources.yaml

+  CmkAlias:
+    Type: "AWS::KMS::Alias"
+    Properties:
+      AliasName: !Sub "arn:${AWS::Partition}:kms:${AWS::Region}:${AWS::AccountId}:alias/${AWS::StackName}"


@ikben this is giving me

"LogicalResourceId": "CmkAlias", "ResourceType": "AWS::KMS::Alias" "ResourceStatus": "CREATE_FAILED", "ResourceStatusReason": "Model validation failed (#/AliasName: failed validation constraint for keyword [pattern])"

See #50

@miparnisari Thanks for the fix. Yours does follow what's in the documentation.

At the time of writing, it did deploy though (I checked the deleted stack in my account, it has the long !Sub). So make sure that it's not the StackName that is causing the failure.

rjlohan reviewed Mar 25, 2020

View reviewed changes

rjlohan added the enhancement New feature or request label Mar 29, 2020

benbridts added 4 commits April 25, 2020 23:50

Ignore docs for now

7596883

Does not play nice with precommit

Improve surrounding and Existing Code

ff7dc2d

- Add missing PutRetentionPolicy to permissions - Remove extra space from LogGroupName description - Make formatting consitent with surrounding code

Add KmsKeyId to LogGroup

468a73d

Update Readme

8149540

benbridts changed the title ~~[WIP] add KmsKeyId to LogGroup~~ Add KmsKeyId to LogGroup Apr 25, 2020

wbingli mentioned this pull request May 4, 2020

Adding Testing Support for Java based handlers aws-cloudformation/cloudformation-cli-java-plugin-testing-support#1

Merged

rjlohan reviewed May 8, 2020

View reviewed changes

wbingli approved these changes May 8, 2020

View reviewed changes

miparnisari suggested changes May 8, 2020

View reviewed changes

PatMyron mentioned this pull request May 14, 2020

AWS::Logs::LogGroup-NewAttribute (KMSKeyId) aws-cloudformation/cloudformation-coverage-roadmap#128

Closed

PatMyron reviewed May 14, 2020

View reviewed changes

aws-logs-loggroup/README.md Outdated Show resolved Hide resolved

benbridts and others added 2 commits May 24, 2020 22:10

Make testing instructions easier to follow

98a1885

Update with suggestion from code review

8294157

Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

miparnisari reviewed May 26, 2020

View reviewed changes

aws-logs-loggroup/README.md Outdated Show resolved Hide resolved

benbridts mentioned this pull request May 26, 2020

Catch all errors that can be thrown by the AWS SDK calls #36

Open

miparnisari approved these changes Jun 14, 2020

View reviewed changes

benbridts and others added 3 commits June 15, 2020 00:03

Check previous resource model, to not remove twice

a82f71b

(suggestion from code review) Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

Apply suggestions from code review

b6f154c

Co-authored-by: Maria Ines Parnisari <mparnisa@amazon.com>

Catch all exceptions when updating the KmsKeyId

e4c4fc6

mascah mentioned this pull request Jun 18, 2020

Support encrypted CW log groups serverless/serverless#4565

Open

miparnisari merged commit 8b9229f into aws-cloudformation:master Jul 15, 2020

benbridts deleted the kms-key-id branch July 15, 2020 21:28

miparnisari reviewed Sep 4, 2020

View reviewed changes

miparnisari mentioned this pull request Sep 4, 2020

[LogGroup] Fix 'Model validation failed' for dev-resources.yaml #50

Closed

PatMyron mentioned this pull request Feb 6, 2021

catch common resource schema issues in cfn validate aws-cloudformation/cloudformation-cli#663

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add KmsKeyId to LogGroup #27

Add KmsKeyId to LogGroup #27

benbridts commented Mar 25, 2020 •

edited

Loading

rjlohan commented Mar 25, 2020

rjlohan left a comment

rjlohan Mar 25, 2020

benbridts commented Mar 29, 2020

rjlohan commented Mar 30, 2020 •

edited

Loading

rjlohan commented Apr 16, 2020

benbridts commented Apr 17, 2020

benbridts commented Apr 25, 2020 •

edited

Loading

rjlohan May 8, 2020

wbingli May 8, 2020

benbridts May 8, 2020

wbingli May 8, 2020

miparnisari May 8, 2020

benbridts May 24, 2020 •

edited

Loading

benbridts May 24, 2020

miparnisari May 26, 2020

benbridts Jun 14, 2020

rjlohan Jul 15, 2020 •

edited

Loading

benbridts commented Jun 14, 2020 •

edited

Loading

Lukas-Franz commented Aug 3, 2020

benbridts commented Aug 3, 2020

miparnisari Sep 4, 2020 •

edited

Loading

benbridts Sep 5, 2020

Add KmsKeyId to LogGroup #27

Add KmsKeyId to LogGroup #27

Conversation

benbridts commented Mar 25, 2020 • edited Loading

rjlohan commented Mar 25, 2020

rjlohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbridts commented Mar 29, 2020

rjlohan commented Mar 30, 2020 • edited Loading

rjlohan commented Apr 16, 2020

benbridts commented Apr 17, 2020

benbridts commented Apr 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbridts May 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjlohan Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

benbridts commented Jun 14, 2020 • edited Loading

Lukas-Franz commented Aug 3, 2020

benbridts commented Aug 3, 2020

miparnisari Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbridts commented Mar 25, 2020 •

edited

Loading

rjlohan commented Mar 30, 2020 •

edited

Loading

benbridts commented Apr 25, 2020 •

edited

Loading

benbridts May 24, 2020 •

edited

Loading

rjlohan Jul 15, 2020 •

edited

Loading

benbridts commented Jun 14, 2020 •

edited

Loading

miparnisari Sep 4, 2020 •

edited

Loading