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 |
|
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 feature-gated ChangesConfigurable PKI and TLS generation
Sequence Diagram(s)PKI manifest generation sequenceDiagram
participant Manifests
participant PKIConfiguration
participant InstallConfig
participant EffectiveSignerPKIConfig
participant DefaultPKIProfile
Manifests->>PKIConfiguration: Generate(...)
PKIConfiguration->>InstallConfig: load installconfig.InstallConfig
PKIConfiguration->>EffectiveSignerPKIConfig: resolve signer PKIConfig
EffectiveSignerPKIConfig->>DefaultPKIProfile: derive default signer values
Manifests->>PKIConfiguration: Files()
Signer certificate generation sequenceDiagram
participant RootCA
participant SignerKeyParams
participant SelfSignedCertKey
participant GenerateSelfSignedCertificate
participant PrivateKeyToPem
RootCA->>SignerKeyParams: read PKIConfig
RootCA->>SelfSignedCertKey: Generate(..., PKIConfig)
SelfSignedCertKey->>GenerateSelfSignedCertificate: create key and certificate
SelfSignedCertKey->>PrivateKeyToPem: encode generated key
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. |
hasbro17
force-pushed
the
pki-3-signer-wiring
branch
2 times, most recently
from
0a3fbe6 to
9d66374
Compare
|
/test e2e-aws-ovn-pki-default-techpreview |
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/asset/manifests/pki.go`:
- Around line 42-48: The Generate method on PKIConfiguration exits early when
the FeatureGateConfigurablePKI is disabled but doesn't clear previously
generated state; ensure you reset p.FileList (e.g., set to nil or an empty
slice) before the early return in PKIConfiguration.Generate so stale PKI
manifests aren't retained across runs when the feature gate is off.
🪄 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: 2313bdbb-b13e-49c2-89f3-c0d815a13343
⛔ Files ignored due to path filters (1)
pkg/types/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (40)
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/conversion_test.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
✅ Files skipped from review due to trivial changes (5)
- pkg/asset/ignition/machine/arbiter_test.go
- pkg/explain/printer_test.go
- pkg/types/defaults/installconfig.go
- pkg/asset/manifests/pki_test.go
- pkg/types/validation/featuregates.go
🚧 Files skipped from review as they are similar to previous changes (30)
- pkg/asset/imagebased/configimage/ingressoperatorsigner.go
- pkg/asset/tls/ironictls.go
- pkg/asset/tls/utils_test.go
- pkg/asset/ignition/machine/worker_test.go
- pkg/asset/ignition/machine/master_test.go
- pkg/types/validation/featuregate_test.go
- pkg/asset/ignition/machine/arbiter_ignition_customizations_test.go
- pkg/types/validation/installconfig_test.go
- pkg/types/validation/installconfig.go
- pkg/types/pki/validation.go
- pkg/types/pki/defaults.go
- pkg/types/pki/defaults_test.go
- pkg/asset/tls/boundsasigningkey.go
- pkg/types/installconfig.go
- data/data/install.openshift.io_installconfigs.yaml
- pkg/asset/tls/keypair.go
- pkg/asset/tls/kubecontrolplane.go
- pkg/asset/tls/apiserver.go
- pkg/asset/tls/certkey.go
- pkg/asset/tls/tls_test.go
- pkg/asset/tls/adminkubeconfig.go
- pkg/asset/tls/signerkey_params.go
- pkg/asset/tls/kubelet.go
- pkg/asset/tls/aggregator.go
- pkg/asset/ignition/machine/worker_ignition_customizations_test.go
- pkg/asset/tls/utils.go
- pkg/asset/tls/root.go
- pkg/asset/tls/certkey_test.go
- pkg/types/pki/validation_test.go
- pkg/asset/tls/tls.go
| func (p *PKIConfiguration) Generate(_ context.Context, dependencies asset.Parents) error { | ||
| installConfig := &installconfig.InstallConfig{} | ||
| dependencies.Get(installConfig) | ||
|
|
||
| if !installConfig.Config.Enabled(features.FeatureGateConfigurablePKI) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Reset generated state before the feature-gate early return.
On Line 47, Generate exits when the gate is disabled but does not clear p.FileList. Reusing the same asset instance can retain stale PKI manifest output from a previous enabled run.
Suggested fix
func (p *PKIConfiguration) Generate(_ context.Context, dependencies asset.Parents) error {
+ p.FileList = nil
+
installConfig := &installconfig.InstallConfig{}
dependencies.Get(installConfig)
if !installConfig.Config.Enabled(features.FeatureGateConfigurablePKI) {
return nil
}📝 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 (p *PKIConfiguration) Generate(_ context.Context, dependencies asset.Parents) error { | |
| installConfig := &installconfig.InstallConfig{} | |
| dependencies.Get(installConfig) | |
| if !installConfig.Config.Enabled(features.FeatureGateConfigurablePKI) { | |
| return nil | |
| } | |
| func (p *PKIConfiguration) Generate(_ context.Context, dependencies asset.Parents) error { | |
| p.FileList = nil | |
| installConfig := &installconfig.InstallConfig{} | |
| dependencies.Get(installConfig) | |
| if !installConfig.Config.Enabled(features.FeatureGateConfigurablePKI) { | |
| return nil | |
| } |
🤖 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.go` around lines 42 - 48, The Generate method on
PKIConfiguration exits early when the FeatureGateConfigurablePKI is disabled but
doesn't clear previously generated state; ensure you reset p.FileList (e.g., set
to nil or an empty slice) before the early return in PKIConfiguration.Generate
so stale PKI manifests aren't retained across runs when the feature gate is off.
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-3-signer-wiring
branch
from
9d66374 to
fcfc32e
Compare
|
/test e2e-aws-ovn-pki-default-techpreview |
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
fcfc32e to
9656851
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