CNTRLPLANE-2012: Wire signer certs to read PKI config via SignerKeyParams#10595
CNTRLPLANE-2012: Wire signer certs to read PKI config via SignerKeyParams#10595hasbro17 wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
|
[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 |
|
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 (39)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (30)
WalkthroughThis PR implements configurable PKI support in the OpenShift installer, enabling users to specify RSA or ECDSA key parameters for signer certificates. The change extends the CRD schema, introduces type-safe configuration structures, generalizes TLS cryptographic infrastructure to support multiple key algorithms, integrates PKI configuration through the asset system, and generates PKI cluster resource manifests. ChangesConfigurable PKI Feature Implementation
🎯 5 (Critical) | ⏱️ ~120 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)
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 |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test e2e-aws-ovn-pki-default-techpreview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/types/pki/defaults.go`:
- Around line 36-53: EffectiveSignerPKIConfig currently returns ic.PKI before
checking the ConfigurablePKI feature gate, allowing user PKI to be honored even
when the gate is off; change the logic in EffectiveSignerPKIConfig so it first
checks ic.Enabled(features.FeatureGateConfigurablePKI) and if the gate is
disabled return nil, and only when the gate is enabled honor ic.PKI (return it)
or fall back to the default SignerCertificates config; update references in
EffectiveSignerPKIConfig to enforce the gate-first behavior.
🪄 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: b8a25289-d43e-4637-8509-4498d6ba55cf
⛔ Files ignored due to path filters (1)
pkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (39)
data/data/install.openshift.io_installconfigs.yamlpkg/asset/ignition/machine/arbiter_ignition_customizations_test.gopkg/asset/ignition/machine/arbiter_test.gopkg/asset/ignition/machine/master_ignition_customizations_test.gopkg/asset/ignition/machine/master_test.gopkg/asset/ignition/machine/worker_ignition_customizations_test.gopkg/asset/ignition/machine/worker_test.gopkg/asset/imagebased/configimage/ingressoperatorsigner.gopkg/asset/manifests/operators.gopkg/asset/manifests/pki.gopkg/asset/manifests/pki_test.gopkg/asset/tls/adminkubeconfig.gopkg/asset/tls/aggregator.gopkg/asset/tls/apiserver.gopkg/asset/tls/boundsasigningkey.gopkg/asset/tls/certkey.gopkg/asset/tls/certkey_test.gopkg/asset/tls/ironictls.gopkg/asset/tls/keypair.gopkg/asset/tls/kubecontrolplane.gopkg/asset/tls/kubelet.gopkg/asset/tls/root.gopkg/asset/tls/signerkey_params.gopkg/asset/tls/tls.gopkg/asset/tls/tls_test.gopkg/asset/tls/utils.gopkg/asset/tls/utils_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
| func EffectiveSignerPKIConfig(ic *types.InstallConfig) *types.PKIConfig { | ||
| if ic.PKI != nil { | ||
| return ic.PKI | ||
| } | ||
|
|
||
| if ic.Enabled(features.FeatureGateConfigurablePKI) { | ||
| // Default signer config matches DefaultPKIProfile().SignerCertificates | ||
| return &types.PKIConfig{ | ||
| SignerCertificates: types.CertificateConfig{ | ||
| Key: types.KeyConfig{ | ||
| Algorithm: types.KeyAlgorithmRSA, | ||
| RSA: types.RSAKeyConfig{KeySize: 4096}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Honor ConfigurablePKI before applying user PKI config.
Line 37 returns ic.PKI even when the feature gate is off. pkg/asset/tls/signerkey_params.go:64-81 calls this helper on a raw install-config.yaml without running install-config validation, so zero-dependency signer assets can still generate custom signer keys from a gated field. Return nil when ConfigurablePKI is disabled and only honor ic.PKI once the gate is enabled.
Suggested fix
func EffectiveSignerPKIConfig(ic *types.InstallConfig) *types.PKIConfig {
- if ic.PKI != nil {
- return ic.PKI
- }
-
- if ic.Enabled(features.FeatureGateConfigurablePKI) {
- // Default signer config matches DefaultPKIProfile().SignerCertificates
- return &types.PKIConfig{
- SignerCertificates: types.CertificateConfig{
- Key: types.KeyConfig{
- Algorithm: types.KeyAlgorithmRSA,
- RSA: types.RSAKeyConfig{KeySize: 4096},
- },
- },
- }
- }
-
- return nil
+ if ic == nil || !ic.Enabled(features.FeatureGateConfigurablePKI) {
+ return nil
+ }
+
+ if ic.PKI != nil {
+ return ic.PKI
+ }
+
+ // Default signer config matches DefaultPKIProfile().SignerCertificates
+ return &types.PKIConfig{
+ SignerCertificates: types.CertificateConfig{
+ Key: types.KeyConfig{
+ Algorithm: types.KeyAlgorithmRSA,
+ RSA: types.RSAKeyConfig{KeySize: 4096},
+ },
+ },
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func EffectiveSignerPKIConfig(ic *types.InstallConfig) *types.PKIConfig { | |
| if ic.PKI != nil { | |
| return ic.PKI | |
| } | |
| if ic.Enabled(features.FeatureGateConfigurablePKI) { | |
| // Default signer config matches DefaultPKIProfile().SignerCertificates | |
| return &types.PKIConfig{ | |
| SignerCertificates: types.CertificateConfig{ | |
| Key: types.KeyConfig{ | |
| Algorithm: types.KeyAlgorithmRSA, | |
| RSA: types.RSAKeyConfig{KeySize: 4096}, | |
| }, | |
| }, | |
| } | |
| } | |
| return nil | |
| func EffectiveSignerPKIConfig(ic *types.InstallConfig) *types.PKIConfig { | |
| if ic == nil || !ic.Enabled(features.FeatureGateConfigurablePKI) { | |
| return nil | |
| } | |
| if ic.PKI != nil { | |
| return ic.PKI | |
| } | |
| // Default signer config matches DefaultPKIProfile().SignerCertificates | |
| return &types.PKIConfig{ | |
| SignerCertificates: types.CertificateConfig{ | |
| Key: types.KeyConfig{ | |
| Algorithm: types.KeyAlgorithmRSA, | |
| RSA: types.RSAKeyConfig{KeySize: 4096}, | |
| }, | |
| }, | |
| } | |
| } |
🤖 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/defaults.go` around lines 36 - 53, EffectiveSignerPKIConfig
currently returns ic.PKI before checking the ConfigurablePKI feature gate,
allowing user PKI to be honored even when the gate is off; change the logic in
EffectiveSignerPKIConfig so it first checks
ic.Enabled(features.FeatureGateConfigurablePKI) and if the gate is disabled
return nil, and only when the gate is enabled honor ic.PKI (return it) or fall
back to the default SignerCertificates config; update references in
EffectiveSignerPKIConfig to enforce the gate-first behavior.
hasbro17
force-pushed
the
pki-3-signer-wiring
branch
from
0e262ca to
2ab8c63
Compare
|
/test e2e-aws-ovn-pki-default-techpreview |
|
Okay the e2e tests pass now: So the SignerKeyParams helps walk the line between IPI and ABI for those signers. |
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)
Refactor pkg/asset/tls/ to support generating signer certificates with configurable key algorithms (RSA or ECDSA). PrivateKeyToPem now returns ([]byte, error) instead of calling logrus.Fatalf, and GenerateSelfSignedCertificate accepts PrivateKeyParams to control key generation. KeyUsage flags are set based on the algorithm since ECDSA keys cannot perform key encipherment. All signer certs pass nil for pkiConfig in this commit, preserving the existing RSA-2048 behavior. Wiring signers to read PKI config is deferred to a follow-up to avoid breaking codepaths that generate signer certs without an install-config on disk (e.g. agent create certificates, node-joiner add-nodes). Assisted-by: Claude Code (Opus 4.6)
Introduce SignerKeyParams, a WritableAsset with zero dependencies that reads install-config.yaml directly from disk and extracts the effective PKI configuration via EffectiveSignerPKIConfig(). When no install-config is present, Load() returns (false, nil) so the asset store falls back to the state file between multi-step invocations (e.g. create manifests followed by create cluster, where install-config.yaml is consumed after the first step). In the agent flow where neither file nor state exists, Generate() leaves PKIConfig nil (RSA-2048). Signers that previously had zero dependencies now depend on SignerKeyParams instead of InstallConfig, avoiding validation that would reject configs valid in the agent flow. Signers that already depended on InstallConfig use EffectiveSignerPKIConfig() to resolve the effective PKI config including feature gate defaults. With TechPreview enabled and no explicit pki config, all signer certs are generated with RSA-4096 (the current DefaultPKIProfile default). Assisted-by: Claude Code (Opus 4.6)
hasbro17
force-pushed
the
pki-3-signer-wiring
branch
from
2ab8c63 to
0a3fbe6
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. |
2 participants
Summary
Part 3 of splitting #10396 into smaller PRs. Depends on #10593 and #10594.
Wires all 11 signer certificates to read PKI configuration for configurable key algorithms
Problem
The challenge is that 6 of these signers previously had zero dependencies — adding
InstallConfigas a dependencywould break agent-based installer (ABI) codepaths that generate signer certs without an install-config on disk (e.g.
agent create certificatesused byset-node-zero.sh,node-joiner add-nodes). It wouldalso pull in standard
InstallConfigvalidation, rejecting configs that are valid under the agent flow's more lenientOptionalInstallConfigrules (e.g. vSphere without credentials).Solution
To solve this, we introduce
SignerKeyParams— aWritableAssetwith zero dependencies that readsinstall-config.yamldirectly from disk, extracts only the PKI and FeatureSet fields, and resolves theeffective PKI config via
EffectiveSignerPKIConfig(). When no install-config is found,Load()returns(false, nil)so the asset store falls back to the state file between multi-step invocations (e.g.create manifestsfollowed bycreate cluster, whereinstall-config.yamlis consumed after the first step). In the agent flow where neither file nor state exists,Generate()leaves PKIConfig nil(RSA-2048).
6 previously zero-dep signers — now depend on
SignerKeyParams:AdminKubeConfigSignerCertKeyKubeAPIServerLocalhostSignerCertKeyKubeAPIServerLBSignerCertKeyRootCAKubeletBootstrapCertSigner5 signers that already depended on InstallConfig — now use
EffectiveSignerPKIConfig()to resolve the effective PKI config including feature gate defaults:KubeAPIServerToKubeletSignerCertKeyAggregatorCAAggregatorSignerCertKeyKubeControlPlaneSignerCertKeyKubeletCSRSignerCertKeyWith TechPreview enabled and no explicit
pkiconfig, all signer certs are generated with RSA-4096 (the currentDefaultPKIProfiledefault). Without TechPreview, all certs remain RSA-2048.PR chain
Summary by CodeRabbit
New Features
Improvements
Tests