CNTRLPLANE-3237: Add KMS preflight check API to operator/v1#2916
CNTRLPLANE-3237: Add KMS preflight check API to operator/v1#2916p0lyn0mial wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @p0lyn0mial! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
/assign @everettraven (since you reviewed the other KMS related PRs) |
|
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:
📝 WalkthroughWalkthroughThis PR adds KMS preflight status types to 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| // Preflight is considered passed when result.status is Healthy and | ||
| // result.detail matches requiredConfigHash. | ||
| // +optional | ||
| Result KMSPluginHealthReport `json:"result,omitempty,omitzero"` |
There was a problem hiding this comment.
Note that not all fields of the result will be needed, but for simplicity we reuse the health report struct. The pod runs on some node, for some keyId, and we read back kekId and status. We slightly abuse the detail field for storing the
config hash so the key controller can match it against requiredConfigHash.
There was a problem hiding this comment.
I think in general we would need:
- status (Success / Failure)
- hash
- kekId
in the result struct. I think we don't need to provide more info for failures because the preflight controller will post its own degraded condition.
There was a problem hiding this comment.
I'm +1 on creating new fields here, there are too many other fields we don't need in the health report.
Status could also be a simple boolean field "PreflightSucceeded".
There was a problem hiding this comment.
I'm fine either way. Technically, a preflight could be considered a one time health check.
There was a problem hiding this comment.
technically yes, I'm also cool if we extract the fields we need into a shared and embedded struct. For only three fields it's a bigger change though.
There was a problem hiding this comment.
If you only need the three fields and this is a distinctly different use than the existing health check API - I'd recommend using a new type for the preflights specifically.
| type KMSPreflightCheck struct { | ||
| // requiredConfigHash is the hash of the KMS provider configuration that | ||
| // requires preflight validation before a new encryption key can be created. | ||
| // The hash is computed using FNV-32 and base64 URL-encoded. |
There was a problem hiding this comment.
maybe we should remove this detail.
| // requires preflight validation before a new encryption key can be created. | ||
| // The hash is computed using FNV-32 and base64 URL-encoded. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=8 |
There was a problem hiding this comment.
Since FNV-32 always outputs exactly 4 bytes, the base64 output is always exactly 8 characters.
There was a problem hiding this comment.
We could always require 8 chars. We unlikely will change the hash alg in the future.
There was a problem hiding this comment.
Requiring exactly 8 characters seems reasonable to me.
Additionally, is there any additional formatting constraints we can put here to ensure that only correctly shaped 8 character strings can be used here?
There was a problem hiding this comment.
the hash is base64 encoded. i haven't found base64 format validation in the codebase.
also in general the hash is produced and consumed by controllers, not users.
not sure if we want to add any additional validation.
There was a problem hiding this comment.
Stackoverflow suggests we could validate with a pattern ^[-A-Za-z0-9+/]*={0,3}$
There was a problem hiding this comment.
since we use base64 URL encoding (- and _ instead of + and /) I think we would need ^[A-Za-z0-9_-]*={0,3}$
but since the hash will be provided by the controller do we want to impose additional validation rules that are implementation details?
|
/assign @tjungblu |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@operator/v1/types_kmsencryption.go`:
- Around line 103-107: The `Result` field in `KMSPluginSpec` documents the
preflight match semantics only when it is present, but it does not say what
omission means. Update the field comment on `Result KMSPluginHealthReport` to
explicitly describe the omitted case, such as when no preflight check has
completed yet or no result is available, while keeping the existing `+optional`
and match-behavior explanation.
- Around line 95-101: The RequiredConfigHash field in KMS encryption types is
documented with an imprecise length range that does not match the fixed FNV-32
base64 URL encoding. Update the comment on RequiredConfigHash in
types_kmsencryption.go to state the exact encoded length, and adjust the
kubebuilder MinLength/MaxLength validations to the true length produced by the
encoding so the schema matches the actual value.
In
`@payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml`:
- Around line 258-263: The `preflight` description in the CRD is misleading
because it implies admission-time validation of the resource, while the actual
flow is async controller-driven and only gates key creation. Update the wording
in the `preflight` description to reflect that it validates KMS provider
configuration before it is used to create a new encryption key, and avoid
phrases like “admitted into the system” that suggest webhook/admission
enforcement.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3eead296-70ae-4f3d-81e3-29f0cf062f95
⛔ Files ignored due to path filters (16)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.model_name.gois excluded by!**/zz_generated*operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
operator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
|
Some high-level comments/questions:
|
|
thanks for taking a look at the pr.
isn't this a standard kube approach ? How does it differ from It could be that the name is unfortunate. We could rename it to
yes, the preflight will run on every configuration change. please take a look at https://github.com/openshift/library-go/blob/master/pkg/operator/encryption/controllers/kms_preflight_controller.go#L181 which contains more details.
the configuration used by the system is actually combined from three resources. it is the kms provider cfg, the secret which holds credentials and the configmap which holds the ca bundle. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Thanks for the additional context, it was helpful for the review.
Have a handful of comments.
I'll be out for a little over a week after today. @JoelSpeed or another API reviewer will likely need to pick this up from here.
| // The preflight validates the API configuration posted by users before it is | ||
| // admitted into the system, catching configuration issues early such as | ||
| // incorrect login credentials or an unreachable Vault service. | ||
| // When omitted, no preflight validation is in progress or has been requested. |
There was a problem hiding this comment.
Is it actually that no preflight validation has been performed yet? IIRC the preflight validation doesn't have to be requested?
| // incorrect login credentials or an unreachable Vault service. | ||
| // When omitted, no preflight validation is in progress or has been requested. | ||
| // +optional | ||
| Preflight KMSPreflightCheck `json:"preflight,omitempty,omitzero"` |
There was a problem hiding this comment.
Nit: Just omitzero should be sufficient here
| // +required | ||
| RequiredConfigHash string `json:"requiredConfigHash"` |
There was a problem hiding this comment.
From the additional context, I agree that requiredConfigHash doesn't seem like the most appropriate naming for what this field is actually used for - I do like observedConfigHash better.
|
|
||
| // result contains the outcome of the most recent preflight check. | ||
| // Preflight is considered passed when result.status is Healthy and | ||
| // result.detail matches requiredConfigHash. |
There was a problem hiding this comment.
Why does result.detail need to match requiredConfigHash? Is requiredConfigHash not set by the preflight controller?
| // Preflight is considered passed when result.status is Healthy and | ||
| // result.detail matches requiredConfigHash. | ||
| // +optional | ||
| Result KMSPluginHealthReport `json:"result,omitempty,omitzero"` |
There was a problem hiding this comment.
If you only need the three fields and this is a distinctly different use than the existing health check API - I'd recommend using a new type for the preflights specifically.
| // requires preflight validation before a new encryption key can be created. | ||
| // The hash is computed using FNV-32 and base64 URL-encoded. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=8 |
There was a problem hiding this comment.
Requiring exactly 8 characters seems reasonable to me.
Additionally, is there any additional formatting constraints we can put here to ensure that only correctly shaped 8 character strings can be used here?
|
[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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operator/v1/types_kmsencryption.go (1)
90-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
MinProperties=1onKMSPreflightCheck.
ObservedConfigHashis already+required, soMinProperties=1at the type level is always satisfied and adds no additional guarantee. Harmless, but worth dropping for clarity if touching this again.🤖 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 `@operator/v1/types_kmsencryption.go` around lines 90 - 94, Remove the redundant type-level MinProperties=1 from KMSPreflightCheck in types_kmsencryption.go since ObservedConfigHash is already marked required; keep the struct definition and field validation focused on the field-level requirement, and only retain validation markers that add meaningful constraints.
🤖 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 `@operator/v1/types_kmsencryption.go`:
- Around line 90-94: Remove the redundant type-level MinProperties=1 from
KMSPreflightCheck in types_kmsencryption.go since ObservedConfigHash is already
marked required; keep the struct definition and field validation focused on the
field-level requirement, and only retain validation markers that add meaningful
constraints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: effa453a-6879-4c31-b319-6a0cfdab4809
⛔ Files ignored due to path filters (16)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.model_name.gois excluded by!**/zz_generated*operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (10)
operator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
p0lyn0mial
left a comment
p0lyn0mial
left a comment
There was a problem hiding this comment.
Hey @JoelSpeed , let me know if you have time to review this PR. If not, I'll try to find a different reviewer.
Also, I haven't added the tests yet. I'll do that once we sort out the details.
| // requires preflight validation before a new encryption key can be created. | ||
| // The hash is computed using FNV-32 and base64 URL-encoded. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=8 |
There was a problem hiding this comment.
the hash is base64 encoded. i haven't found base64 format validation in the codebase.
also in general the hash is produced and consumed by controllers, not users.
not sure if we want to add any additional validation.
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +required | ||
| KEKId string `json:"kekId"` |
There was a problem hiding this comment.
same as
api/operator/v1/types_kmsencryption.go
Line 56 in a33e8e1
| // Decrypt calls successfully. | ||
| // Failed means the validation did not pass. | ||
| // +required | ||
| Status KMSPreflightResultStatus `json:"status"` |
There was a problem hiding this comment.
I think boolean values are generally discouraged, so I decided to use an enum.
We don't need an additional details field because the result is only used for the key controller's gating decision.
The preflight controller sets its own EncryptionKMSPreflightControllerDegraded operator condition with the error details (reason and message).
There was a problem hiding this comment.
Correct, though we try to avoid proxies (e.g. success failure semantics) in favour of describing the actual behaviour an end user would observe. I haven't gone into this enough to suggest what the end user effect of this is
There was a problem hiding this comment.
The field is only used by the controllers. I am fine either way.
|
|
||
| // KMSPreflightCheck describes a preflight validation request and its result. | ||
| // | ||
| // +openshift:compatibility-gen:level=1 |
There was a problem hiding this comment.
Does this have an effect at this level? We normally only see this on the root type
There was a problem hiding this comment.
I just followed what we did for
api/operator/v1/types_kmsencryption.go
Line 19 in a33e8e1
| // requires preflight validation before a new encryption key can be created. | ||
| // The hash is computed using FNV-32 and base64 URL-encoded. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=8 |
There was a problem hiding this comment.
Stackoverflow suggests we could validate with a pattern ^[-A-Za-z0-9+/]*={0,3}$
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=8 | ||
| // +required | ||
| ObservedConfigHash string `json:"observedConfigHash"` |
There was a problem hiding this comment.
Linter should be complaining of a lack of omitempty here
| // +required | ||
| ConfigHash string `json:"configHash"` | ||
|
|
||
| // kekId refers to the remote KEK id from KMS v2 StatusResponse.key_id. |
There was a problem hiding this comment.
Normally try to avoid acronyms in APIs, kek is keyEncryptionKey?
There was a problem hiding this comment.
The upstream is keyID not kekID, would it make sense to align to keyID?
There was a problem hiding this comment.
ah, right, I know it's confusing.
the reason is that a keyID is used in OCP (xref). You can think of it as the unique ID of a secret that represents a KMS plugin instance. We decided to use secrets because the existing symmetric encryption implementation already stores encryption keys in secrets - and the entire machines is based on that.
So we decided to use kekID for remote encryption keys instead. I hope it make more sense now.
There was a problem hiding this comment.
Oh boy... If that's confusing for me, it's going to be confusing for others too
What about
remoteKeyEncryptionKey:
keyID: ...
Are we likely to have any additional kek related fields in the future that might make sense as logical siblings to the current kekID?
There was a problem hiding this comment.
since OCP already uses keyID I would not change it.
for the other key, how about: remoteKeyEncryptionKeyID, remoteEncryptionKeyID or just remoteKeyID ?
There was a problem hiding this comment.
I like remoteKeyID
There was a problem hiding this comment.
renamed to remoteKeyID . we should also rename
api/operator/v1/types_kmsencryption.go
Line 56 in a33e8e1
I can do that in this PR or a new one.
p0lyn0mial
force-pushed
the
kms-preflight-check-api
branch
from
a33e8e1 to
5e3b0de
Compare
p0lyn0mial
changed the title
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml (1)
167-214: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the complete
resultcontract.The tests cover valid result objects and invalid
observedConfigHashvalues, but not missing/invalidresult.configHash,remoteKeyID, orstatus, malformed result hashes, or the documented hash-mismatch state. Add those cases, including a failed result withoutremoteKeyIDif the controller permits it.Based on the CRD contract added in this cohort, each new validation rule should have regression coverage.
Also applies to: 272-326
🤖 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 `@operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml` around lines 167 - 214, Add negative regression cases alongside “Should accept preflight with a Succeeded result” and the related preflight tests for missing or invalid result.configHash, remoteKeyID, and status, malformed hashes, and the documented config-hash mismatch state. Include a Failed result without remoteKeyID if permitted by the controller, and assert each case’s expected validation or normalization behavior according to the CRD contract.
🤖 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
`@operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml`:
- Around line 167-228: Add a test case with different observedConfigHash and
result.configHash values, asserting the expected schema validation error instead
of acceptance. Add equivalent mismatched-hash rejection cases to the
KubeAPIServer and Authentication test suites, alongside the existing preflight
result tests.
In `@operator/v1/types_kmsencryption.go`:
- Around line 95-103: The comments for ObservedConfigHash, status, configHash,
and remoteKeyID must document their validation markers: explicitly state that
each +required field is required, and explain that each Pattern accepts URL-safe
Base64 characters with optional “=” padding. Update the corresponding field
comments while preserving the existing markers and validation behavior.
- Line 101: Restrict the validation pattern for the KMS hash fields
observedConfigHash and configHash to canonical padded Base64 representing
exactly 4 bytes, matching values such as AQAAAA==; update the kubebuilder regex
accordingly and ensure both fields use it consistently.
- Around line 90-110: Add a Kubernetes CEL cross-field validation to
KMSPreflightCheck requiring result.configHash to equal observedConfigHash when
the result is present, while preserving support for an omitted result. Update
the API-generated CRDs and regenerate the associated validation tests.
In
`@payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml`:
- Around line 224-286: Document the new validation constraints in the source API
comments for the preflight fields, including that preflight is non-empty,
observedConfigHash is required and exactly 8 characters using the permitted
pattern, and result is required when applicable. Update the relevant types and
validation markers around the KMS encryption preflight definitions in
types_kmsencryption.go, then regenerate all CRDs so their descriptions match the
source documentation.
- Around line 233-259: Update the observedConfigHash and result.configHash
schema patterns to match padded base64url encoding: allow the URL-safe alphabet
with valid 0–2 padding characters only in the correct position, and reject
malformed padding or mixed standard/base64url characters. Add validation cases
covering valid padded values plus invalid padding such as AAAAA=== and invalid
alphabet characters.
---
Nitpick comments:
In `@operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml`:
- Around line 167-214: Add negative regression cases alongside “Should accept
preflight with a Succeeded result” and the related preflight tests for missing
or invalid result.configHash, remoteKeyID, and status, malformed hashes, and the
documented config-hash mismatch state. Include a Failed result without
remoteKeyID if permitted by the controller, and assert each case’s expected
validation or normalization behavior according to the CRD contract.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 814b0948-712c-4910-8a10-8e315015bc67
⛔ Files ignored due to path filters (16)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.model_name.gois excluded by!**/zz_generated*operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (13)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yaml
| - name: Should accept preflight with a Succeeded result | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: {} | ||
| updated: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: {} | ||
| status: | ||
| encryptionStatus: | ||
| preflight: | ||
| observedConfigHash: AQAAAA== | ||
| result: | ||
| status: Succeeded | ||
| configHash: AQAAAA== | ||
| remoteKeyID: vault-transit-key-v3 | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| status: | ||
| encryptionStatus: | ||
| preflight: | ||
| observedConfigHash: AQAAAA== | ||
| result: | ||
| status: Succeeded | ||
| configHash: AQAAAA== | ||
| remoteKeyID: vault-transit-key-v3 | ||
| - name: Should accept preflight with a Failed result | ||
| initial: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: {} | ||
| updated: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: {} | ||
| status: | ||
| encryptionStatus: | ||
| preflight: | ||
| observedConfigHash: AQAAAA== | ||
| result: | ||
| status: Failed | ||
| configHash: AQAAAA== | ||
| remoteKeyID: vault-transit-key-v3 | ||
| expected: | | ||
| apiVersion: operator.openshift.io/v1 | ||
| kind: OpenShiftAPIServer | ||
| spec: | ||
| logLevel: Normal | ||
| operatorLogLevel: Normal | ||
| status: | ||
| encryptionStatus: | ||
| preflight: | ||
| observedConfigHash: AQAAAA== | ||
| result: | ||
| status: Failed | ||
| configHash: AQAAAA== | ||
| remoteKeyID: vault-transit-key-v3 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add a mismatched-hash rejection test.
The accepted result cases only use identical observedConfigHash and configHash, so they cannot catch the missing cross-field validation. Add a case with different hashes and assert the schema error; mirror it in the KubeAPIServer and Authentication suites.
🤖 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
`@operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml`
around lines 167 - 228, Add a test case with different observedConfigHash and
result.configHash values, asserting the expected schema validation error instead
of acceptance. Add equivalent mismatched-hash rejection cases to the
KubeAPIServer and Authentication test suites, alongside the existing preflight
result tests.
| // KMSPreflightCheck describes a preflight validation request and its result. | ||
| // | ||
| // +openshift:compatibility-gen:level=1 | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type KMSPreflightCheck struct { | ||
| // observedConfigHash is a hash of the KMS provider configuration and | ||
| // its referenced resources that has been observed and requires preflight | ||
| // validation before a new encryption key can be created. | ||
| // The value must be exactly 8 characters. | ||
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=8 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9_-]*={0,3}$` | ||
| // +required | ||
| ObservedConfigHash string `json:"observedConfigHash,omitempty"` | ||
|
|
||
| // result contains the outcome of the most recent preflight check. | ||
| // Preflight is considered passed when result.status is Succeeded and | ||
| // result.configHash matches observedConfigHash. | ||
| // When omitted, no preflight check result has been reported yet. | ||
| // +optional | ||
| Result KMSPreflightResult `json:"result,omitzero"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline operator/v1/types_kmsencryption.go --view expanded || true
echo
echo "== relevant lines =="
sed -n '1,220p' operator/v1/types_kmsencryption.go | cat -n
echo
echo "== search for XValidation/configHash =="
rg -n "XValidation|configHash|observedConfigHash|KMSPreflightCheck" operator/v1 -g '!**/vendor/**' || trueRepository: openshift/api
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema validation for KMSPreflightCheck =="
rg -n -C 3 "KMSPreflightCheck|observedConfigHash|configHash|XValidation" \
operator/v1/zz_generated.* operator/v1/tests/*/KMSEncryption.yaml || true
echo
echo "== KMSEncryption test cases around preflight =="
for f in operator/v1/tests/*/KMSEncryption.yaml; do
echo "--- $f ---"
rg -n -C 2 "preflight|observedConfigHash|configHash|mismatch|reject" "$f" || true
doneRepository: openshift/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in operator/v1/zz_generated.crd-manifests/*.crd.yaml; do
if rg -n "KMSPreflightCheck|observedConfigHash|configHash|x-kubernetes-validations" "$f" >/tmp/kmspref.$$ 2>/dev/null; then
echo "--- $f ---"
cat /tmp/kmspref.$$
echo
fi
done
rm -f /tmp/kmspref.$$Repository: openshift/api
Length of output: 44406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml \
operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml \
operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml
do
echo "--- $f ---"
rg -n -C 8 "observedConfigHash|configHash|x-kubernetes-validations|result.status|Succeeded|Failed" "$f" | sed -n '1,120p'
echo
doneRepository: openshift/api
Length of output: 20180
Add a cross-field validation for the preflight hashes.
KMSPreflightCheck says preflight passes only when result.configHash matches observedConfigHash, but the schema only requires both fields independently. Add the XValidation rule here and regenerate the CRDs/tests.
🤖 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 `@operator/v1/types_kmsencryption.go` around lines 90 - 110, Add a Kubernetes
CEL cross-field validation to KMSPreflightCheck requiring result.configHash to
equal observedConfigHash when the result is present, while preserving support
for an omitted result. Update the API-generated CRDs and regenerate the
associated validation tests.
Source: Coding guidelines
| // The value must be exactly 8 characters. | ||
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=8 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9_-]*={0,3}$` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file and nearby definitions
FILE="operator/v1/types_kmsencryption.go"
wc -l "$FILE"
sed -n '70,170p' "$FILE"
# Find references to the relevant field/type and validation markers
rg -n "AQAAAA==|Pattern=`\^\[A-Za-z0-9_-\\]\*={0,3}\$`|Base64|base64|kmsencryption|key" operator/v1 -SRepository: openshift/api
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="operator/v1/types_kmsencryption.go"
TEST="operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml"
echo "=== types_kmsencryption.go (relevant lines) ==="
nl -ba "$FILE" | sed -n '90,150p'
echo
echo "=== KMSEncryption.yaml (relevant observedConfigHash/configHash tests) ==="
nl -ba "$TEST" | sed -n '140,330p'Repository: openshift/api
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="operator/v1/types_kmsencryption.go"
TEST="operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml"
echo "=== types_kmsencryption.go relevant slice ==="
sed -n '96,150p' "$FILE"
echo
echo "=== KMSEncryption.yaml observedConfigHash/configHash cases ==="
grep -n -A8 -B4 'observedConfigHash: AQAAAA==\|invalid base64 characters\|configHash: AQAAAA==\|Should reject observedConfigHash' "$TEST"Repository: openshift/api
Length of output: 7995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
pat = re.compile(r'^[A-Za-z0-9_-]*={0,3}$')
samples = [
"AQAAAA==", # current test value
"ABCDEFGH", # 8 data chars, no padding
"ABCDEFG=", # 7 + 1
"ABCDEF==", # 6 + 2
"ABCDE===", # 5 + 3
"ABCD====", # too much padding
]
for s in samples:
print(s, len(s), bool(pat.fullmatch(s)))
PY
echo
echo "=== Remaining KMSEncryption.yaml slice ==="
sed -n '313,340p' operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlRepository: openshift/api
Length of output: 773
Restrict the KMS hash fields to canonical padded Base64.
observedConfigHash and configHash currently accept non-canonical 8-char forms like ABCDEFGH, ABCDEFG=, and ABCDE===. If these are meant to be the padded 4-byte URL-safe Base64 values used in the tests (AQAAAA==), tighten the regex to that shape; otherwise switch to raw encoding and update the length constraints.
🤖 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 `@operator/v1/types_kmsencryption.go` at line 101, Restrict the validation
pattern for the KMS hash fields observedConfigHash and configHash to canonical
padded Base64 representing exactly 4 bytes, matching values such as AQAAAA==;
update the kubebuilder regex accordingly and ensure both fields use it
consistently.
| // The value must be exactly 8 characters. | ||
| // +kubebuilder:validation:MinLength=8 | ||
| // +kubebuilder:validation:MaxLength=8 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9_-]*={0,3}$` |
There was a problem hiding this comment.
or actually this should be ^[A-Za-z0-9_-]*={0,2}$
p0lyn0mial
force-pushed
the
kms-preflight-check-api
branch
2 times, most recently
from
c838aa8 to
7744172
Compare
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (3)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml (1)
498-520: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
remoteKeyIDupper bound.The API contract also limits
remoteKeyIDto 1024 characters, but this suite only tests the empty value. Add a 1025-character value and assert the maximum-length validation error, mirroring the other resource suites.🤖 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 `@operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml` around lines 498 - 520, Extend the “Should reject empty remoteKeyID” test coverage in the KMSEncryption test suite with a case using a 1025-character remoteKeyID, and assert the API validation error for exceeding the 1024-character maximum, matching the equivalent resource-suite tests.operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml (1)
399-416: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
remoteKeyIDupper bound.The suite checks only an empty value, but the API contract also requires rejection above 1024 characters. Add a 1025-character test case to protect the generated CRD schema.
🤖 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 `@operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml` around lines 399 - 416, Add a test case alongside “Should reject empty remoteKeyID” in the KMSEncryption validation suite using a remoteKeyID containing 1025 characters, and assert the status error indicates the maximum length of 1024 is exceeded, covering the generated CRD schema upper bound.operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml (1)
399-416: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
remoteKeyIDupper bound.This suite tests only an empty
remoteKeyID, while the API contract also enforces a 1024-character maximum. Add a 1025-character rejection case and keep it synchronized with the other resource suites.🤖 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 `@operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml` around lines 399 - 416, Add a test case alongside “Should reject empty remoteKeyID” in the KubeAPIServer encryption status validation suite that sets result.remoteKeyID to exactly 1025 characters and expects the maximum-length validation error; mirror the corresponding test and naming conventions used by the other resource suites.
🤖 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 `@operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yaml`:
- Around line 498-520: Extend the “Should reject empty remoteKeyID” test
coverage in the KMSEncryption test suite with a case using a 1025-character
remoteKeyID, and assert the API validation error for exceeding the
1024-character maximum, matching the equivalent resource-suite tests.
In `@operator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yaml`:
- Around line 399-416: Add a test case alongside “Should reject empty
remoteKeyID” in the KubeAPIServer encryption status validation suite that sets
result.remoteKeyID to exactly 1025 characters and expects the maximum-length
validation error; mirror the corresponding test and naming conventions used by
the other resource suites.
In
`@operator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yaml`:
- Around line 399-416: Add a test case alongside “Should reject empty
remoteKeyID” in the KMSEncryption validation suite using a remoteKeyID
containing 1025 characters, and assert the status error indicates the maximum
length of 1024 is exceeded, covering the generated CRD schema upper bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5c249432-0c73-43df-9213-a66bf6522279
⛔ Files ignored due to path filters (17)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/authentications.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/kubeapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.featuregated-crd-manifests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.model_name.gois excluded by!**/zz_generated*operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (13)
operator/v1/tests/authentications.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/kubeapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/tests/openshiftapiservers.operator.openshift.io/KMSEncryption.yamloperator/v1/types_kmsencryption.gopayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-CustomNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_50_authentication_01_authentications-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_30_openshift-apiserver_01_openshiftapiservers-TechPreviewNoUpgrade.crd.yaml
- operator/v1/types_kmsencryption.go
- payload-manifests/crds/0000_50_authentication_01_authentications-CustomNoUpgrade.crd.yaml
p0lyn0mial
force-pushed
the
kms-preflight-check-api
branch
from
7744172 to
76b836e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@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. |
5 participants
In general the preflight validates the API configuration posted by users before an encryption key is created for it. We want to catch configuration issues early like incorrect login credentials or an unreachable Vault service.
There is a coordination between the key controller and the preflight controller (new controller, still being developed). The key controller "asks" for checks for the given config by setting requiredConfigHash (a hash over the current configuration).
The preflight controller schedules a pod which implements the checks and posts its results as pod conditions. The preflight controller reads those conditions, sets its own operator conditions (like Degraded, InProgress for admins), and
fills the result for the key controller to decide next steps. The key controller considers preflight passed when result.status is Healthy and result.detail matches requiredConfigHash.
Note that not all fields of the result will be needed, but for simplicity we reuse the health report struct. The pod runs on some node, for some keyId, and we read back kekId and status. We slightly abuse the detail field for storing the
config hash so the key controller can match it against requiredConfigHash.
A link to the KEP https://github.com/openshift/enhancements/blob/master/enhancements/kube-apiserver/kms-encryption-foundations.md (the content might be a bit outdated)
I'm planning to add some tests.