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 1 commit 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. |
|
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:
📝 WalkthroughWalkthroughAdds 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.
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. |
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
9d66374 to
fcfc32e
Compare
|
/test e2e-aws-ovn-pki-default-techpreview |
d217ac0 to
c665423
Compare
c665423 to
e4030fd
Compare
|
/test e2e-aws-ovn-pki-default-techpreview |
e4030fd to
de6f285
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-aws-ovn-techpreview |
|
/test verify-deps |
|
/test e2e-aws-ovn-techpreview |
|
Scheduling required tests: Scheduling tests matching the |
|
@tthvo, |
|
@tthvo, |
|
/test e2e-aws-ovn-techpreview e2e-azure-ovn-techpreview |
|
/test ? |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
Note: running the ABI SNO jobs because it uses ZTP manifests (no install-config) and iso-no-registry (ABI OVE) as it generates a self-signed cert for the IRI registry |
andfasano
left a comment
andfasano
left a comment
There was a problem hiding this comment.
In general the approach sounds fine to me: refactoring the graph by introducing a new technical asset SignerKeyParams, still built from the install-config.yaml, to avoid introducing unwanted dependencies (and with zero deps) has been adopted also in other cases (ie InfraEnvFile).
I have just a minor point about the usage of the feature gate check, otherwise it looks good to me (waiting also for the results of two additional ABI jobs)
| return nil | ||
| } | ||
|
|
||
| if ic.PKI != nil { |
There was a problem hiding this comment.
There's a comment by CodeRabbit on that. Even though the method comments mention that install-config validation rejects pki when the gate is off, I personally found a little bit confusing having defined a specific asset for PKI (essentially to break the dependency on InstallConfig), but assuming an elsewhere behavior. From a design point of view, it looks like an implicit strong coupling.
For this reason I'd suggest to simply move the feature gate check on top of the method (well, after the nil check), ie:
if !ic.Enabled(features.FeatureGateConfigurablePKI) {
return nil
}
This will make this asset pretty independent (it's ok to keep the validation in IC, because this is a synthetic asset designed for internal reasons, not known by the user via an explicit external artefact)
There was a problem hiding this comment.
Good point, yeah I think this is kind of implicit right now but probably clearer to check the feature gate here as well.
I'll add the condition for if ConfigurablePKI feature gate is disabled returns nil (RSA-2048 legacy path).
Introduce SignerKeyParams, a WritableAsset with zero dependencies that reads install-config.yaml via AssetBase.LoadFromFile (strict YAML, deprecated field conversion, defaults) 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)
439394f to
0d8bb38
Compare
|
/test e2e-agent-compact-ipv4-iso-no-registry |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Approved based on the reviews 👍 Thanks! I hope to see more tests passing and verification detailed that this works and doesn't introduce regressions before merging 👀
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[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-azure-nat-gateway-single-zone |
|
/test e2e-aws-ovn-pki-rsa-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. |
| // Load reads install-config.yaml through the standard LoadFromFile pipeline | ||
| // (strict YAML, deprecated field conversion, defaults) and extracts the | ||
| // effective PKI config. Returns (false, nil) when the file is missing, | ||
| // allowing the asset store to fall back to the state file between |
There was a problem hiding this comment.
Is this asset (indirectly) required by something in the create manifests stanza? If not then it won't be generated and stored in the asset store (and therefore wouldn't be available from anywhere in time for the next stanza).
Ditto for agent create cluster-manifests.
Since it is (for some reason) a WritableAsset that nevertheless does not generate any files, perhaps adding it explicitly to the relevant targets would be advisable to ensure that it is definitely generated.
There was a problem hiding this comment.
IIUC I think your concern is about persisting SignerKeyParams across stanzas? (Please clarify if that's not the case though).
So yes it's indirectly required. SignerKeyParams is required by the 6 signers, so it's resolved whenever those signers are resolved. That includes create manifests and create cluster.
agent create cluster-manifests doesn't create any signers so it doesn't resolve SignerKeyParams. (agent image depends on AdminKubeConfigSignerCertKey so that does resolve it).
But regarding the create manifests -> create cluster stanzas:
SignerKeyParams doesn't need to persist across stanzas since the signer certs themselves are saved to the state file after create manifests and loaded from state in create cluster without regeneration.
The signer certs are the WritableAssets that get persisted.
SignerKeyParams itself is just an intermediate for passing config to generate signers and not a persistent artifact between stanzas.
|
This seems like a good solution to avoid breaking anything. Since |
|
Filed CNTRLPLANE-3856 to track the day-0 gap and document future options for full consistency in the OVE flow. |
6 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
6 of these signers previously had zero dependencies — adding
InstallConfigas a dependency would 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 would also pull in standardInstallConfigvalidation, rejecting configs that are valid under the agent flow's more lenientOptionalInstallConfigrules (e.g. vSphere without credentials).Solution
We introduce
SignerKeyParams— aWritableAssetwith zero dependencies that readsinstall-config.yamlviaAssetBase.LoadFromFile(strict YAML, deprecated field conversion, defaults) 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:AdminKubeConfigSignerCertKeyKubeAPIServerLocalhostSignerCertKeyKubeAPIServerServiceNetworkSignerCertKeyKubeAPIServerLBSignerCertKeyRootCAKubeletBootstrapCertSigner5 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