Conversation
iliapolo
added
status/review
Co-authored-by: Eli Polonsky <epolon@amazon.com>
iliapolo
left a comment
iliapolo
left a comment
There was a problem hiding this comment.
@otaviomacedo @corymhall I think the major decisions in the RFC are solid. That is:
- The decision not to use CloudFormation hooks.
- The decision to invoke validations in the framework.
Can we move on to API discussions?
Co-authored-by: Eli Polonsky <epolon@amazon.com>
text/0477-policy-validation.md
Outdated
| * Developer writes a CDK application without the correct validations config. | ||
| * Developer deploys non-compliant stacks. | ||
| * Deployment guardrails catch these violations, and instruct the developer to add a validations property to their | ||
| application. | ||
| * Developer adds the validations property, and avoids these violations going forward. |
There was a problem hiding this comment.
I think this is covered? Whaddayatrynasay?
There was a problem hiding this comment.
Are you talking about all the bullet points or just the last one?
| How to fix: | ||
| > Using override `app.findChild('my-bucket').addPropertyOverride('SSEAlgorithm', 'aws:kms');` | ||
| > Add to construct properties for `cdk-app/MyStack/Bucket` | ||
| `encryption: BucketEncryption.KMS` |
There was a problem hiding this comment.
I like this, but this needs be propped further up front that there will be hints attached for fixing violations, and that's one of the value adds of this feature!
|
|
||
| Recommendation: Missing value for key `SSEAlgorithm` - must specify `aws:kms` | ||
| How to fix: | ||
| > Using override `app.findChild('my-bucket').addPropertyOverride('SSEAlgorithm', 'aws:kms');` |
There was a problem hiding this comment.
Oh my God can we generate the correct code here? That'd be neat!
There was a problem hiding this comment.
Yes I think we can provide the correct code to use, but we may or may not be able to generate it depending on the plugin/tool. I'm not sold on the escape hatch mechanism here, it would be much better if we could provide instructions on how to update the actual construct in question.
1 thing we could do is we could know whether or not the user is using one of our L2 constructs. If they are then we "know" how to fix all of the issues. It would probably require hand written instructions (unless we had a way to trace a CFN property back to the L2 property).
updating section on exemptions/suppressions
text/0477-policy-validation.md
Outdated
|
|
||
| // globally for the entire app (an app is a stage) | ||
| const app = new App({ | ||
| validationPlugins: [ |
There was a problem hiding this comment.
This has a potential to extend into lifecycle phase based plugins (App Lifecycle) Something to the effect of
const app = new App({
plugins: [{
phase: "postSynthesis",
plugin: new CfnGuardValidator(...)
},...
]
Co-authored-by: Eli Polonsky <epolon@amazon.com>
Updated the part about human readable format.
text/0477-policy-validation.md
Outdated
|
|
||
| ```ts | ||
| const app = new App({ | ||
| context: { '@aws-cdk/core:validationReportJson': true }, |
There was a problem hiding this comment.
This is a bit strange. Normally JSON output is needed in order for a machine to parse the output, but running cdk synth also produces additional stuff that will make the output non parsable. That is, it will contains some lines, then a JSON report, and some more lines?
I think its going to be hard to make sure the CLI doesn't print anything when this feature is turned on, so I would suggest allowing to pass a file where the JSON report will be written to. Is this possible via context?
There was a problem hiding this comment.
If the report returns anything that means it fails and synth will not write anything to stdout. The only thing written to stdout will be the report.
text/0477-policy-validation.md
Outdated
|
|
||
| ```ts | ||
| const app = new App({ | ||
| context: { '@aws-cdk/core:validationReportJson': true }, |
There was a problem hiding this comment.
Side note: why @aws-cdk/core? I think we should steer clear of that prefix.
There was a problem hiding this comment.
Just trying to stay consistent and match the feature flags https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.cx_api-readme.html.
| The `validate` method returns an instance of `ValidationPluginReport`, which | ||
| tells the CDK whether the template is compliant, which violations were found | ||
| (if any), and any metadata about the report. These are the report related | ||
| interfaces: |
There was a problem hiding this comment.
nit: this should be in the appendix or design sections, not the working backwards.
Co-authored-by: Eli Polonsky <epolon@amazon.com>
otaviomacedo
added
the
status/api-approved
API approved.
otaviomacedo
added
status/implementing
5 participants
This is a request for comments about validation of policies by the CDK at synth time. See #477 for additional details.
APIs are signed off by @iliapolo.
Rendered version
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license