DynamoDB controller doesn't work with KMS aliases, requires ARN due to how AWS API returns ARN

[jbilliau-rcd]

Describe the bug
Creating a Table object like below:

apiVersion: dynamodb.services.k8s.aws/v1alpha1
kind: Table
metadata:
  name: dev-302040-test-whale-orange
  labels:
    helm-chart-release: aws-resources-0.0.1
    release: helmatest-ack
    app-id: "302040"
    app: dev-test-whale
    environment: dev
    deploy-date: "2024-08-15"
  annotations:
    services.k8s.aws/deletion-policy: delete
spec:
  tableName: dev-302040-test-whale-orange
  attributeDefinitions:
    - attributeName: Bill
      attributeType: S
  keySchema:
    - attributeName: Bill
      keyType: HASH
  sseSpecification:
    enabled: true
    sseType: KMS
    kmsMasterKeyID: alias/dev-302040-test-whale-orange

results in the following error endlessly, never resolving:

conditions:
- message: >-
cannot update table LimitExceededException: Subscriber limit exceeded:
Encryption mode changes are limited in the 24h window ending at
2024-08-16T13:54:50.348Z. After the first 4 change, each subsequent
change in the same window can be performed at most once every 21600
seconds. Number of updates today: 4. Last change at
2024-08-15T13:56:28.094Z. Next changes can be made at
2024-08-15T19:56:28.094Z.
status: 'True'
type: ACK.Recoverable

This appears to be the same issue that plagued crossplane, due to how the AWS API always returns an ARN on create or update calls. crossplane-contrib/provider-aws#1907

I suspect ACK needs the same fix.

Steps to reproduce

Create a Table object using a CMK KMS like below:

sseSpecification:
    enabled: true
    sseType: KMS
    kmsMasterKeyID: alias/dev-302040-test-whale-orange

But if you switch kmsMasterKeyID to the ARN of the key, it instantly resolves and fixes itself.

Expected outcome
Table should support KMS Aliases like a lot of the other ACK resources do. Otherwise, you can't create the KMS key and use it to encrypt objects in the same Helm chart/kubectl apply due to the ARN not being known yet.

Environment

Any.

• Kubernetes version: 1.28
• Using EKS (yes/no), if so version? Yes, 1.28
• AWS service targeted (S3, RDS, etc.) Dynamodb

[michaelhtm]

Hey @jbilliau-rcd, I attempted this, and you are right, that issue persists. It seems like the DynamoDB api accepts KMS alias as input, but when you call describe it returns the ARN. I was not able to retrieve the KMS alias from the ARN...In ACK we need to do a Read operation to see if there are any diffs and check whether we need to call an update or not, which is what is happening in your case.

kro is solving this problem tho, where you can use CEL to retrieve the kms ARN and use it on your table!
I hope that helps

[jbilliau-rcd]

hey @michaelhtm , thanks for responding. Is this something that will be fixed in the dynamodb controller itself? We already have ACK resources wrapped up in our own helm chart, didn't really want to use vro to get around an issue that would preferably be fixed in the controller itself.

[michaelhtm]

Hey @jbilliau-rcd, I have a PR that allows you to use references for the KMSKeyID, this is a Gitops friendly approach as you just need to specify the key resource name in your table CR manifest. Here's an example:

apiVersion: kms.services.k8s.aws/v1alpha1
kind: Key
metadata:
  name: myk
spec:
  description: "Key created by ACK tests"

---
# Table used to test multiple interfering updates at once
apiVersion: dynamodb.services.k8s.aws/v1alpha1
kind: Table
metadata:
  name: mytable
spec:
  tableName: mytable
  billingMode: PAY_PER_REQUEST
  tableClass: STANDARD
  attributeDefinitions:
    - attributeName: Bill
      attributeType: S
    - attributeName: Total
      attributeType: S
  keySchema:
    - attributeName: Bill
      keyType: HASH
    - attributeName: Total
      keyType: RANGE
  sseSpecification:
    enabled: true
    sseType: KMS
    kmsMasterKeyRef:
      from:
        name: myk

[michaelhtm]

Hey @jbilliau-rcd, we just released the dynamoDB controller with the fix from above. Let us know if there are any issues with it

[michaelhtm]

Closing this issue for now, please feel free to reopen if you have any more questions. Thanks!
/close

[ack-prow]

@michaelhtm: Closing this issue.

In response to this:

Closing this issue for now, please feel free to reopen if you have any more questions. Thanks!
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jbilliau-rcd]

Awesome, I'll get around to testing/implementing this shortly. Thanks @michaelhtm !

[jbilliau-rcd]

Hey @michaelhtm , I just tested this, isn't working for me. I passed this into my Table object:

sseSpecification:
    enabled: true
    sseType: KMS
    kmsMasterKeyRef:
      from:
        name: test-214003-whale-key

but when I check the yaml of the deployed object, the kmsMasterKeyRef field is completely missing. If I do a kubectl get crd tables.dynamodb.services.k8s.aws -o yaml | grep kmsMasterKeyRef, I get nothing; it seems like the CRD wasn't updated?

I am running the latest release, is there something else I need to do other than just upgrade to the latest helm chart? Oddly enough I do see it here, not sure why it's not applying to my cluster when I update my chart release.

[jbilliau-rcd]

Hmmm ok more interesting information; I do a helm get manifest ack-dynamodb-controller -n ack-system and the CRD isn't in the returned manifest. I don't ever recall, nor have any record, of my applying them manually. So if the chart doesn't deploy them, does the controller? if so, it doesn't seem to be updating them like it should.

[michaelhtm]

Hey @jbilliau-rcd , i recently found out helm does not update your crd, so you'll have to update them manually. It only installs them for you if you've never installed them before.

[jbilliau-rcd]

That's....inconvenient. :(

[michaelhtm]

Agree