Open SamuelBucheliZ opened 1 year ago
This is the result of the CDK role not having the ability to update KMS keys like this. It's correct: the lambda that is being created probably doesn't have the right permission to update the key. It also probably shouldn't be using an imported key?
Further to what @SamuelBucheliZ mentioned, we also see issues/similar behaviour when "ICluster" ( created from "from_cluster_attributes") used for User creation( aws_redshift_alpha.User) instead of the "Cluster".
Describe the bug
When creating a user with the Redshift Alpha module, an encryption key can be provided (see https://docs.aws.amazon.com/cdk/api/v2/docs/aws-redshift-alpha-readme.html#creating-users and https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_aws-redshift-alpha.User.html ).
We have observed that the behavior differs depending on how the encryption key is created:
fromKeyArn
(see https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_kms.Key.html#static-fromwbrkeywbrarnscope-id-keyarn ), respectivelyfrom_key_arn
in Python, to import an externally defined key, the user creation fails due to missing permissions (see error message in "Current Behavior")Expected Behavior
We would expect the user creation to succeed in the case of using an externally defined key the same way as it does for a key created directly with the initializer / constructor. The type signature of the User initializer / constructor specifies
IKey
as the expected type, andfromKeyArn
/from_key_arn
provides an object of typeIKey
. Therefore, we would expect it to work for any implementation ofIKey
provided by CDK.It seems that CDK sets the correct permissions for the key created directly with the initializer / constructor, but does not do so for the imported key?
Current Behavior
It seems that for some reason the required permissions are not set on the KMS key. The creation of the test stack (see reproduction steps) fails with
Ideally, the CDK code should set the correct permissions on the key (which it seems to do in the case the key is not created from the ARN).
Reproduction Steps
The following test stack demonstrates the issue. Note that for the sake of having a minimal example here, we first create the
secret_encryption_key
and then use its ARN to create thekey_from_arn
. Of course, in this example we could directly use thesecret_encryption_key
(and then everything would work). However, in the real case this is based on, we only have the ARN to work with, i.e., the key will always be created using thefrom_key_arn
method.Possible Solution
Unknown. The creation of the user works if
secret_encryption_key
is used instead ofkey_from_arn
above. However, this is not possible in our setup (thesecret_encryption_key
is created elsewhere and we only have the ARN available). It is unclear whether there is a problem with the key construct itself or whether there is a problem with the way the key is handled in the Redshift Alpha user construct.Additional Information/Context
Overall, this is probably somewhat related to discussions around the Liskov substitution principle ( https://en.wikipedia.org/wiki/Liskov_substitution_principle ).
We have observed similar issues with interfaces in CDK previously. For example, a Redshift cluster created from
fromClusterAttributes
provides anICluster
, but cannot be used equivalently to an actualCluster
, even though most methods only specifyICluster
in their types.As far as we know, there may be some technical reasons for this (e.g., synth time vs. deploy time issues). However, this can make maintaining a larger CDK code base somewhat cumbersome, as it makes relying on typical software engineering best practices (e.g., the usage of interfaces) somewhat tricky. Therefore, in cases where this is not avoidable, it may be desirable adjust the types used in the various methods (e.g., if only a specific implementation works, the method should only accept this specific implementation) or to at least further clarify this in the documentation.
CDK CLI Version
2.95.0 (build cfa7e88)
Framework Version
2.96.2
Node.js Version
v18.17.1
OS
various (locally tested on Windows and MacOS, in CI/CD pipeline with LInux images)
Language
Python
Language Version
3.10.13
Other information
No response