feat(config): add custom policy rule constructs #21794
Conversation
|
|
TheRealAmazonKendra
changed the title
There was a problem hiding this comment.
Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests).
Please update and then we will provide a meaningful review.
There was a problem hiding this comment.
Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests).
Please update and then we will provide a meaningful review.
|
Sorry for the lack of intent in the explanation. I described the awareness of the issues and the intention to realize this function developed. |
There was a problem hiding this comment.
Thank you for your contribution! Overall this is great work and we appreciate you taking the time to do the implementation! I just have a few comments inline.
Additionally, overall, I think there's some room in rule.ts to remove duplication of code. If you are open to taking that on, I'd definitely love to see some refactoring.
| rule tableisactive when | ||
| resourceType == "AWS::DynamoDB::Table" { | ||
| configuration.tableStatus == %status | ||
| } | ||
| rule checkcompliance when | ||
| resourceType == "AWS::DynamoDB::Table" | ||
| tableisactive { | ||
| let pitr = supplementaryConfiguration.ContinuousBackupsDescription.pointInTimeRecoveryDescription.pointInTimeRecoveryStatus | ||
| %pitr == "ENABLED" | ||
| } |
There was a problem hiding this comment.
Can we model these rules in a way that they can be implemented via code instead of just being blocks of code? I get that they're custom rules so there may be a lot of complexity there, so if it doesn't make sense, let me know.
There was a problem hiding this comment.
Sorry if my understanding is wrong. You need to attempt to parse the GuardDSL in the link below into a grammar in CDK.
https://github.com/aws-cloudformation/cloudformation-guard/blob/main/docs/CLAUSES.md
I am aware that this is a complex effort at this stage, and while it is great as a loadmap, would it be acceptable at this stage to work on it without a detailed parsing of the string as we do with other CustomRules?
There was a problem hiding this comment.
The only problem with that approach is that, if we're changing the contract in the future, that's a breaking change. I suppose we can leave the string option and/or deprecate it so that it's not technically breaking. Considering that this is how we implement else where, though, I suppose I'm fine with this.
| configRuleName: this.physicalName, | ||
| description: props.description, | ||
| inputParameters: props.inputParameters, | ||
| scope: Lazy.any({ produce: () => renderScope(this.ruleScope) }), // scope can use values such as stack id (see CloudFormationStackDriftDetectionCheck) |
There was a problem hiding this comment.
Let's add an additional integration test that uses something like stackId where it needs to be produced late.
There was a problem hiding this comment.
@TheRealAmazonKendra
This comment is actually still copied from CustomRule.
I am reading the documentation that might be relevant, but there doesn't appear to be an i/f where I can enter a stackid.
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_config.RuleScope.html
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_config.CloudFormationStackDriftDetectionCheck.html
As a suggestion, can we remove this misleading comment from CustomRule and CustomPolicy.
There was a problem hiding this comment.
The comment looks accurately written. Render scope does the following
function renderScope(ruleScope?: RuleScope): CfnConfigRule.ScopeProperty | undefined {
return ruleScope ? {
complianceResourceId: ruleScope.resourceId,
complianceResourceTypes: ruleScope.resourceTypes?.map(resource => resource.complianceResourceType),
tagKey: ruleScope.key,
tagValue: ruleScope.value,
} : undefined;
}
So, if the resource that this scope is being assigned to doesn't yet have its resource id created at the time of this policy rule being declared, this needs to produce that information after that resource has been created. Basically, if this is declared.
This test can be written by creating a resource of some sort and then making the rule scope specific to that resource by adding the id as the second input for fromResource. It will have to be produced via Lazy.
watany-dev
force-pushed
the
add-config-custom-policy
branch
from
a2a16c7 to
50611ad
Compare
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
| * that triggers AWS Config to evaluate your AWS resources. | ||
| * | ||
| */ | ||
| readonly eventSource : string; |
There was a problem hiding this comment.
| readonly eventSource : string; | |
| readonly eventSource: string; |
| /** | ||
| * The frequency at which you want AWS Config to run evaluations for a custom rule with a periodic trigger. | ||
| */ | ||
| readonly maximumExecutionFrequency? : MaximumExecutionFrequency; |
There was a problem hiding this comment.
| readonly maximumExecutionFrequency? : MaximumExecutionFrequency; | |
| readonly maximumExecutionFrequency?: MaximumExecutionFrequency; |
| /** | ||
| * The type of notification that triggers AWS Config to run an evaluation for a rule. | ||
| */ | ||
| readonly messageType : MessageType; |
There was a problem hiding this comment.
| readonly messageType : MessageType; | |
| readonly messageType: MessageType; |
| configRuleName: this.physicalName, | ||
| description: props.description, | ||
| inputParameters: props.inputParameters, | ||
| scope: Lazy.any({ produce: () => renderScope(this.ruleScope) }), // scope can use values such as stack id (see CloudFormationStackDriftDetectionCheck) |
There was a problem hiding this comment.
The comment looks accurately written. Render scope does the following
function renderScope(ruleScope?: RuleScope): CfnConfigRule.ScopeProperty | undefined {
return ruleScope ? {
complianceResourceId: ruleScope.resourceId,
complianceResourceTypes: ruleScope.resourceTypes?.map(resource => resource.complianceResourceType),
tagKey: ruleScope.key,
tagValue: ruleScope.value,
} : undefined;
}
So, if the resource that this scope is being assigned to doesn't yet have its resource id created at the time of this policy rule being declared, this needs to produce that information after that resource has been created. Basically, if this is declared.
This test can be written by creating a resource of some sort and then making the rule scope specific to that resource by adding the id as the second input for fromResource. It will have to be produced via Lazy.
| this.ruleScope = props.ruleScope; | ||
|
|
||
| sourceDetails.push({ | ||
| eventSource: 'aws.config', |
There was a problem hiding this comment.
Are any other event sources possible?
There was a problem hiding this comment.
https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-config-configrule-source-sourcedetails.html
After checking the document, it seems that only aws.config is entered. Define it as an enum like MessageType
watany-dev
force-pushed
the
add-config-custom-policy
branch
from
50611ad to
bef9728
Compare
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
watany-dev
force-pushed
the
add-config-custom-policy
branch
from
bef9728 to
d05b24b
Compare
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
mergify
bot
dismissed
TheRealAmazonKendra’s stale review
Pull request has been modified.
|
@Mergifyio update |
☑️ Nothing to do
|
|
Odd that it updated in the opposite direction. Did you build first with the most updated changes from main? |
|
Hmmm, the BUILD may have been leaking. I will try the buildup again. |
|
@TheRealAmazonKendra /workspace/aws-cdk/packages/aws-cdk: yarn build (55 remaining)
yarn run v1.22.19
$ cdk-build
[eslint-import-resolver-typescript]: option `directory` is deprecated, please use `project` instead
Validating circular imports
Validating resources
Validating attributions
In package package.json
- [@aws-cdk/node-bundle => outdated-attributions] THIRD_PARTY_LICENSES is outdated (fixable)
Error: Some package.json files had errors
at main (/workspace/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:30:15)
at Object.<anonymous> (/workspace/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint.js:33:1)
at Module._compile (internal/modules/cjs/loader.js:1085:14)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
at Module.load (internal/modules/cjs/loader.js:950:32)
at Function.Module._load (internal/modules/cjs/loader.js:790:12)
at Module.require (internal/modules/cjs/loader.js:974:19)
at require (internal/modules/cjs/helpers.js:101:18)
at Object.<anonymous> (/workspace/aws-cdk/tools/@aws-cdk/pkglint/bin/pkglint:2:1)
at Module._compile (internal/modules/cjs/loader.js:1085:14)
Error: pkglint exited with error code 1
Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error: last command failed. fix problem and resume by executing: /workspace/aws-cdk/scripts/foreach.sh
directory: /workspace/aws-cdk/packages/aws-cdk |
|
I'll try a full build just to be sure. |
watany-dev
force-pushed
the
add-config-custom-policy
branch
from
415b303 to
12ce62a
Compare
Ah, yes. Just run |
|
Thank you. |
|
@TheRealAmazonKendra |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
feat(config) aws#21441 I have created a `new config.CustomPolicy` so that this functionality is available in L2 Constructs. The resources that can currently be created with `AWS::Config::ConfigRule` can be created with `config.CustomRule` and `config.ManagedRule` in the CDK. This is because the restrictions on the various properties are different. CustomPolicy has different constraints compared to CustomRule as follows. - There is a restriction on the format that can be selected in `SourceDetails`. - [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-config-configrule-source.html) - Properties that refer to Lambda are unnecessary. - `CustomPolicyDetails` must be specified. - [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-config-configrule-source-sourcedetails.html) To avoid this limitation and complexity, `CustomPolicy` can be separated, making it more convenient for users. It also reduces the dependence on each rule type for updates during maintenance. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [x] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
feat(config) #21441
I have created a
new config.CustomPolicyso that this functionality is available in L2 Constructs.The resources that can currently be created with
AWS::Config::ConfigRulecan be created withconfig.CustomRuleandconfig.ManagedRulein the CDK. This is because the restrictions on the various properties are different.CustomPolicy has different constraints compared to CustomRule as follows.
SourceDetails.CustomPolicyDetailsmust be specified.To avoid this limitation and complexity,
CustomPolicycan be separated, making it more convenient for users. It also reduces the dependence on each rule type for updates during maintenance.All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license