CNTRLPLANE-3237: key controller schedules a preflight check - #2392
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
WalkthroughEncryption controller construction now injects KMS status and preflight dependencies. KMS key creation computes referenced-resource hashes, waits for matching successful preflight status, and avoids persistence while pending or failed. Tests cover the new gate and integration setup. ChangesKMS preflight integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant keyController
participant SecretsAndConfigMaps
participant encryptionStatusProvider
participant Kubernetes API
keyController->>SecretsAndConfigMaps: Fetch referenced Secret and ConfigMap
keyController->>encryptionStatusProvider: Read encryption status
keyController->>encryptionStatusProvider: Update observed configuration hash
encryptionStatusProvider-->>keyController: Return pending, failure, or success
keyController->>Kubernetes API: Persist key after successful preflight
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![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
🧹 Nitpick comments (6)
pkg/operator/encryption/controllers/kms_preflight_controller.go (1)
53-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStruct field alignment is not gofmt-clean, and the field name stutters.
gofmt aligns struct field names/types, so
provider kmsProviderConfignext to the longer field will be reformatted. Also consider naming the fieldresources(the type already carries the full name).♻️ Proposed fix
type kmsConfigHasher struct { - provider kmsProviderConfig - kmsConfigHasherResourceProvider kmsConfigHasherResourceProvider + provider kmsProviderConfig + resources kmsConfigHasherResourceProvider // namespace is the namespace where the referenced Secrets and ConfigMaps are stored (e.g., openshift-config). namespace string } // newKMSConfigHasher creates a hasher for a KMS provider config and its referenced resources. // namespace is the namespace where the referenced Secrets and ConfigMaps are stored (e.g., openshift-config). -func newKMSConfigHasher(provider kmsProviderConfig, kmsConfigHasherResourceProvider kmsConfigHasherResourceProvider, namespace string) *kmsConfigHasher { - return &kmsConfigHasher{provider: provider, kmsConfigHasherResourceProvider: kmsConfigHasherResourceProvider, namespace: namespace} +func newKMSConfigHasher(provider kmsProviderConfig, resources kmsConfigHasherResourceProvider, namespace string) *kmsConfigHasher { + return &kmsConfigHasher{provider: provider, resources: resources, namespace: namespace} }Update the two call sites at lines 95 and 128 accordingly (
h.resources.getSecret(...)/h.resources.getConfigMap(...)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/kms_preflight_controller.go` around lines 53 - 63, Rename the kmsConfigHasher field kmsConfigHasherResourceProvider to resources and update its constructor assignment plus both call sites in the hasher methods to use h.resources. Run gofmt so the struct field declarations are properly aligned.pkg/operator/encryption/controllers/key_controller_test.go (2)
550-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComposite-literal keys here are misaligned for gofmt.
expectedActions:retains the old two-space padding while the neighbouring keys were widened, so gofmt will rewrite this block.♻️ Proposed fix
apiServerObjects: []runtime.Object{apiServerWithKMS}, targetNamespace: "kms", encryptionStatusProvider: kmsCreateKeyStatusProvider, - expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "get:configmaps:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"}, + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "get:configmaps:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 550 - 553, Align the expectedActions composite-literal key with the surrounding fields in the test case containing apiServerObjects, targetNamespace, and encryptionStatusProvider, so the struct literal is gofmt-compliant without changing its value.
1062-1211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a subtest for the preflight-pending-with-stale-result case.
TestKMSPreflightGatecovers hash-write/backoff, pending, succeeded, and failed. One uncovered branch ofensureKMSPreflightPassed:ObservedConfigHashmatches butResult.ConfigHashbelongs to an older config (stale Succeeded/Failed) — should back off, not create a key or error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/key_controller_test.go` around lines 1062 - 1211, Add a subtest to TestKMSPreflightGate covering a matching ObservedConfigHash with a stale Result.ConfigHash from an older configuration, using both succeeded and failed result statuses if practical. Assert Sync backs off without creating a key or returning the preflight failure error, and without updating status; reuse computeExpectedHash, buildController, and actionsWithoutKey.pkg/operator/encryption/controllers/key_controller.go (2)
426-438: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the prefetched objects against nil before dereferencing.
p.secret/p.configMapare nil whenever the provider config has no referenced Secret/ConfigMap. Today the hasher short-circuits on an empty name so these are unreachable, but the coupling is implicit — a future change inreferencedSecretName/referencedConfigMapNamehandling turns this into a panic.🛡️ Proposed fix
func (p *prefetchedKMSConfigHasherResourceProvider) getSecret(_ context.Context, namespace, name string) (*corev1.Secret, error) { + if p.secret == nil { + return nil, fmt.Errorf("no prefetched secret available for requested %s/%s", namespace, name) + } if p.secret.Namespace != namespace || p.secret.Name != name { return nil, fmt.Errorf("prefetched secret %s/%s does not match requested %s/%s", p.secret.Namespace, p.secret.Name, namespace, name) } return p.secret, nil } func (p *prefetchedKMSConfigHasherResourceProvider) getConfigMap(_ context.Context, namespace, name string) (*corev1.ConfigMap, error) { + if p.configMap == nil { + return nil, fmt.Errorf("no prefetched configmap available for requested %s/%s", namespace, name) + } if p.configMap.Namespace != namespace || p.configMap.Name != name {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/key_controller.go` around lines 426 - 438, Update getSecret and getConfigMap to check p.secret and p.configMap for nil before accessing Namespace or Name; return the existing mismatch error (or an equivalent safe error) when the prefetched object is absent, while preserving the current matching-object return behavior.
245-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRequeue interval is a magic value.
30*time.Secondfor the preflight backoff would read better as a named constant next to the existing2*time.Minuteprogressing backoff, and makes it tunable in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/key_controller.go` around lines 245 - 259, The preflight requeue in the key-generation flow uses an inline 30-second duration. Define a named constant alongside the existing 2-minute progressing-backoff constant, then use that constant in the syncContext.Queue().AddAfter call after generateKeySecret reports !preconditionMet.pkg/operator/apiserver/controllerset/apiservercontrollerset.go (1)
372-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the alignment padding and extra blank line
gofmtwill collapse the parameter alignment here and remove the extra blank line below.♻️ Proposed formatting fix
- resourceSyncer *resourcesynccontroller.ResourceSyncController, + resourceSyncer *resourcesynccontroller.ResourceSyncController,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/apiserver/controllerset/apiservercontrollerset.go` around lines 372 - 373, Run gofmt on the function signature containing resourceSyncer and encryptionStatusProvider, removing the manual alignment padding and extra blank line while preserving the parameter declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 370-376: Guard the KMS preflight path around
ensureKMSPreflightPassed so a nil encryptionStatusProvider is detected before
the call and returned as a clear configuration error. Preserve the existing
preflightPassed handling when the provider is present, and avoid changing
non-KMS behavior.
---
Nitpick comments:
In `@pkg/operator/apiserver/controllerset/apiservercontrollerset.go`:
- Around line 372-373: Run gofmt on the function signature containing
resourceSyncer and encryptionStatusProvider, removing the manual alignment
padding and extra blank line while preserving the parameter declarations.
In `@pkg/operator/encryption/controllers/key_controller_test.go`:
- Around line 550-553: Align the expectedActions composite-literal key with the
surrounding fields in the test case containing apiServerObjects,
targetNamespace, and encryptionStatusProvider, so the struct literal is
gofmt-compliant without changing its value.
- Around line 1062-1211: Add a subtest to TestKMSPreflightGate covering a
matching ObservedConfigHash with a stale Result.ConfigHash from an older
configuration, using both succeeded and failed result statuses if practical.
Assert Sync backs off without creating a key or returning the preflight failure
error, and without updating status; reuse computeExpectedHash, buildController,
and actionsWithoutKey.
In `@pkg/operator/encryption/controllers/key_controller.go`:
- Around line 426-438: Update getSecret and getConfigMap to check p.secret and
p.configMap for nil before accessing Namespace or Name; return the existing
mismatch error (or an equivalent safe error) when the prefetched object is
absent, while preserving the current matching-object return behavior.
- Around line 245-259: The preflight requeue in the key-generation flow uses an
inline 30-second duration. Define a named constant alongside the existing
2-minute progressing-backoff constant, then use that constant in the
syncContext.Queue().AddAfter call after generateKeySecret reports
!preconditionMet.
In `@pkg/operator/encryption/controllers/kms_preflight_controller.go`:
- Around line 53-63: Rename the kmsConfigHasher field
kmsConfigHasherResourceProvider to resources and update its constructor
assignment plus both call sites in the hasher methods to use h.resources. Run
gofmt so the struct field declarations are properly aligned.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b5f281c-38c1-4dbd-8ce6-386a2453eadf
📒 Files selected for processing (7)
pkg/operator/apiserver/controllerset/apiservercontrollerset.gopkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gotest/e2e-encryption/encryption_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-encryption/encryption_test.go`:
- Around line 1045-1057: Update noopKMSEncryptionStatusProvider so
GetKMSEncryptionStatus returns persisted status and UpdateKMSEncryptionStatus
applies the provided mutation to that status. Protect reads and mutations with
synchronization because key and preflight controllers access the provider
concurrently; keep ApplyKMSEncryptionStatus behavior unchanged unless required
for status persistence.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d0755b6-0594-4061-afb2-066a87421e69
📒 Files selected for processing (8)
pkg/operator/apiserver/controllerset/apiservercontrollerset.gopkg/operator/encryption/controllers.gopkg/operator/encryption/controllers/key_controller_test.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/kms/preflight/always_succeed_deployer.gopkg/operator/encryption/kms/preflight/always_succeed_deployer_test.gotest/e2e-encryption/encryption_test.go
ardaguclu
left a comment
ardaguclu
left a comment
There was a problem hiding this comment.
Just dropped a minor comment. Overall mechanism looks really nice to me.
| // NewAlwaysSucceedKMSPreflightDeployer returns a KMSPreflightDeployer that | ||
| // always reports a successful preflight without running any real check. | ||
| // Use as a temporary stand-in until a real pod-based deployer is available. | ||
| func NewAlwaysSucceedKMSPreflightDeployer() *AlwaysSucceedKMSPreflightDeployer { |
There was a problem hiding this comment.
Would it be better moving this under test/e2e-encryption?
There was a problem hiding this comment.
I was planning to use it tmp in production, for example openshift/cluster-kube-apiserver-operator@c65f8ca
The benefit would be that we would be testing the entire stack except the deployers.
WDYT ?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/controllers/kms_preflight_controller_test.go (1)
936-959: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the identity path skips KMS status reads.
The scenario checks cleanup and conditions, but it does not assert that
GetKMSEncryptionStatusis not called. A regression that moves status retrieval before the encryption-type check could pass this test. Configure the fake provider to fail on a status read or record reads and assert zero.This check uses the supplied identity-revert scenario and PR objective.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go` around lines 936 - 959, Update the identity-revert test case around the fakeEncryptionStatusProvider to detect KMS status reads, configuring it to fail or record calls when GetKMSEncryptionStatus is invoked. Assert that the read count remains zero while preserving the existing cleanup and non-degraded condition assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/operator/encryption/controllers/kms_preflight_controller_test.go`:
- Around line 936-959: Update the identity-revert test case around the
fakeEncryptionStatusProvider to detect KMS status reads, configuring it to fail
or record calls when GetKMSEncryptionStatus is invoked. Assert that the read
count remains zero while preserving the existing cleanup and non-degraded
condition assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f444232-2e42-43c1-8cb4-fda0f16f7820
📒 Files selected for processing (5)
pkg/operator/encryption/controllers/key_controller.gopkg/operator/encryption/controllers/kms_preflight_controller.gopkg/operator/encryption/controllers/kms_preflight_controller_test.gopkg/operator/encryption/kms/preflight/always_succeed_deployer.gopkg/operator/encryption/kms/preflight/always_succeed_deployer_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/encryption/kms/preflight/always_succeed_deployer_test.go
- pkg/operator/encryption/kms/preflight/always_succeed_deployer.go
- pkg/operator/encryption/controllers/key_controller.go
p0lyn0mial
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@p0lyn0mial: 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. |
openshift-ci
Bot
added
the
needs-rebase
a29f422 to
b8819dc
Compare
openshift-ci
Bot
removed
the
needs-rebase
240b024 to
9bdf2ef
Compare
|
|
||
| func (p *prefetchedKMSConfigHasherResourceProvider) getSecret(_ context.Context, namespace, name string) (*corev1.Secret, error) { | ||
| if p.secret == nil || p.secret.Namespace != namespace || p.secret.Name != name { | ||
| sNS, sName := "", "" |
There was a problem hiding this comment.
This is redundant, since we already return error?
|
|
||
| func (p *prefetchedKMSConfigHasherResourceProvider) getConfigMap(_ context.Context, namespace, name string) (*corev1.ConfigMap, error) { | ||
| if p.configMap == nil || p.configMap.Namespace != namespace || p.configMap.Name != name { | ||
| cmNS, cmName := "", "" |
There was a problem hiding this comment.
This is redundant, since we already return error?
| } | ||
| } | ||
|
|
||
| var refCM *corev1.ConfigMap |
There was a problem hiding this comment.
What about there is another referenced configMap?. This logic seems only work, if there is 1 configmap?
| // Fetch the referenced Secret and ConfigMap, copying their data into the | ||
| // key state. The fetched objects are reused by prefetchedKMSConfigHasherResourceProvider | ||
| // to compute the config hash without a second API round-trip. | ||
| var refSecret *corev1.Secret |
There was a problem hiding this comment.
Same, I think we shouldn't assume that there is only 1 referenced resource. Hash calculation should take account that there might be more. If optional another referenced Secret is updated in API, that won't trigger preflight controller, since hash is not changed.
There was a problem hiding this comment.
This is preexisting. The current code assumes only a single secret/cm.
does it make sense ?
There was a problem hiding this comment.
Yes, it would be pretty easy to switch to support multiple referenced data in key controller. But preflight hash calculator, in my opinion, should support multiple reference data without guessing any format in key controller. However, this can be fixed in a follow up PR.
| // | ||
| // Callers are responsible for requeuing when this returns (false, nil). | ||
| func (c *keyController) ensureKMSPreflightPassed(ctx context.Context, configHash string) (bool, error) { | ||
| encryptionStatus, err := c.encryptionStatusProvider.GetKMSEncryptionStatus(ctx) |
There was a problem hiding this comment.
I only see the fake encryptionStatusProviders or noop. Haven't we merged the prod ready encryptionStatusProvider yet?
There was a problem hiding this comment.
the providers live in the operator repos, for example: https://github.com/openshift/cluster-kube-apiserver-operator/blob/main/pkg/operator/encryptionstatusprovider/provider.go
9bdf2ef to
2c3fc98
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
a test pr at openshift/cluster-kube-apiserver-operator#2252 in general i got green https://prow.ci.openshift.org/pr-history/?org=openshift&repo=cluster-kube-apiserver-operator&pr=2252 the latest failure of |
2c3fc98 to
3ce65c5
Compare
|
/lgtm |
|
integration tests caught something or need to be updated https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_library-go/2392/pull-ci-openshift-library-go-master-e2e-aws-encryption/2084937438702080000 |
3ce65c5 to
4404c02
Compare
| } | ||
|
|
||
| func (p *noopKMSEncryptionStatusProvider) UpdateKMSEncryptionStatus(_ context.Context, _ func(*operatorv1.KMSEncryptionStatus)) error { | ||
| return nil |
There was a problem hiding this comment.
In integration tests, key controller expect the key to be created. I think this mock should return successful status, so that key is created.
There was a problem hiding this comment.
I think you are right. Testing locally. Thanks.
4404c02 to
c16225a
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 |
|
@p0lyn0mial: 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. |
3 participants
Summary by CodeRabbit