Open JeremyJonas opened 1 year ago
@JeremyJonas , I really like this idea and I think we should implement it! I have a few suggestions that I think we can work out over the course of the PR.
I think we should mark this is a breaking change, since it will break any user created packs that are currently using any of the exported rules
Not sure what the reasoning behind current abstraction is, would love to find out?
The shortest answer is that this was my first project in TypeScript and there was originally only one pack. 😅
I think we should mark this is a breaking change, since it will break any user created packs that are currently using any of the exported rules
On second thought we can break this into separate PRs. This PR can be focused on changing the internals (as outlined).
I can work on seperate PR afterwards to rewrite the existing rules. That PR can be marked as breaking
@dontirun Yes I agree, the internals could be written to support backward compatibility (support both ways of writing rules and the optionally providing a dictionary on packs to expose them) and keep in v1. It would be a bit extra code, but definitely possible.
You could actually export the new IRule format suggested as "named export" and the old way as "default". That way even developers of rules/packs would not have a breaking change as well :)
export const APIGWAccessLogging: IRule = {
/**
* APIs have access logging enabled
* @param node the CfnResource to check
*/
validate(node: CfnResource): NagRuleCompliance {
// ...
},
name: parse(__filename).name,
info: 'The API does not have access logging enabled.',
explanation: 'Enabling access logs helps operators view who accessed an API and how the caller accessed the API.',
};
// backward compatibility
export default Object.defineProperty(
(node: CfnResource): NagRuleCompliance => APIGWAccessLogging.validate(node),
'name',
{ value: APIGWAccessLogging.name }
);
This definately is a +1
Description
Currently Rules only define a
name
and the Pack must define theinfo
andexplanation
, but this seems redundant if multiple Packs share the same rules. I see some minor differences in these details between packs, but seems trivial (assuming HIPAA for instance has specific info/explanation that differs for compliance terminology maybe).It would be great to have the Rule file contain a default
info
andexplanation
so pack creators don't copy paste from existing packs. And make it so packs can overwrite this default. This would also provide better context in the rule directly rather than relying on the pack.Additionally, following a pattern/interface were Packs expose a dictionary of Rules would be very beneficial to creating subsets of an existing Pack that retains the same details, or creating suppression utils with explicit rule references rather than loosely couple literal names.
The additional benefit of this would be that the RULES.md markdown file could also be auto-generated.
https://github.com/cdklabs/cdk-nag/blob/bd4bb47501203c3052502945451b9168d5c0c3f0/src/packs/aws-solutions.ts#L877-L885
https://github.com/cdklabs/cdk-nag/blob/c43361f58717a5dee1e94ac84318ccf32f6d57e3/src/rules/apigw/APIGWAccessLogging.ts#L10-L46
Use Case
Proposed Solution
Rules
Rule Interface
Rule Implementation
Pack
Pack Rule Interface
NagPack (base)
NagPack (implementation)
Other information
No response
Acknowledge