Skip to content

Conversation

JanStern
Copy link

Description

relates to #934

KMS has been added to the CLI. Now the following commands exist:

  • key (create, delete, import, list, restore, rotate)
  • key ring (create, delete, list)
  • version (destroy, disable, enable, list, restore)
  • wrapping key (create, destroy, list)

Checklist

  • Issue was linked above
  • Code format was applied: make fmt
  • Examples were added / adjusted (see e.g. here)
  • Docs are up-to-date: make generate-docs (will be checked by CI)
  • Unit tests got implemented or updated
  • Unit tests are passing: make test (will be checked by CI)
  • No linter issues: make lint (will be checked by CI)

Important Decisions

The CLI implementation of KMS reflects the state of the API, which includes some seemingly unfinished decisions.

  1. The Region doesn't matter even though it exists: Every API Endpoint has the region as a required attribute. However, the value is currently meaningless. All requests have the same result no matter what the region is. Still I haven't removed/mocked the structure in the code since I assume that in the future KMS will be region specific.
  2. Backend is a fixed but required value: When creating a wrapped key or a key, backend is a required body parameter that is currently only "software". Here following the same argument as before I have made it a flag in anticipation of future updates.
  3. I recommend extra detail to Importing a Key: Following this example I experimented with the key importing and tried to import an invalid key. I did encode random text in base64 and it was accepted. It created a new key version but with the status "Errors existing", which feels wrong. Nonetheless, I don't think that the formatting checks in the CLI should be stricter. (I just wanted to draw attention to that)

Hope this actually helps and huge thanks to whomever tries to tackle this monster merge.

@JanStern JanStern requested a review from a team as a code owner August 23, 2025 22:26
@JanStern
Copy link
Author

JanStern commented Sep 4, 2025

@rubenhoenle are there any updates on the PR? I would love to use it in my pipeline.


var (
testProjectId = uuid.NewString()
testRegion = "eu01"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
testRegion = "eu01"
const testRegion = "eu01"

@rubenhoenle
Copy link
Member

The Region doesn't matter even though it exists: Every API Endpoint has the region as a required attribute. However, the value is currently meaningless. All requests have the same result no matter what the region is. Still I haven't removed/mocked the structure in the code since I assume that in the future KMS will be region specific.

Well, the region is required for every API endpoint, it will just be "eu01" for now all the time. But please use the regular multi-region implementation like we do for all the other commands. So we're ready for the future :)

Backend is a fixed but required value: When creating a wrapped key or a key, backend is a required body parameter that is currently only "software". Here following the same argument as before I have made it a flag in anticipation of future updates.

Backend was deprecated in the v1beta version of the KMS service API already and got removed with the switch to the v1 version. So you'll have to remove it anyways, sorry 😄

@JanStern
Copy link
Author

JanStern commented Sep 5, 2025

Thank you for taking the time to review my code. I have implemented your suggestions to the best of my abilities.

However, I have not been able to switch from the v1beta to v1 since even at the latest version v0.5.1 of the kms go package the backend is still a mandatory field. Thus, for now this is the most accurate and working version of kms we can get for the cli until the go package will be updated.

Will this be a dealbreaker? Or can we move ahead with the merge and update kms later? (Isn't it in beta for a reason?)

@rubenhoenle
Copy link
Member

However, I have not been able to switch from the v1beta to v1 since even at the latest version v0.5.1 of the kms go package the backend is still a mandatory field. Thus, for now this is the most accurate and working version of kms we can get for the cli until the go package will be updated.

https://github.com/stackitcloud/stackit-sdk-go/blob/0c904a9a4c9ca0ff90c90b366cd0acba591a3292/services/kms/CHANGELOG.md#v100

The v1.0.0 should bring all the changes we need 😄

Will have to look through the rest of your PR another time, hopefully tomorrow, sorry.

Copy link

This PR was marked as stale after 7 days of inactivity and will be closed after another 7 days of further inactivity. If this PR should be kept open, just add a comment, remove the stale label or push new commits to it.

@github-actions github-actions bot added the Stale label Sep 19, 2025
@JanStern
Copy link
Author

@rubenhoenle let's not let this merge become stale. What do you think, can we get it to the needed state?

@github-actions github-actions bot removed the Stale label Sep 20, 2025
@JanStern
Copy link
Author

@rubenhoenle anything I can do for your?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants