search

crossplane-contrib / provider-upjet-aws

Official AWS Provider for Crossplane by Upbound.
https://marketplace.upbound.io/providers/upbound/provider-aws
Apache License 2.0
139 stars 118 forks source link

RDS Cluster needs KMS ARN instead of KMS Key ID #892

Open cristicalin opened 10 months ago

cristicalin commented 10 months ago

What happened?

In a composition which contains a KMS key and an RDS cluster I expected to be able to use the kmsKeyIdSelector key of the RDS cluster to properly match the KMS key in the composition. Unfortunately the query used seems to fetch the KMS Key ID instead of the ARN. I suspect the issue si somewhere around https://github.com/upbound/provider-aws/blob/main/apis/rds/v1beta1/zz_cluster_types.go#L441 but I lack sufficient understanding of how the provider code works at the moment to provide a solution.

How can we reproduce it?

  1. Create a composition with a KMS key and and RDS cluster
  2. In the RDS cluster set the following for spec.forProvider:
kmsKeyIdSelector:
  matchControllerRef: true

Instantiate a claim for that composition and see this error in the cluster status:

observe failed: cannot run refresh: refresh failed: "kms_key_id" (mrk-4847128a609f4ba981df90ecfff90907) is an invalid ARN: arn: invalid prefix:

Complete resource example:

    - name: kms-key
      base:
        apiVersion: kms.aws.upbound.io/v1beta1
        kind: Key
        spec:
          forProvider:
            deletionWindowInDays: 7
            multiRegion: true
          providerConfigRef:
            name: provider-aws-upbound-kms
      patches:
        - type: PatchSet
          patchSetName: location
        - type: FromCompositeFieldPath
          fromFieldPath: spec.claimRef.name
          toFieldPath: metadata.annotations[crossplane.io/external-name]
          transforms:
            - type: string
              string:
                type: Format
                fmt: "%s-key"
      base:
        apiVersion: rds.aws.upbound.io/v1beta1
        kind: Cluster
        spec:
          forProvider:
            allowMajorVersionUpgrade: true
            applyImmediately: true
            kmsKeyIdSelector:
              matchControllerRef: true
            storageEncrypted: true
            masterUsername: root
            manageMasterUserPassword: true
            masterUserSecretKmsKeyIdSelector:
              matchControllerRef: true
            skipFinalSnapshot: true
            dbSubnetGroupNameSelector:
              matchControllerRef: true
            vpcSecurityGroupIds:
              - sg-0a6dee57df5ea8255
          providerConfigRef:
            name: provider-aws-upbound-rds
      patches:
        - type: PatchSet
          patchSetName: location
        - type: PatchSet
          patchSetName: engine
        - type: PatchSet
          patchSetName: engineVersion
        - type: FromCompositeFieldPath
          fromFieldPath: spec.claimRef.name
          toFieldPath: metadata.annotations[crossplane.io/external-name]
        - type: FromCompositeFieldPath
          fromFieldPath: spec.claimRef.name
          toFieldPath: spec.forProvider.dbClusterParameterGroupName
          transforms:
            - type: string
              string:
                type: Format
                fmt: "%s-cluster-parameter-group"
        - type: FromCompositeFieldPath
          fromFieldPath: spec.claimRef.name
          toFieldPath: spec.forProvider.dbInstanceParameterGroupName
          transforms:
            - type: string
              string:
                type: Format
                fmt: "%s-db-parameter-group"

What environment did it happen in?

mbbush commented 10 months ago

To switch from kms ids to ARNs, the change will be in https://github.com/upbound/provider-aws/blob/main/config/rds/config.go, updating the other kms key references to use the ARN extractor like on line 103, instead of the default, which probably just gets the key id, then running make generate.

Honestly we should probably do this for all kms key references. If the key is in the same account, generally the id works just as well as the ARN. But if you're referencing a key in a different account using just the id, AWS tries to find a key with that id in your account and fails.

If nobody else beats me to it, I could probably do this next week. But I'm also happy for someone else to take it.

But I don't think that's actually the issue here. I'm suspicious of your patch to the external name annotation. I think that might be somehow setting the id of the key to something other than what AWS uses. mrk-1234567890abcedf1234567890abcdef is not how I remember kms key ids being formatted.

cristicalin commented 10 months ago

@mbbush I tried to remove the external name but I have the same issue. Here is a kubectl describe of the key and the cluster:

key

Name:         cristi-test-89j2v-gf2xh
Namespace:
Labels:       crossplane.io/claim-name=cristi-test
              crossplane.io/claim-namespace=sandbox
              crossplane.io/composite=cristi-test-89j2v
Annotations:  crossplane.io/composition-resource-name: kms-key
              crossplane.io/external-create-succeeded: 2023-10-02T11:25:48Z
              crossplane.io/external-name: mrk-b65b8bb7943e4b92a1b5c5be4f654cf6
              upjet.crossplane.io/provider-meta: null
