API-1844: KMS Encryption Provider

[swghosh]

Add support for KMS to existing encryption controller(s)

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@swghosh: This pull request references API-1844 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

Add support for KMS to existing encryption controller(s)

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: swghosh
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[swghosh]

/cc @dgrisonnet @tkashem

[dgrisonnet]

Some high level design comments.

I am mostly wandering if instead of continuing in the route of generate key secret like we do for the other encryption providers, we could instead rely on a KMS config secret/CM that the existing state machines would look for similarly to what they are already doing with key secrets

[dgrisonnet]

I was looking at your change to the key controller and the more I am thinking about it the more I think we should handle local keys and kms keys differently. A lot of the existing key controller logic is about generating the key and backing it. Whereas for KMS we want to generating a plugin config and backing it up.

Because of that, I think it would be more appropriate to create a new controller for KMS. wdyt?

[dgrisonnet]

Actually, there is one particular logic for which we will need the keyController and a new key state which is when the key is rotated on the external KMS.

When that happens we the KMS plugin will returned a new key_id in the responses: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3299-kms-v2-improvements#key_id-and-rotation.
In our case to handle that scenario we want the keyController to probe the Status procedure call and create a new key secret when the key_id changes to trigger a storage migration.

I recall discussing that with you in the past and we agreed that this could be done at a later point, but looking at your PR, was the intent behind your key controller changes to act as a placeholder to include that logic later?

[swghosh]

I was looking at your change to the key controller and the more I am thinking about it the more I think we should handle local keys and kms keys differently. A lot of the existing key controller logic is about generating the key and backing it. Whereas for KMS we want to generating a plugin config and backing it up.

Reasonably, we can follow-up with this when we add a new controller to manage the lifecycle of kms-plugin itself. Because then we'd need to update the status somewhere to list active KMS config hashes for the plugin controller to spin up unix sockets.

[swghosh]

we could instead rely on a KMS config secret/CM that the existing state machines would look for similarly to what they are already doing with key secrets

we're already doing that: we keep track of the KMSConfig set in the apiserver.config.openshift.io/cluster resource and persist the configs in a backed secret, which the state machine uses to read info from.

The related details/implementation would be in pkg/operator/encryption/secrets/secrets.go where the kmsConfig is being marshalled/unmarshalled from the secret before being read by the state machine in pkg/operator/encryption/controllers/{key_controller.go,state_controller.go}.

[swghosh]

Previous failure was CI infra related, when cluster failed to come up.

/retest

[openshift-ci]

@swghosh: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.