CNTRLPLANE-2120: Add KMS foundations in encryption controllers in library-go #1900
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-2120 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 story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
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. |
ardaguclu
force-pushed
the
kms-encryption-controllers
branch
from
c091cbc to
f734b05
Compare
flavianmissi
left a comment
flavianmissi
left a comment
There was a problem hiding this comment.
Have to take a short break from reviewing, but leaving the comments I got so far.
|
/cc @ibihim @flavianmissi |
ardaguclu
force-pushed
the
kms-encryption-controllers
branch
2 times, most recently
from
1794054 to
1ddc3d8
Compare
|
@flavianmissi I was uncomfortable about the disconnects between the sections and the verbosity. So I overhauled the EP to have better clarity. Please let me know your thoughts. |
ardaguclu
force-pushed
the
kms-encryption-controllers
branch
3 times, most recently
from
e920a9c to
5804b76
Compare
ardaguclu
force-pushed
the
kms-encryption-controllers
branch
from
f39a0d7 to
8f79ed6
Compare
|
/lgtm |
|
/cc @benluddy |
|
As we agreed with @flavianmissi, in next iterations there will be another condition to notify users to delete unused kms plugins from cluster, when prune_controller prunes them. |
ibihim
left a comment
ibihim
left a comment
There was a problem hiding this comment.
Great work. I have some questions though as a beginner to downstream e2ee with kms
| 4. Reuse existing migration controller (no changes needed) | ||
|
|
||
| **Tech Preview v2 additions:** | ||
| - Poll KMS plugin Status endpoint for `key_id` changes in apiserver operators |
There was a problem hiding this comment.
How do we call the KMS plugin Status endpoint?
It gets exposed by the kube-apiserver endpoint, right?
There was a problem hiding this comment.
That is a good question. We left it TBD for now. Accessing kms plugins require privileged container. API servers (kube-apiserver, oauth-apiserver, openshift-apiserver) are privileged. So they can access. However operators (cluster-kube-apiserver-operator, openshift-apiserver-operator, cluster-authentication-operator) are not privileged. We'll have to find a way for operators.
There was a problem hiding this comment.
We've discussed this a little more, and yes the kube-apiserver does export healthcheck endpoints specific to the kms plugin(s) configured via EncryptionConfig, so that's definitely an option.
There was a problem hiding this comment.
I didn't know that and this is great finding. If that is the case, this resolves one of the important pain points :).
There was a problem hiding this comment.
According to my research today, this endpoint only exposes health status of kms plugin, whether it is ok or not. We still need to get key_id to detect key is rotated or not.
|
I'll update this EP base on the changes in openshift/api#2622 |
openshift-ci
bot
added
the
do-not-merge/hold
|
|
||
| **KMS** is the external Key Management Service (AWS KMS, HashiCorp Vault, etc.) that stores and manages the Key Encryption Key (KEK). | ||
|
|
||
| **KMS plugin** is a gRPC service implementing Kubernetes KMS v2 API, running as a sidecar to API server pods. It communicates with the external KMS to encrypt/decrypt data encryption keys (DEKs). |
There was a problem hiding this comment.
We don't imagine plugins will run as true sidecar containers anymore, correct?
There was a problem hiding this comment.
That is correct. I've updated the EP based on the last discussions. Please let me know your thoughts about this.
|
|
||
| #### Variation: Configuration Changes (Key Rotation) | ||
|
|
||
| When cluster admin updates KMS configuration (e.g., new key ARN, different region): |
There was a problem hiding this comment.
Isn't key_id -- or a hash of key_id -- sufficient by itself to consider previously-encrypted resources "stale"? I would have expected any significant change to a KMS provider config to result in a new key_id (and insignificant changes would not).
There was a problem hiding this comment.
Isn't key_id -- or a hash of key_id -- sufficient by itself to consider previously-encrypted resources "stale"?
There are 2 dimensions. First one is to detect key rotation in the same KMS Plugin (we'll track key_id to detect this). Another one is to migrate to new KMS Plugin from old one (we track unix domain socket path to detect this).
| 2. Compares new hash with hash in most recent encryption key secret annotation. | ||
| 3. If hashes differ: | ||
| - Creates new encryption key secret with new hash | ||
| - migrationController automatically triggers re-encryption |
There was a problem hiding this comment.
Do we have any mechanism to provide backpressure in a situation where a KMS provider is rotating key_id very quickly?
There was a problem hiding this comment.
That is a good point. I don't think we have any, since this is entirely managed externally. But in the future maybe we can provide a new degraded condition by detecting the frequency (I'm not sure how). cc: @flavianmissi
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ardaguclu: This pull request references CNTRLPLANE-2120 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 story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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. |
|
/hold cancel /cc @flavianmissi @ibihim @benluddy could you PTAL?. Thank you |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@ardaguclu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Plugin lifecycle management decision may have an impact on this EP. So I'm adding hold until the decision is made |
openshift-ci
bot
added
the
do-not-merge/hold
5 participants
This PR is based on #1872 (changes in
enhancements/kube-apiserver/kms-encryption-foundations.md).There are many aspects that need to be implemented to support KMS in OpenShift. We have decided to open more granular EPs to better track the work.
This EPs main aim is to focus on the encryption controller changes in library-go. This EP defers some concepts to future in order to start with simpler, manageable iterations.