Handle SecretsManager dynamic references #202
Conversation
CloudFormation has a capability that allows you to dynamically resolve secretsmanager values at deploy time. This PR adds special handling for these values by mapping them to the `getSecretVersion` function. I can think of two ways that these values would end up being referenced in a CDK application 1. The user provides a string value for the secretId and the reference is a complete string. 2. The user provides a reference value for the secretId and the reference is a Fn::Join intrinsic There may be other cases, but if there are I think they are not very common and we can handle them if users create issues. re #183, fixes #199
integration/secretsmanager/index.ts
Outdated
| if (props.HostedRotationLambda) { | ||
| throw new Error('Hosted Rotation is not supported'); | ||
| } | ||
| return new aws.secretsmanager.SecretRotation( |
There was a problem hiding this comment.
Why wouldn't we add this mapping directly to pu-cdk?
There was a problem hiding this comment.
ah I forgot about this. It actually should be available as a CCAPI type, but I was getting errors so I added this and forgot to go back and investigate. I'll update it.
src/converters/app-converter.ts
Outdated
| const secretInfo = secret[2]; | ||
| const arn = this.processIntrinsics(ref); | ||
| const info = parseDynamicSecretReferenceInfo(secretInfo); | ||
| return [ |
There was a problem hiding this comment.
We could combine that with the snippet from the func from above.
src/converters/app-converter.ts
Outdated
| throw new Error(`reference to unsupported pseudo parameter ${target}`); | ||
| return this.cdkStack.node.id; | ||
| // // Can't support these | ||
| // throw new Error(`reference to unsupported pseudo parameter ${target}`); |
There was a problem hiding this comment.
Why is this error commented out now?
There was a problem hiding this comment.
I forgot to remove it. I figure the stack node id is a good substitute
src/converters/app-converter.ts
Outdated
| * | ||
| * This is useful in cases where the full reference contains unresolved values, e.g. | ||
| * | ||
| * "MasterUserPassword": { |
There was a problem hiding this comment.
This is really really strange, the execution model here is strange. I have been looking up some references here. Sounds like https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/dynamic-references-secretsmanager.html is describing the feature, and AI gave me this example:
Resources:
MySecret:
Type: AWS::SecretsManager::Secret
Properties:
Name: MyDatabaseSecret
Description: "A secret for my database credentials"
SecretString: !Sub |
{
"username": "${DBUsername}",
"password": "${DBPassword}"
}
MyDatabase:
Type: AWS::RDS::DBInstance
Properties:
DBInstanceIdentifier: MyDBInstance
MasterUsername: !Join
- ""
- - '{{resolve:secretsmanager:'
- !Ref MySecret
- ':SecretString:username}}'
MasterUserPassword: !Join
- ""
- - '{{resolve:secretsmanager:'
- !Ref MySecret
- ':SecretString:password}}'
I would guess that CF takes this form and:
- first, resolves Ref references
- second, computes Fn::Join heuristic to get a string
- finally, searches the string for dynamic references and substitutes them
Is that right?
If we're parsing Fn::Join form this means we have a very different evaluation order that will work for Fn::Join but fail to compose the same way if other intrinsics are in play. So this is a very partial solution. Is getting to emulate the CF evaluation sequence not feasible or is something we can backlog?
There was a problem hiding this comment.
Ok, i've updated this to (I think) do what you are saying.
integration/examples_nodejs_test.go
Outdated
| test := getJSBaseOptions(t). | ||
| With(integration.ProgramTestOptions{ | ||
| Dir: filepath.Join(getCwd(t), "secretsmanager"), | ||
| }) |
There was a problem hiding this comment.
Has this been gofmt'd? We might be missing some liner setup in the repo.
There was a problem hiding this comment.
I think this may have happened when I manually resolved merge conflicts in the github ui, but you are correct that we don't have any checks on this in CI.
There was a problem hiding this comment.
This looks a lot better to me, thank you!
I am still a little unsure about:
processIntrinsics - what are the possible inputs and outputs?
processSecretsManagerRefrenceValue - what are the possible inputs and outputs?
Because the functions are very generic and type as any I am wondering what is expected inside that any. It looks like a generic mapping pass over and some typing could help track of this maybe?
E.g. we have a lot of this:
type V = boolean | string | V[] | {[index:string]: V};
But something is injecting outputs at some point and we probably deal with:
type V = boolean | string | V[] | {[index:string]: V} | pulumi.Output<V>;
Squinting at this quickly I did not find bugs yet but perhaps something to look at later on as the code matures.
I think we could definitely do better in the I'll create a backlog item to revisit this. |
CloudFormation has a capability that allows you to dynamically resolve secretsmanager values at deploy time.
This PR adds special handling for these values by mapping them to the
getSecretVersionfunction. I can think of two ways that these values would end up being referenced in a CDK applicationThere may be other cases, but if there are I think they are not very common and we can handle them if users create issues.
re #183, fixes #199