feat(config): add constructs for managed and custom rules #2326

jogold · 2019-04-18T14:58:23Z

Specific classes are exposed for an AWS managed rule (ManagedRule) and a custom rule
(CustomRule) with support for compliance and re-evaluation events.

Higher level constructs for configured AWS managed rules are also exposed (starting with Access
Keys Rotated and CloudFormation rules). This defines a pattern for future configured managed rules.

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Specific classes are exposed for an AWS managed rule (`ManagedRule`) and a custom rule (`CustomRule`) with support for compliance and re-evaluation events. Higher level constructs for configured AWS managed rules are also exposed (starting with Access Keys Rotated and CloudFormation rules). This defines a pattern for future configured managed rules.

rix0rrr

Solid work as always Jonathan. Thanks!

Just some small things

packages/@aws-cdk/aws-config/README.md

packages/@aws-cdk/aws-config/lib/managed-rules.ts

rix0rrr · 2019-05-21T11:38:34Z

packages/@aws-cdk/aws-config/lib/rule.ts

+   * The arn of the rule.
+   * @attribute
+   */
+  readonly configRuleArn?: string;


Is this really optional?

I don't need configRuleArn, configRuleId or configRuleComplianceType when working with an imported rule. And I don't directly see why a user importing a rule would have to specify those.

Using only configRuleName, I can set up the onEvent(), onComplianceChange() and onReEvaluationStatus() for both new and imported rules.

If remove those properties (configRuleArn, configRuleId, configRuleComplianceType) from the IRule interface, I'm faced with the new awslint:attribute-tag rule errors:

error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackDriftDetectionCheck.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleArn error: [awslint:attribute-tag:@aws-cdk/aws-config.ManagedRule.configRuleId] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleId error: [awslint:attribute-tag:@aws-cdk/aws-config.ManagedRule.configRuleComplianceType] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleComplianceType error: [awslint:attribute-tag:@aws-cdk/aws-config.ManagedRule.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleArn error: [awslint:attribute-tag:@aws-cdk/aws-config.CustomRule.configRuleId] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.CustomRule.configRuleId error: [awslint:attribute-tag:@aws-cdk/aws-config.CustomRule.configRuleComplianceType] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.CustomRule.configRuleComplianceType error: [awslint:attribute-tag:@aws-cdk/aws-config.CustomRule.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.CustomRule.configRuleArn error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackNotificationCheck.configRuleId] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleId error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackNotificationCheck.configRuleComplianceType] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleComplianceType error: [awslint:attribute-tag:@aws-cdk/aws-config.AccessKeysRotated.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleArnerror: [awslint:attribute-tag:@aws-cdk/aws-config.AccessKeysRotated.configRuleComplianceType] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleComplianceType error: [awslint:attribute-tag:@aws-cdk/aws-config.AccessKeysRotated.configRuleId] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleId error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackNotificationCheck.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleArn error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackDriftDetectionCheck.configRuleComplianceType] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleComplianceType error: [awslint:attribute-tag:@aws-cdk/aws-config.CloudFormationStackDriftDetectionCheck.configRuleId] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleId

If I add /** @attribute */ on the abstract properties of the abstract class RuleNew, the linter is not happy. The only valid alternative is to add /** @attribute */ both in ManagedRule and CustomRule properties.

What is the guideline here?

Good point, that has to do with the base class being invisible. I have created an issue for it: aws/jsii#510

For now, best I can suggest is to put the tag on the individual rules. It feels a little silly, but that seems better to me than making it optional in the interface for the purposes of it being absent for an imported resources -- the interface is exactly meant to define the interface that can be relied upon to always be there.

I just tested, and it seems like it should be inherited properly from the RuleNew class as long as you avoid adding docstrings to the subtype.

So if there are no docstrings on the inherited members, you can do the full docs on the base member.

You mean like this? Can't get this to work...

/** * A new managed or custom rule. */ abstract class RuleNew extends RuleBase { /** * The arn of the rule. * * @attribute */ public abstract readonly configRuleArn: string;

/** * A new managed rule. * * @resource AWS::Config::ConfigRule */ export class ManagedRule extends RuleNew { public readonly configRuleName: string; public readonly configRuleArn: string;

error: [awslint:attribute-tag:@aws-cdk/aws-config.ManagedRule.configRuleArn] attribute properties must have an "@attribute" doctag on: @aws-cdk/aws-config.ManagedRule.configRuleArn

rix0rrr · 2019-05-21T11:38:41Z

packages/@aws-cdk/aws-config/lib/rule.ts

+   * The id of the rule.
+   * @attribute
+   */
+  readonly configRuleId?: string;


Is this really optional?

rix0rrr · 2019-05-21T11:38:45Z

packages/@aws-cdk/aws-config/lib/rule.ts

+   * The compliance status of the rule.
+   * @attribute
+   */
+  readonly configRuleComplianceType?: string;


Is this really optional?

packages/@aws-cdk/aws-config/lib/rule.ts

rix0rrr · 2019-05-21T11:41:32Z

packages/@aws-cdk/aws-config/lib/rule.ts

+   * @param value the tag value
+   */
+  public addTagScope(key: string, value?: string) {
+    this.addScope({


Maybe a check to confirm that resource and tag scopes aren't mixed?

Can you clarify?

Currently the last call to addXxx() sets the scope. This is to allow setting the scope even if it's already set by a managed rule, e.g. in CloudFormationStackNotificationCheck the scope is set to AWS::CloudFormation::Stack but a user could want to restrict it to a specific resource or tag by calling .addXxx():

const stackNotificationsCheck = new CloudFormationStackNotificationCheck(this, 'Check'); // The scope is set to AWS::CloudFormation::Stack stackNotificationsCheck.addTagScope('stage', 'prod'); // Changed the scope stage=prod // addScope() of RuleNew has now been called twice

I see. The addXxx name confused me, seemed like they would be cumulative. I assume this is because jsii won't let you name a method like setXxx, right?

I think I would rather have names like scopeToTag(), scopeToResource() etc. then.

Yes exactly, tried with setXxx. OK for scopeToXxx.

jogold requested a review from a team as a code owner April 18, 2019 14:58

jogold added 13 commits April 24, 2019 17:18

Merge branch 'master' into configservice

9cc50d8

use aws-events-targets in tests

178397a

Merge branch 'master' into configservice

31c8ed0

make abstract classes internal and ImportedRule class local

f11337d

Merge branch 'master' into configservice

72e5e07

fromRuleName

608de3f

JSDoc and readonly

2816f55

Merge branch 'master' into configservice

def7878

@resource and @Attribute

84adef0

Merge branch 'master' into configservice

1b0c7ba

Merge branch 'master' into configservice

9ff64ad

ITopic

d8f8fef

remove export()

bc4a0f7

rix0rrr reviewed May 21, 2019

View reviewed changes

jogold added 9 commits May 21, 2019 14:17

typo

36aadd1

remove remaining export()

56e7be9

full link to Managed Rules

156d2c0

support IRole for drifts and update role following doc update

66bd7fc

better role

34fd2b9

Merge branch 'master' into configservice

c240d5c

EventRule > Rule

1a67868

remove optionals from IRule

7469643

addXxx() > scopeToXxx()

22a21de

rix0rrr approved these changes May 23, 2019

View reviewed changes

rix0rrr merged commit deed353 into aws:master May 23, 2019

jogold deleted the configservice branch May 23, 2019 08:47

jogold mentioned this pull request May 23, 2019

fix: update all 'onXxx' methods to be CloudWatch Events #2609

Merged

4 tasks

snyk-bot mentioned this pull request Aug 22, 2019

[Snyk] Upgrade @aws-cdk/aws-certificatemanager from 0.18.1 to 0.39.0 udinachmany/trivia-api#1

Open

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): add constructs for managed and custom rules #2326

feat(config): add constructs for managed and custom rules #2326

jogold commented Apr 18, 2019

rix0rrr left a comment •

edited

Loading

rix0rrr May 21, 2019

jogold May 21, 2019

rix0rrr May 22, 2019

rix0rrr May 22, 2019

jogold May 22, 2019 •

edited

Loading

rix0rrr May 21, 2019

rix0rrr May 21, 2019

rix0rrr May 21, 2019

jogold May 21, 2019

rix0rrr May 22, 2019

jogold May 22, 2019

feat(config): add constructs for managed and custom rules #2326

feat(config): add constructs for managed and custom rules #2326

Conversation

jogold commented Apr 18, 2019

Pull Request Checklist

rix0rrr left a comment • 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

jogold May 22, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment •

edited

Loading

jogold May 22, 2019 •

edited

Loading