CNTRLPLANE-3237: Add new KMS foundational mechanism in encryption controller - #2161
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-2993 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced the shared KMS Unix socket with per-key sockets ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/uncc @dgrisonnet @tkashem |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 274-278: The test mock KMS plugin DaemonSet still binds a fixed
socket (kms.sock) which no longer matches the per-key socket naming produced by
KMSConfiguration (APIVersion/Name/Endpoint using defaultKMSEndpointPrefix and
keyID), so update the DaemonSet container command: remove the fixed "rm -f
/var/run/kmsplugin/kms.sock" cleanup and change the mock plugin invocation to
create/bind sockets using the per-key filename pattern (e.g., kms-<id>.sock)
that matches fmt.Sprintf("%s-%d.sock", defaultKMSEndpointPrefix, keyID) used
when constructing KMSConfiguration; ensure the mount/path and any socket
creation logic use that same pattern so tests use per-key sockets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93e09477-3cea-49cb-a98c-88e338798171
📒 Files selected for processing (9)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/state_controller_test.gopkg/operator/encryption/encryptionconfig/config_test.gopkg/operator/encryption/secrets/secrets.gopkg/operator/encryption/secrets/secrets_test.gopkg/operator/encryption/secrets/types.gopkg/operator/encryption/statemachine/transition_test.gopkg/operator/encryption/testing/helpers.go
9622e29 to
1a8f3a2
Compare
|
@ardaguclu: This pull request references CNTRLPLANE-2993 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 |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retitle CNTRLPLANE-3237: Add new KMS foundational mechanism in encryption controller |
openshift-ci
Bot
changed the title
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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. |
1a8f3a2 to
c1d3fda
Compare
| if err := json.Unmarshal(v, kmsConfiguration); err != nil { | ||
| return state.KeyState{}, fmt.Errorf("secret %s/%s has invalid %s data: %v", s.Namespace, s.Name, EncryptionSecretKMSEncryptionConfig, err) | ||
| } | ||
| key.KMSConfiguration = kmsConfiguration |
There was a problem hiding this comment.
should we rename key.KMSConfiguration to key.KMSEncryptionConfiguration / key.KMSEncryptionConfig ?
There was a problem hiding this comment.
I've updated to key.KMSEncryptionConfig
| EncryptionSecretKMSConfig = "encryption.apiserver.operator.openshift.io/kms-config" | ||
| // EncryptionSecretKMSEncryptionConfig is the data field key that stores the serialized KMS | ||
| // encryption configuration for KMS mode in the encryption-key secret. | ||
| EncryptionSecretKMSEncryptionConfig = "encryption.apiserver.operator.openshift.io-kms-encryption-config" |
There was a problem hiding this comment.
shouldn't this be kms-encryption-config ?
There was a problem hiding this comment.
All annotations and data fields in this area uses encryption.apiserver.operator.openshift. prefix, I think we should still have it to distinguish.
There was a problem hiding this comment.
We have a flexibility to just use kms-encrypyion-config as data key, though.
There was a problem hiding this comment.
if that is the convention then we should stick to it. thanks.
There was a problem hiding this comment.
why not encryption.apiserver.operator.openshift.io/kms-encryption-config ?
There was a problem hiding this comment.
I tried to use it and got an error. After the investigations, I found out the reason https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L7949-L7954 that secret data key can not contain / character.
| encryptionSecretKeyDataForTest = "encryption.apiserver.operator.openshift.io-key" | ||
| encryptionSecretMigratedTimestampForTest = "encryption.apiserver.operator.openshift.io/migrated-timestamp" | ||
| encryptionSecretMigratedResourcesForTest = "encryption.apiserver.operator.openshift.io/migrated-resources" | ||
| encryptionSecretKMSEncryptionConfigForTest = "encryption.apiserver.operator.openshift.io-kms-encryption-config" |
There was a problem hiding this comment.
shouldn't this be kms-encryption-config ?
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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. |
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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. |
|
/retest |
|
Manual testing proves that this PR properly works with plugin lifecycle changes openshift/cluster-kube-apiserver-operator#2035 (tested on openshift/cluster-kube-apiserver-operator#2103). In my opinion, if we prefer moving small incremental changes, we can merge this PR maybe |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@ardaguclu: This pull request references CNTRLPLANE-3237 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 "5.0.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. |
| subPath: softhsm-config.json | ||
| - name: softhsm-tokens | ||
| mountPath: /var/lib/softhsm/tokens | ||
| - name: kms-plugin-2 |
There was a problem hiding this comment.
could we move these changes to a new PR ?
also, could we just have a single manifest ?
could we extend the current template to accept additional param (keyID) ? and then we could use the keyID to generate name and the socket name.
then we would change
and we could extend (or write a new function)
to accept the number of replicas.There was a problem hiding this comment.
I've dropped test changes from this PR in favor of #2165
67e3cac to
ca7eb30
Compare
ca7eb30 to
d2417fd
Compare
|
/retest |
| } | ||
| key.KMSEncryptionConfig = kmsConfiguration | ||
| } | ||
| if key.KMSEncryptionConfig == nil { |
There was a problem hiding this comment.
can key.KMSEncryptionConfig be nil ? (given &apiserverconfigv1.KMSConfiguration{} ) ?
There was a problem hiding this comment.
I think, empty KMSEncryptionConfig will be forbidden in API side https://github.com/openshift/api/pull/2805/changes#diff-616d67895c3421c2d091662d30ed47b4b6f0b57db9411e29618d22b964ddb9efR212`
Since KMSEncryptionConfig is generated by key_controller in each new key, technically it is not possible to have nil KMSEncryptionConfig.
There was a problem hiding this comment.
Just for historical context: We have added this check to be defensive #2086 (comment)
There was a problem hiding this comment.
I mean it can be nil when there is no EncryptionSecretKMSEncryptionConfig right ?
There was a problem hiding this comment.
if v, ok := s.Data[EncryptionSecretKMSEncryptionConfig]; ok && len(v) > 0 {
...
} else {
return state.KeyState{}, fmt.Errorf("KMSEncryptionConfig can not be nil, when mode is KMS")
}
There was a problem hiding this comment.
I mean it can be nil when there is no EncryptionSecretKMSEncryptionConfig right ?
yes that is correct.
There was a problem hiding this comment.
Key controller always populates this field. But when this field is empty (due to manual deletion by modifying the key Secret, etc.), we will return error by checking key.KMSEncryptionConfig == nil
| EncryptionSecretKMSConfig = "encryption.apiserver.operator.openshift.io/kms-config" | ||
| // EncryptionSecretKMSEncryptionConfig is the data field key that stores the serialized KMS | ||
| // encryption configuration for KMS mode in the encryption-key secret. | ||
| EncryptionSecretKMSEncryptionConfig = "encryption.apiserver.operator.openshift.io-kms-encryption-config" |
There was a problem hiding this comment.
if that is the convention then we should stick to it. thanks.
| EncryptionSecretKMSConfig = "encryption.apiserver.operator.openshift.io/kms-config" | ||
| // EncryptionSecretKMSEncryptionConfig is the data field key that stores the serialized KMS | ||
| // encryption configuration for KMS mode in the encryption-key secret. | ||
| EncryptionSecretKMSEncryptionConfig = "encryption.apiserver.operator.openshift.io-kms-encryption-config" |
There was a problem hiding this comment.
why not encryption.apiserver.operator.openshift.io/kms-encryption-config ?
dea3114 to
4dfab23
Compare
4dfab23 to
216f898
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
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. |
Bumps library-go to include openshift/library-go#2161 (CNTRLPLANE-3237: Add new KMS foundational mechanism in encryption controller)
Bumps library-go to include openshift/library-go#2161 (CNTRLPLANE-3237: Add new KMS foundational mechanism in encryption controller)
What: Defines the cobra/pflag command interface for the kms-health-monitor sidecar. Why: I was told to split up the PR openshift#2161 Notes: Used for ref https://github.com/ardaguclu/library-go/blob/21d59d1efa2b372350d178aa0696ecdc8b3a6380/pkg/operator/encryption/kms/health/cmd.go
3 participants
This PR brings the minimal changes required for the KMS TPv2 work described here openshift/enhancements#1960.
Main aim is to store data in Data field of Secret instead of annotation, insert key id into unix domain socket path. This PR should work with plugin lifecycle smoothly.
Changes here break KMS TP v1 functionality, so it must not be merged in release-4.22 code base (so that this can be merged after the branch cut of 4.23).
This PR excludes;
Summary by CodeRabbit
Refactor
Tests