CNTRLPLANE-2012: Add PKI config types, validation, and CR manifest generation#10593
CNTRLPLANE-2012: Add PKI config types, validation, and CR manifest generation#10593hasbro17 wants to merge 1 commit into
Conversation
|
@hasbro17: This pull request references CNTRLPLANE-2012 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 either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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-robot
added
the
jira/valid-reference
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
WalkthroughAdds feature-gated configurable PKI: InstallConfig types and CRD schema, defaults and conversion to upstream PKI profile, validation and feature-gate wiring, a manifest asset to emit a PKI CR, and tests/print output updates. ChangesConfigurable PKI Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/types/pki/validation.go (1)
12-116: ⚡ Quick winMake the
fipsflag meaningful or remove it for now.
fipsis threaded through every PKI validator here but never read, so FIPS-enabled installs currently accept the exact same algorithms, key sizes, and curves as non-FIPS installs. That leaves the validator contract misleading and the FIPS-specific path effectively unimplemented.🤖 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/types/pki/validation.go` around lines 12 - 116, The fips boolean is never used — either remove it from the PKI validator signatures or make it enforce FIPS-safe choices; implement the latter: use the existing fips parameter in ValidateKeyConfig, validateRSAKeyConfig, and validateECDSAKeyConfig to tighten accepted values when fips==true (e.g. in validateRSAKeyConfig, require a larger minimum RSA keySize when fips is true such as >=3072 and still multiples of 1024; in validateECDSAKeyConfig, disallow non-FIPS curves like P521 when fips is true and limit to P256/P384; and in ValidateKeyConfig ensure the Forbidden/Required checks still apply under the fips branch), keeping the same function names (ValidatePKIConfig, ValidateKeyConfig, validateRSAKeyConfig, validateECDSAKeyConfig) so callers remain consistent.pkg/asset/manifests/pki_test.go (1)
140-142: 💤 Low valueConsider verifying the Custom field is nil/empty in Default mode.
The test correctly checks that the mode is Default, but doesn't verify that the
Customfield is nil or empty. While checking the mode is sufficient, explicitly assertingpkiCR.Spec.CertificateManagement.Customis empty would make the test more thorough.Optional test enhancement
if tc.expectMode != configv1alpha1.PKICertificateManagementModeCustom { + assert.Empty(t, pkiCR.Spec.CertificateManagement.Custom) return }🤖 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/asset/manifests/pki_test.go` around lines 140 - 142, The test currently returns early when tc.expectMode != configv1alpha1.PKICertificateManagementModeCustom; add an explicit assertion for the Default case to verify pkiCR.Spec.CertificateManagement.Custom is nil/empty. Locate the check around tc.expectMode and after confirming the mode is Default, assert that pkiCR.Spec.CertificateManagement.Custom is nil or has zero length (using t.Fatalf/t.Errorf or your test assertion helper) to make the test more thorough.
🤖 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 `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 5088-5098: The description for installConfig.pki currently states
signerCertificates affects "all installer-generated certificate authority (CA)
certificates" which is incorrect for this PR; update the text in the YAML
properties for signerCertificates (and any parent description referencing
installConfig.pki and ConfigurablePKI) to clearly state that the PKI block is
emitted for day-2 rotation/rotation operations and does not change
installer-time certificate generation, and mention the feature gate
ConfigurablePKI as controlling emission for rotation only so oc explain
consumers are not misled.
In `@pkg/types/pki/defaults.go`:
- Around line 41-50: EffectiveSignerPKIConfig() is hardcoding RSA-4096 signer
defaults instead of deriving them from the single source of truth
DefaultPKIProfile(); update EffectiveSignerPKIConfig() to construct its returned
*types.PKIConfig by reading DefaultPKIProfile().SignerCertificates (and nested
Key/Key.RSA fields) rather than re-encoding literal RSA-4096 values so future
changes to DefaultPKIProfile() automatically propagate to the signer path.
In `@pkg/types/validation/installconfig_test.go`:
- Around line 3121-3134: The test case "invalid PKI signer with unsupported
algorithm" is missing the TechPreview feature gate for ConfigurablePKI; update
the installConfig builder (the anonymous func returning *types.InstallConfig) to
set c.FeatureSet = configv1.TechPreviewNoUpgrade so the PKI field validation
executes, or alternatively change expectedError to the feature-gate validation
message you use elsewhere (i.e., assert a feature-gate error) — adjust either
the installConfig() setup or expectedError to consistently reflect whether
ConfigurablePKI is enabled; check symbols: types.InstallConfig, c.PKI
(types.PKIConfig), and c.FeatureSet (configv1.TechPreviewNoUpgrade).
- Around line 3136-3150: The test case creates an InstallConfig via the inline
installConfig func that sets c.PKI but does not enable the tech-preview feature
gate; to match the other PKI tests and exercise the PKI validation path, update
that inline function (the one calling validInstallConfig() and setting c.PKI /
types.PKIConfig) to also set c.FeatureSet = configv1.TechPreviewNoUpgrade (same
as in the test around line 3108) so the PKI field validation is executed; update
imports if needed to reference configv1.TechPreviewNoUpgrade.
In `@pkg/types/validation/installconfig.go`:
- Around line 288-290: ValidateInstallConfig currently validates c.PKI via
pkivalidation.ValidatePKIConfig(c.PKI, field.NewPath("pki"), c.FIPS) before
feature-gate checks, causing duplicate/noisy errors when ConfigurablePKI is
disabled; change the flow so PKI content validation only runs if the
ConfigurablePKI feature is enabled (i.e. after or guarded by
ValidateFeatureSet/feature-gate check), by wrapping the existing c.PKI
validation block with the feature-gate condition or moving that call to after
ValidateFeatureSet so that ValidatePKIConfig is invoked only when the
ConfigurablePKI gate allows it.
---
Nitpick comments:
In `@pkg/asset/manifests/pki_test.go`:
- Around line 140-142: The test currently returns early when tc.expectMode !=
configv1alpha1.PKICertificateManagementModeCustom; add an explicit assertion for
the Default case to verify pkiCR.Spec.CertificateManagement.Custom is nil/empty.
Locate the check around tc.expectMode and after confirming the mode is Default,
assert that pkiCR.Spec.CertificateManagement.Custom is nil or has zero length
(using t.Fatalf/t.Errorf or your test assertion helper) to make the test more
thorough.
In `@pkg/types/pki/validation.go`:
- Around line 12-116: The fips boolean is never used — either remove it from the
PKI validator signatures or make it enforce FIPS-safe choices; implement the
latter: use the existing fips parameter in ValidateKeyConfig,
validateRSAKeyConfig, and validateECDSAKeyConfig to tighten accepted values when
fips==true (e.g. in validateRSAKeyConfig, require a larger minimum RSA keySize
when fips is true such as >=3072 and still multiples of 1024; in
validateECDSAKeyConfig, disallow non-FIPS curves like P521 when fips is true and
limit to P256/P384; and in ValidateKeyConfig ensure the Forbidden/Required
checks still apply under the fips branch), keeping the same function names
(ValidatePKIConfig, ValidateKeyConfig, validateRSAKeyConfig,
validateECDSAKeyConfig) so callers remain consistent.
🪄 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: 1ba2c9a4-d566-448c-9f76-1b7ae60ac656
⛔ Files ignored due to path filters (1)
pkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (16)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/manifests/operators.gopkg/asset/manifests/pki.gopkg/asset/manifests/pki_test.gopkg/explain/printer_test.gopkg/types/defaults/installconfig.gopkg/types/installconfig.gopkg/types/pki/conversion.gopkg/types/pki/defaults.gopkg/types/pki/defaults_test.gopkg/types/pki/validation.gopkg/types/pki/validation_test.gopkg/types/validation/featuregate_test.gopkg/types/validation/featuregates.gopkg/types/validation/installconfig.gopkg/types/validation/installconfig_test.go
hasbro17
force-pushed
the
pki-1-types-and-manifest
branch
from
f1faa3f to
6f6a58a
Compare
| RSA: configv1alpha1.RSAKeyConfig{KeySize: local.Key.RSA.KeySize}, | ||
| ECDSA: configv1alpha1.ECDSAKeyConfig{Curve: configv1alpha1.ECDSACurve(local.Key.ECDSA.Curve)}, | ||
| }, |
There was a problem hiding this comment.
Can we have a test run the output through yaml.Marshall and confirms only one is set? Or, check Algorithm before setting only one of either RSA/ECDSA? I think this is ok thanks to omitzero, but a test or explicitly check might add clarity.
There was a problem hiding this comment.
Makes sense, I'll add a new test in conversion_test.go to marshal the converted output and confirm only the relevant key config appears (RSA output omits ecdsa:, ECDSA output omits rsa:). So if our omitzero reliance breaks we'll know.
| // ECDSA certificate rotation. Once operator support lands, switch to ECDSA P-384 | ||
| // signers and ECDSA P-256 defaults to match the upstream library-go profile: | ||
| // https://github.com/openshift/library-go/blob/12d8376369b7c5b76f688d01089882ca28e351c3/pkg/pki/profile.go#L11-L26 | ||
| func DefaultPKIProfile() configv1alpha1.PKIProfile { |
There was a problem hiding this comment.
Consider removing this entire file from this PR. Just return the config below in PKIConfiguration.Generate for now. I have comments about EffectiveSignerPKIConfig , but it moke no sense if its not being called yet.
There was a problem hiding this comment.
For DefaultPKIProfile() the manifest generator uses it now, and PR 3 will use it via EffectiveSignerPKIConfig(). Defining it in the manifest generator and then extracting it later is just code churn for no gain.
It's the single source of truth for the default profile, shared between the manifest generator (this PR) and the TLS layer so I'd rather define it more explicitly for when the time comes to change it or migrate off from it to something defined in openshift/api as Ben alluded to. Or if we end up sharing the defaults from library-go.
Happy to move EffectiveSignerPKIConfig() and related tests to PR 3 though.
There was a problem hiding this comment.
Done, moved EffectiveSignerPKIConfig() to PR 3.
Add the configurable PKI API surface to InstallConfig behind the ConfigurablePKI feature gate. When the gate is active, the installer generates a config.openshift.io/v1alpha1 PKI custom resource manifest that day-2 operators use for certificate rotation parameters. The default PKI profile uses RSA-4096 until all day-2 operators (CKAO, CKMO, etc.) support ECDSA rotation. When pki is not specified in install-config the PKI CR uses mode: Default. When pki is specified the PKI CR uses mode: Custom with DefaultPKIProfile as the base and user signerCertificates overrides layered on top. No certificate generation changes are included — all certs remain RSA-2048. Non-TechPreview clusters are completely unaffected. Assisted-by: Claude Code (Opus 4.6)
hasbro17
force-pushed
the
pki-1-types-and-manifest
branch
from
6f6a58a to
3d195a4
Compare
|
@hasbro17: The following tests failed, say
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
Part 1 of splitting #10396 into smaller, independently reviewable PRs.
This PR adds the configurable PKI API surface behind the
ConfigurablePKIfeature gate:PKIConfig,CertificateConfig,KeyConfigand related types onInstallConfig.PKIValidatePKIConfig()validates algorithm, key size, and curve parameters (FIPS-aware)ConfigurablePKIregistered as a TechPreview gateinstall-config.yamlschema updated withpkifieldconfig.openshift.io/v1alpha1 PKIcustom resource (cluster-pki-02-config.yaml) for day-2 operator certificate rotationDefaultPKIProfile()uses RSA-4096 until day-2 operators (CKAO, CKMO) support ECDSA rotationNo certificate generation changes — all certs remain RSA-2048. Non-TechPreview clusters are completely unaffected.
PR chain
Summary by CodeRabbit
New Features
Tests