feat: Simpler appliesTo syntax for referencing CDK nodes
Opened this issue · 1 comments
Description
Support passing unresolved strings to appliesTo.
Use Case
This commonly applies to cases where L2 constructs add wildcard resources to roles. For example, the .grantInvoke method on L2 Lambda Function construct adds [this.functionArn, `${this.functionArn}:*`]
.
The idea is to support the following syntax, which is more consistent with how the rest of CDK works.
NagSuppressions.addResourceSuppressions(
lambdaInvokeRole,
[{
id: 'AwsSolutions-IAM5',
reason: 'The role to invoke the lambda function is intended to be allowed to also invoke any of its layers',
appliesTo: [`Resource::${lambda.functionArn}:*`],
}],
true,
);
This question did previously come up on #957, but I wanted to propose a simpler syntax for use cases that a direct logicalId suppression would be cumbersome. An example of one of those more cumbersome cases is inside a construct that is used multiple times throughout the stack(s). I am aware that extracting the logicalId as that ticket does would also work, though is not as intuitive as building the string in the manner that would commonly be done for other uses cases within CDK.
Proposed Solution
A quick prototype version that worked for me was to add one more argument to NagSuppressionHelper.doesApply
named resolve
. resolve
is really just Stack.resolve
. It was then used like
if (typeof appliesTo === 'string') {
return flattenCfnReference(resolve(appliesTo)) === findingId;
}
else {
const regex = NagSuppressionHelper.toRegEx(appliesTo.regex);
return regex.test(findingId);
}
NagSuppresionHelper.doesApply
is only called in one location, NagPack.ignoreRule
, so I also updated that call site to pass in the equivalent of
const resourceStack = Stack.of(resource);
const resolve = resourceStack.resolve.bind(resourceStack);
Including the new imports, the prototype only had a 7 line diff.
Other information
There are two drawbacks I'm aware of to supporting this, although I'm not sure that either of them would be very important in practice.
The first is that the computational complexity of ignoring rules has increased deep inside the loops it is iterating through. Even on strings with no tokens there is still work being done to identify that to make both of those function calls return the input.
The second is that the metadata in the CFN templates are larger due to containing the token formatting syntax. In the example above it would contain something like
{
"Fn::Join": [
"",
[
"Resource::",
"Fn::GetAtt": ["ExampleFunctionLogicalId12345678", "Arn"],
":*"
]
]
}
I suspect that the second drawback could be mitigated by updating the code that inserts the metadata to use resolve
and use flattenCfnReference
. I did not attempt this because it seemed too involved for my little prototype to attempt a metadata rewrite. I was also concerned about erroneously flattening before a logical id was overridden, which of course just increases the complexity of trying to identify when it is safe to update the metadata into the flattened format.
Acknowledge
- I may be able to implement this feature request
- This feature might incur a breaking change
I like the idea! Some things to think about.
I was also concerned about erroneously flattening before a logical id was overridden
I think an implementation should look further into this. I think its best to use as little template space as possible, and this may not be a problem depending on what app lifecycle phase the overrides take place.