API Version:  kms.aws.upbound.io/v1beta1
Kind:         Key
Metadata:
  Creation Timestamp:  2023-10-02T11:25:45Z
  Finalizers:
    finalizer.managedresource.crossplane.io
  Generate Name:  cristi-test-89j2v-
  Generation:     3
  Owner References:
    API Version:           fm-tech.net/v1alpha1
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  XDBCluster
    Name:                  cristi-test-89j2v
    UID:                   8d703fdb-93be-4559-af4e-47f58186edee
  Resource Version:        176766382
  UID:                     a59289c4-4987-4d77-b6ad-9bdb14bbd1d5
Spec:
  Deletion Policy:  Delete
  For Provider:
    Customer Master Key Spec:  SYMMETRIC_DEFAULT
    Deletion Window In Days:   7
    Is Enabled:                true
    Key Usage:                 ENCRYPT_DECRYPT
    Multi Region:              true
    Policy:                    {"Id":"key-default-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::027159582536:root"},"Resource":"*","Sid":"Enable IAM User Permissions"}],"Version":"2012-10-17"}
    Region:                    eu-west-1
    Tags:
      Crossplane - Kind:            key.kms.aws.upbound.io
      Crossplane - Name:            cristi-test-89j2v-gf2xh
      Crossplane - Providerconfig:  provider-aws-upbound-kms
  Init Provider:
  Management Policies:
    *
  Provider Config Ref:
    Name:  provider-aws-upbound-kms
Status:
  At Provider:
    Arn:                                 arn:aws:kms:eu-west-1:027159582536:key/mrk-b65b8bb7943e4b92a1b5c5be4f654cf6
    Bypass Policy Lockout Safety Check:  false
    Custom Key Store Id:
    Customer Master Key Spec:            SYMMETRIC_DEFAULT
    Deletion Window In Days:             7
    Description:
    Enable Key Rotation:                 false
    Id:                                  mrk-b65b8bb7943e4b92a1b5c5be4f654cf6
    Is Enabled:                          true
    Key Id:                              mrk-b65b8bb7943e4b92a1b5c5be4f654cf6
    Key Usage:                           ENCRYPT_DECRYPT
    Multi Region:                        true
    Policy:                              {"Id":"key-default-1","Statement":[{"Action":"kms:*","Effect":"Allow","Principal":{"AWS":"arn:aws:iam::027159582536:root"},"Resource":"*","Sid":"Enable IAM User Permissions"}],"Version":"2012-10-17"}
    Tags:
      Crossplane - Kind:            key.kms.aws.upbound.io
      Crossplane - Name:            cristi-test-89j2v-gf2xh
      Crossplane - Providerconfig:  provider-aws-upbound-kms
    Tags All:
      Crossplane - Kind:            key.kms.aws.upbound.io
      Crossplane - Name:            cristi-test-89j2v-gf2xh
      Crossplane - Providerconfig:  provider-aws-upbound-kms
  Conditions:
    Last Transition Time:  2023-10-02T11:26:10Z
    Reason:                Available
    Status:                True
    Type:                  Ready
    Last Transition Time:  2023-10-02T11:25:48Z
    Reason:                ReconcileSuccess
    Status:                True
    Type:                  Synced
    Last Transition Time:  2023-10-02T11:25:54Z
    Reason:                Success
    Status:                True
    Type:                  LastAsyncOperation
    Last Transition Time:  2023-10-02T11:25:54Z
    Reason:                Finished
    Status:                True
    Type:                  AsyncOperation
Events:
  Type     Reason                           Age    From                                          Message
  ----     ------                           ----   ----                                          -------
  Normal   CreatedExternalResource          4m44s  managed/kms.aws.upbound.io/v1beta1, kind=key  Successfully requested creation of external resource
  Warning  CannotInitializeManagedResource  4m44s  managed/kms.aws.upbound.io/v1beta1, kind=key  Operation cannot be fulfilled on keys.kms.aws.upbound.io "cristi-test-89j2v-gf2xh": the object has been modified; please apply your changes to the latest version and try again
  Warning  CannotUpdateManagedResource      4m36s  managed/kms.aws.upbound.io/v1beta1, kind=key  Operation cannot be fulfilled on keys.kms.aws.upbound.io "cristi-test-89j2v-gf2xh": the object has been modified; please apply your changes to the latest version and try again

cluster

Name:         cristi-test-89j2v-hcsh9
Namespace:
Labels:       crossplane.io/claim-name=cristi-test
              crossplane.io/claim-namespace=sandbox
              crossplane.io/composite=cristi-test-89j2v
Annotations:  crossplane.io/composition-resource-name: db-cluster
              crossplane.io/external-name: cristi-test
API Version:  rds.aws.upbound.io/v1beta1
Kind:         Cluster
Metadata:
  Creation Timestamp:  2023-10-02T11:25:45Z
  Generate Name:       cristi-test-89j2v-
  Generation:          3
  Owner References:
    API Version:           fm-tech.net/v1alpha1
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  XDBCluster
    Name:                  cristi-test-89j2v
    UID:                   8d703fdb-93be-4559-af4e-47f58186edee
  Resource Version:        176766564
  UID:                     6dfbc135-9c66-4b72-abd2-791b17cac23d
Spec:
  Deletion Policy:  Delete
  For Provider:
    Allow Major Version Upgrade:       true
    Apply Immediately:                 true
    Db Cluster Parameter Group Name:   cristi-test-cluster-parameter-group
    Db Instance Parameter Group Name:  cristi-test-db-parameter-group
    Db Subnet Group Name:              cristi-test-subnet-group
    Db Subnet Group Name Ref:
      Name:  cristi-test-89j2v-f4cnm
    Db Subnet Group Name Selector:
      Match Controller Ref:  true
    Engine:                  aurora-postgresql
    Engine Version:          15.2
    Kms Key Id:              mrk-b65b8bb7943e4b92a1b5c5be4f654cf6
    Kms Key Id Ref:
      Name:  cristi-test-89j2v-gf2xh
    Kms Key Id Selector:
      Match Controller Ref:  true
    Master Password Secret Ref:
      Key:                password
      Name:               master-password
      Namespace:          sandbox
    Master Username:      root
    Region:               eu-west-1
    Skip Final Snapshot:  true
    Storage Type:
    Tags:
      Crossplane - Kind:            cluster.rds.aws.upbound.io
      Crossplane - Name:            cristi-test-89j2v-hcsh9
      Crossplane - Providerconfig:  provider-aws-upbound-rds
    Vpc Security Group Ids:
      sg-0a6dee57df5ea8255
  Init Provider:
  Management Policies:
    *
  Provider Config Ref:
    Name:  provider-aws-upbound-rds
  Write Connection Secret To Ref:
    Name:       cristi-test-connection-secret
    Namespace:  sandbox
Status:
  At Provider:
  Conditions:
    Last Transition Time:  2023-10-02T11:26:15Z
    Message:               observe failed: cannot run refresh: refresh failed: "kms_key_id" (mrk-b65b8bb7943e4b92a1b5c5be4f654cf6) is an invalid ARN: arn: invalid prefix:
    Reason:                ReconcileError
    Status:                False
    Type:                  Synced
Events:
  Type     Reason                           Age                   From                                              Message
  ----     ------                           ----                  ----                                              -------
  Warning  CannotInitializeManagedResource  5m15s                 managed/rds.aws.upbound.io/v1beta1, kind=cluster  Operation cannot be fulfilled on clusters.rds.aws.upbound.io "cristi-test-89j2v-hcsh9": the object has been modified; please apply your changes to the latest version and try again
  Warning  CannotResolveResourceReferences  5m2s (x4 over 5m15s)  managed/rds.aws.upbound.io/v1beta1, kind=cluster  cannot resolve references: mg.Spec.ForProvider.KMSKeyID: referenced field was empty (referenced resource may not yet be ready)
  Warning  CannotObserveExternalResource    7s (x7 over 4m45s)    managed/rds.aws.upbound.io/v1beta1, kind=cluster  cannot run refresh: refresh failed: "kms_key_id" (mrk-b65b8bb7943e4b92a1b5c5be4f654cf6) is an invalid ARN: arn: invalid prefix:
mbbush commented 10 months ago

oh, the mrk-... id format is for multi region keys.

This is just a bug. The terraform provider docs for aws_rds_cluster.kms_key_id specify that it should be the ARN (despite being named _id grumble grumble).

I'll see if I can open a PR to fix this.

mbbush commented 10 months ago

@cristicalin Can you please validate that if you edit the Cluster.rds managed resource to remove the kmsKeyIdSelector and kmsKeyRef, and specify the output of status.atProvider.arn from the Key.kms managed resource in spec.forProvider.kmsKeyId, the db cluster becomes ready and works properly?

Assuming that works, as a workaround you can add two patches to your composition, a ToCompositeFieldPath patch from the status.atProvider.arn of the key to some new field in the status of your composite, then a FromCompositeFieldPath patch with 

policy:
  fromFieldPath: Required

to set the spec.forProvider.kmsKeyId. This will ensure the database cluster managed resource is not created until the kms key is ready and has patched its ARN into the composite.

cristicalin commented 10 months ago

@mbbush I already tested the workaround after @haarchri recommended it on slack https://crossplane.slack.com/archives/C01718T2476/p1695650328337859 and it works but that doesn't look like the proper solution to this problem

mbbush commented 10 months ago

that doesn't look like the proper solution to this problem

Agreed. I'm glad you're unblocked for now. I'm working on a proper fix.

This looks like a much bigger problem than just RDS: there are dozens of resources we reference this way, using a terraform field named "kms_key_id", most of whose terraform docs say they actually need a key ARN, but the crossplane heuristic code that adds the KNS reference makes it by the key id, not the arn.

"Always use the ARN for KMS key references" will be easy. The tricky part will be figuring out whether that's a breaking change to some of the existing resources, and if so how to handle it.

github-actions[bot] commented 4 months ago

This provider repo does not have enough maintainers to address every issue. Since there has been no activity in the last 90 days it is now marked as stale. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

github-actions[bot] commented 1 week ago

This provider repo does not have enough maintainers to address every issue. Since there has been no activity in the last 90 days it is now marked as stale. It will be closed in 14 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.