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
|
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:
WalkthroughAdds a ChangesConfigurablePKI Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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.
hasbro17
force-pushed
the
pki-1-types-and-manifest
branch
from
6f6a58a to
3d195a4
Compare
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
Overall, looks good to me! Thank you 👍
I just have some comments; and golint/gofmt warnings need fixing :D
| // +union | ||
| // | ||
| //nolint:godot |
There was a problem hiding this comment.
I think you can move the // +union up to avoid skipping lint here.
// +union
// +kubebuilder:validation:XValidation:rule="...",message="..."
// +kubebuilder:validation:XValidation:rule="...",message="..."There was a problem hiding this comment.
Oh yeah that works. Will move it up.
| // We maintain local copies so that new fields added upstream do not silently | ||
| // appear in the install-config YAML API without explicit opt-in and validation | ||
| // by the installer. Conversion to the upstream type happens at the manifest | ||
| // boundary (see pkg/types/pki/conversion.go). |
There was a problem hiding this comment.
| // boundary (see pkg/types/pki/conversion.go). | |
| // We maintain local copies so that new o/api fields added do not silently | |
| // appear in the install-config YAML API without explicit opt-in and validation | |
| // by the installer. Conversion to the o/api type happens at the manifest |
nit: In the context of installer, the word "upstream" is usually understood as upstream CAPI provider. Let's explicitly say openshift/api or o/api in short.
| // Required when algorithm is ECDSA, and forbidden otherwise. | ||
| // +optional | ||
| // +unionMember | ||
| ECDSA ECDSAKeyConfig `json:"ecdsa,omitzero"` |
There was a problem hiding this comment.
Since RSA and ECDSA are mutually exclusive union members, I think using pointers makes presence explicit: the installer can distinguish "not specified" from "zero value" without relying on field-level checks. Note that CEL markers are not used in the installer.
Do you envision more configurable fields being added for each algorithm in the future? If so, pointers should be future-proof. WDTY?
There was a problem hiding this comment.
Maybe @sanchezl can chime in on if this was raised during the upstream API review
openshift/api#2645
But just to address your points:
I think using pointers makes presence explicit: the installer can distinguish "not specified" from "zero value" without relying on field-level checks
The installer types are intentionally a local copy subset of the upstream types to keep them aligned. Adding pointers here would mean our conversion functions (CertificateConfigToUpstream) need pointer dereferencing, and we'd drift from the upstream API's design decisions.
Also on omitzero: The only scenario where value types would be a problem is if zero were a valid value for one of these fields but it isn't for either key size or curve name.
Note that CEL markers are not used in the installer
That's a fair point. But the validation in ValidateKeyConfig() already handles the mutual exclusivity check using zero value detection (which works because again zero is never a valid value for either field).
Pointers don't gain us much here imo, since zero is an explicit omission in this case.
Do you envision more configurable fields being added for each algorithm in the future?
These structs are unlikely to grow (RSA key generation needs a key size, ECDSA needs a curve) and there aren't obvious additional parameters on the horizon (@sanchezl to keep me honest).
Even if fields are added upstream we'd still have to update validation downstream to match regardless so not like it saves us too much work.
Keeping value types avoids nil-check ceremony and keeps conversion to/from upstream straightforward.
So I would lean towards keeping the types aligned with upstream even in the absence of CEL markers, unless the installer team feels strongly about this.
There was a problem hiding this comment.
All structs within the union MUST be pointers
https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md
It seems like there may be a misunderstanding. claude couldn't find any discussion of pointers in the api pr. cc @everettraven
There was a problem hiding this comment.
@everettraven @sanchezl I had a little more time to review this and api guidelines and git metadata are pretty clear: openshift/api#2645 should be updated to make these structs pointers
@hasbro17 it is much simpler and future proof to just check that the struct is nil rather than check all the struct's fields, which can grow over time
There was a problem hiding this comment.
The conventions have spoken, I must comply 🙂
@patrickdillon Fair enough, and thanks for pointing out the API conventions. I'll update my copy here for now and follow up with @sanchezl to see if we can get o/api updated as well.
There was a problem hiding this comment.
Update to pointer types
| } | ||
| } | ||
|
|
||
| if c.PKI != nil && c.Enabled(features.FeatureGateConfigurablePKI) { |
There was a problem hiding this comment.
| if c.PKI != nil && c.Enabled(features.FeatureGateConfigurablePKI) { | |
| if c.PKI != nil |
We can skip the feature gate check since it's already checked in pkg/types/validation/featuregates.go
There was a problem hiding this comment.
Yeah this isn't strictly for validation. I think this came from a code rabbit review comment earlier about adding this to reduce the error noise:
#10593 (comment)
On a non-TechPreview install, pki: {} will report both nested PKI shape errors and the later “field is protected by ConfigurablePKI” error, which makes the gated-field failure noisy and misleading
But I can remove this if you feel otherwise.
There was a problem hiding this comment.
Thanks for updating!
On a non-TechPreview install, pki: {} will report both nested PKI shape errors and the later “field is protected by ConfigurablePKI” error,
On this note, the 2 errors are expected (not noisy at all). It informs the user: to use pki field, they must enable the feature and provide a field pki configurations 👍
| // ValidatePKIConfig validates the PKI configuration. | ||
| // When pkiConfig is non-nil, signerCertificates must be fully specified. | ||
| // NOTE: All fields are value types (not pointers). Use zero-value checks. | ||
| func ValidatePKIConfig(pkiConfig *types.PKIConfig, fldPath *field.Path, fips bool) field.ErrorList { |
There was a problem hiding this comment.
fips is being passed, but unused. Does FIPS have some additional PKI restrictions to be compliant?
There was a problem hiding this comment.
Good catch, I think I added this expecting more restrictions, but current validation rules (RSA 2048-8192, ECDSA P-256/P-384/P-521) are already FIPS compliant.
Removed the unused FIPS parameter from all validation functions and tests.
We can add FIPS specific restrictions if that changes in the future.
| }, | ||
| }, | ||
| }, | ||
| expectError: false, |
There was a problem hiding this comment.
nit: highly recommend to check the error message so that we know the right "message" is shown to the user.
|
|
||
| // CertificateConfigToUpstream converts the installer-local CertificateConfig | ||
| // to the upstream configv1alpha1.CertificateConfig for use in the PKI CR manifest. | ||
| func CertificateConfigToUpstream(local types.CertificateConfig) configv1alpha1.CertificateConfig { |
There was a problem hiding this comment.
| func CertificateConfigToUpstream(local types.CertificateConfig) configv1alpha1.CertificateConfig { | |
| func ConvertToAPICertConfig(certConf types.CertificateConfig) configv1alpha1.CertificateConfig { |
nit: like the other comment, we use "upstream" to mean CAPI provider here. Let's try this, WDYT?
| 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.
This is another reason to use pointer so that we only populate the field if the corresponding RSA/ECDSA is non-nil.
There was a problem hiding this comment.
Same as the earlier pointer discussion.
We're mirroring the openshift/api value-type design, and conversion_test.go verifies that omitzero correctly omits the unused field in marshaled output
| // DefaultPKIProfile returns the default PKI profile for OpenShift clusters. | ||
| // Currently uses RSA-4096 until all day-2 operators (CKAO, CKMO, etc.) support | ||
| // 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 |
There was a problem hiding this comment.
I'm curious 🤔. The enhancement said:
Currently, OpenShift uses hardcoded defaults (primarily RSA 2048-bit keys)
IIUC, the default should be using RSA-2048, right? What are the reasons for this func to use RSA-4026?
There was a problem hiding this comment.
RSA-2048 is the legacy default when the ConfigurablePKI feature gate is off (non-TechPreview). DefaultPKIProfile() defines the default for when the gate is on.
We use RSA-4096 as a stronger interim default
until day-2 operators support ECDSA rotation, at which point this switches to ECDSA P-384 to match the upstream library-go profile.
What are the reasons for this func to use RSA-4026?
The whole point of configurable PKI is to move to stronger keys than the legacy RSA-2048.
So @sanchezl can correct me but we're using the PKI feature enablement as an opportunity to move to that new default as well.
There was a problem hiding this comment.
Ahh right, thanks for the details! Definitely +1 for using stronger keys. Though, when we completely switch to this new default, we need to mention it in:
There was a problem hiding this comment.
Sorry forgot to update here, I'm leaving the docs as the last follow up once the implementation is settled with all 3 PRs merged.
#10396 had it all in one PR but that was hard to review.
| } else { | ||
| assert.Equal(t, tc.expectDefaultsRSA, profile.Defaults.Key.RSA.KeySize) |
There was a problem hiding this comment.
nit: we can use an if tc.expectSignerAlgo == configv1alpha1.KeyAlgorithmRSA here instead.
hasbro17
force-pushed
the
pki-1-types-and-manifest
branch
from
3d195a4 to
7b88840
Compare
|
|
||
| configData, err := yaml.Marshal(config) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to marshal PKI config") |
There was a problem hiding this comment.
please use fmt.Errorf
There was a problem hiding this comment.
Let's use fmt.Errorf like Patrick said :D
There was a problem hiding this comment.
Whoops, forgot this. Will update.
| } | ||
|
|
||
| if c.AdditionalTrustBundlePolicy == "" { | ||
| c.AdditionalTrustBundlePolicy = types.PolicyProxyOnly | ||
| } | ||
|
|
| // Required when algorithm is ECDSA, and forbidden otherwise. | ||
| // +optional | ||
| // +unionMember | ||
| ECDSA ECDSAKeyConfig `json:"ecdsa,omitzero"` |
There was a problem hiding this comment.
All structs within the union MUST be pointers
https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md
It seems like there may be a misunderstanding. claude couldn't find any discussion of pointers in the api pr. cc @everettraven
| return allErrs | ||
| } | ||
|
|
||
| if config.Algorithm != types.KeyAlgorithmRSA && config.Algorithm != types.KeyAlgorithmECDSA { |
There was a problem hiding this comment.
nit: To ease future maintenance, let's also address #10593 (comment), WDYT? On line 91, you already do the same :D
|
|
||
| // CertificateConfigToUpstream converts the installer CertificateConfig | ||
| // to the openshift/api configv1alpha1.CertificateConfig for use in the PKI CR manifest. | ||
| func CertificateConfigToUpstream(certConf types.CertificateConfig) configv1alpha1.CertificateConfig { |
There was a problem hiding this comment.
Sorry if I wasn't clear. We should also rename this func too to not mention "upstream", for example, ConvertToAPICertConfig (or any ya like :D).
Additionally, you can move this func into pkg/asset/manifests/pki.go as we only need to call it once to generate the manifests. This way, tests in pki_test.go will also cover it. Why? conversion.go in pkg/types usually use for converting deprecated install-config fields to new ones; thus, it also avoid potential confusion.
hasbro17
force-pushed
the
pki-1-types-and-manifest
branch
from
7b88840 to
8160e36
Compare
|
Per the discussion in #10593 (comment) I've changed the union members Added a local |
|
|
||
| configData, err := yaml.Marshal(config) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to marshal PKI config") |
There was a problem hiding this comment.
Let's use fmt.Errorf like Patrick said :D
|
|
||
| validCurves := sets.New(types.ECDSACurveP256, types.ECDSACurveP384, types.ECDSACurveP521) | ||
| if !validCurves.Has(config.Curve) { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("curve"), config.Curve, |
There was a problem hiding this comment.
We should use field.UnSupported like line 37, right?
| validAlgorithms := sets.New(types.KeyAlgorithmRSA, types.KeyAlgorithmECDSA) | ||
| if !validAlgorithms.Has(config.Algorithm) { | ||
| allErrs = append(allErrs, field.NotSupported(fldPath.Child("algorithm"), | ||
| config.Algorithm, []string{string(types.KeyAlgorithmRSA), string(types.KeyAlgorithmECDSA)})) | ||
| return allErrs | ||
| } |
There was a problem hiding this comment.
I think we can simplify it like:
validAlgorithms := []types.KeyAlgorithm{types.KeyAlgorithmRSA, types.KeyAlgorithmECDSA}
if !sets.New(validAlgorithms...).Has(config.Algorithm) {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("algorithm"),
config.Algorithm, validAlgorithms))
return allErrs
}There was a problem hiding this comment.
Yeah that's much simpler. Will update.
| errs := ValidateKeyConfig(tc.config, fldPath) | ||
| if tc.expectError { | ||
| if assert.Len(t, errs, tc.errorCount, "expected %d errors, got: %v", tc.errorCount, errs) { | ||
| assert.Contains(t, errs[0].Detail, tc.errorMsg) |
There was a problem hiding this comment.
The error may not always be the first one so let's try:
assert.Regexp(t, tc.errorMsg, errs.ToAggregate())This is consistent with other test files.
| errs := ValidatePKIConfig(tc.pkiConfig, fldPath) | ||
| if tc.expectError { | ||
| if assert.Len(t, errs, tc.errorCount) { | ||
| assert.Contains(t, errs[0].Detail, tc.errorMsg) |
There was a problem hiding this comment.
The error may not always be the first one so let's try:
assert.Regexp(t, tc.errorMsg, errs.ToAggregate())
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
8160e36 to
85252ce
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tthvo 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
|
/test e2e-aws-ovn-techpreview |
|
/test e2e-aws-ovn-dualstack-ipv6-primary-techpreview e2e-aws-ovn-dualstack-ipv4-primary-techpreview |
|
/test e2e-azure-ovn-techpreview |
|
@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. |
5 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
Release Notes
New Features
Tests