CNTRLPLANE-2847: Support PKI customization#1593
Open
rh-roman wants to merge 2 commits into
Open
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughFeature-gated configurable PKI was added: install-config PKI extraction, a pkiProfile provider threaded through render functions, and PKIProfileProvider plumbing into the etcd cert signer controller and certificate creation. go.mod updated dependency versions for openshift/api and library-go. Changes
Sequence DiagramssequenceDiagram
participant Renderer as Render (newTemplateData)
participant InstallConfig as Install Config
participant PKIExtractor as getPKIProfileProvider
participant Bootstrap as createBootstrapCertSecrets
participant StaticProvider as NewStaticPKIProfileProvider
participant CertCreator as EtcdCertSignerController / Cert Configs
Renderer->>InstallConfig: check FeatureGateConfigurablePKI
alt enabled
Renderer->>PKIExtractor: extract PKI profile from install-config
PKIExtractor-->>Renderer: pkiProfileProvider (static)
else disabled
Renderer->>Renderer: pkiProfileProvider = nil
end
Renderer->>Bootstrap: call createBootstrapCertSecrets(..., pkiProfileProvider)
alt pkiProfileProvider non-nil
Bootstrap->>StaticProvider: wrap profile into provider
StaticProvider-->>CertCreator: supply PKIProfileProvider for cert configs
else nil
Bootstrap->>CertCreator: create cert configs without static provider
end
CertCreator-->>Bootstrap: secrets & configmaps
sequenceDiagram
participant Starter as Operator Starter
participant FeatureGate as FeatureGate sync
participant StarterLogic as starter.RunOperator
participant ControllerCtor as NewEtcdCertSignerController
participant ConfigInformer as PKI Config Informer
participant Controller as EtcdCertSignerController
Starter->>FeatureGate: ensure initial feature gates
Starter->>FeatureGate: re-read current gates
FeatureGate-->>StarterLogic: FeatureGateConfigurablePKI enabled?
alt enabled
StarterLogic->>ConfigInformer: obtain PKIs lister/informer
StarterLogic->>ControllerCtor: pass cluster PKI provider, additional informers
else disabled
StarterLogic->>ControllerCtor: pass nil provider, no additional informers
end
ControllerCtor-->>Controller: Controller constructed with pkiProfileProvider
Controller->>Controller: build cert configs with PKIProfileProvider metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
rh-roman
force-pushed
the
pki-config-with-render
branch
from
3291755 to
bb43ee0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 163: Remove the personal fork replace directive in go.mod that maps
github.com/openshift/library-go to github.com/sanchezl/library-go (the replace
line shown in the diff); either revert that replace to the upstream
github.com/openshift/library-go at the intended commit or remove the replace
entirely and add a comment/PR note documenting and justifying the fork if it is
truly required for this change, then run `go mod tidy` to ensure module graph is
consistent (look for the replace entry in go.mod to edit and verify with go
tooling).
In `@pkg/cmd/render/render.go`:
- Around line 834-869: getPKIConfig currently only unmarshals signerCertificates
into a localPKI struct from pkiJSON, causing valid non-signer PKI sections to be
ignored and an empty algorithm to trigger an unsupported-algorithm error; fix by
unmarshalling the entire PKI profile (into the appropriate
configv1alpha1.PKIProfile or a full local struct matching all pki fields) from
pkiJSON instead of the narrow signer-only struct, then merge/override only the
provided fields onto pki.DefaultPKIProfile() (e.g., replace
profile.SignerCertificates, profile.ServingCertificates,
profile.ClientCertificates, profile.PeerCertificates, profile.Defaults when
present) and avoid treating an absent signer algorithm as an error.
In `@pkg/tlshelpers/tlshelpers.go`:
- Around line 242-248: createCertForNode currently constructs
certrotation.PeerRotation for all certificate types, causing CertificateType()
to always return peer and making ConfigurablePKI resolve serving certs against
the peer profile; update createCertForNode to choose the rotation type based on
the requested certificate: when invoked for CreatePeerCertificate keep using
certrotation.PeerRotation, but for CreateServingCertificate and
CreateMetricsServingCertificate instantiate certrotation.ServingRotation (or
branch to return ServingRotation for serving/metrics types) so CertificateType()
returns the correct serving type and PKI profile resolution works as intended;
reference the functions createCertForNode, CreatePeerCertificate,
CreateServingCertificate, CreateMetricsServingCertificate and the types
certrotation.PeerRotation / certrotation.ServingRotation when making the change.
🪄 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: Pro Plus
Run ID: e4f8108b-e324-4e18-a890-c34cab4f0a37
⛔ Files ignored due to path filters (14)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/client_cert_rotation_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/target.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/v1helpers/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/profile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/resolve.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
go.modpkg/cmd/render/certs.gopkg/cmd/render/certs_test.gopkg/cmd/render/render.gopkg/operator/etcdcertsigner/etcdcertsignercontroller.gopkg/operator/etcdcertsigner/etcdcertsignercontroller_test.gopkg/operator/starter.gopkg/tlshelpers/target_cert_creator_test.gopkg/tlshelpers/tlshelpers.go
rh-roman
force-pushed
the
pki-config-with-render
branch
from
bb43ee0 to
e456c5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/cmd/render/render.go (1)
834-869:⚠️ Potential issue | 🟠 MajorParse the full PKI profile instead of only
signerCertificates.This still drops valid
pki.defaults,pki.servingCertificates, andpki.clientCertificatesinput, and it still errors when those sections are present without an explicitsignerCertificates.key.algorithm. Bootstrap rendering will ignore part of the requested PKI profile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/render/render.go` around lines 834 - 869, Unmarshal the entire PKI profile instead of the ad-hoc localPKI so we don't drop pki.defaults, pki.servingCertificates, or pki.clientCertificates; change the json.Unmarshal target from the localPKI struct to the canonical configv1alpha1.PKIProfile (or a struct matching its JSON), then convert that unmarshaled PKI into the pki.Profile returned by this function: preserve pki.defaults, servingCertificates, and clientCertificates fields, and construct profile.SignerCertificates from the PKIProfile.SignerCertificates.Key (detect/validate algorithm and map RSA/ECDSA fields to configv1alpha1.KeyConfig as you already do), but if signerCertificates.key.algorithm is missing, do not error—use the default signer key from pki.DefaultPKIProfile() or otherwise derive a safe default before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/render/render.go`:
- Around line 823-827: When FeatureGateConfigurablePKI is enabled and
installConfig omits "pki", don't return nil; instead return the cluster default
PKI profile so bootstrap behavior matches in-cluster ListerPKIProfileProvider.
In the block where pkiData, found := installConfig["pki"] is checked, detect the
gate (FeatureGateConfigurablePKI) and return pki.DefaultPKIProfile() (or
equivalent default profile object) rather than nil; this ensures
createCertSecrets sees a non-nil pkiProfile (so it will create a
StaticPKIProfileProvider) and matches the
ListerPKIProfileProvider/DefaultPKIProfile behavior. Ensure the change
references the same symbols: FeatureGateConfigurablePKI, pki.DefaultPKIProfile,
createCertSecrets, and StaticPKIProfileProvider.
In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller.go`:
- Around line 171-179: The PKI lister-backed provider created via
pki.NewClusterPKIProfileProvider(configInformer.Config().V1alpha1().PKIs().Lister())
is used without registering its informer, so the controller may start before the
PKI cache is synced and PKI changes won't trigger reconciliation; fix this by
adding the corresponding informer
(configInformer.Config().V1alpha1().PKIs().Informer()) to the controller's
WithInformers(...) call so the factory waits for the PKI informer sync and PKI
resource events will drive reconciliations.
---
Duplicate comments:
In `@pkg/cmd/render/render.go`:
- Around line 834-869: Unmarshal the entire PKI profile instead of the ad-hoc
localPKI so we don't drop pki.defaults, pki.servingCertificates, or
pki.clientCertificates; change the json.Unmarshal target from the localPKI
struct to the canonical configv1alpha1.PKIProfile (or a struct matching its
JSON), then convert that unmarshaled PKI into the pki.Profile returned by this
function: preserve pki.defaults, servingCertificates, and clientCertificates
fields, and construct profile.SignerCertificates from the
PKIProfile.SignerCertificates.Key (detect/validate algorithm and map RSA/ECDSA
fields to configv1alpha1.KeyConfig as you already do), but if
signerCertificates.key.algorithm is missing, do not error—use the default signer
key from pki.DefaultPKIProfile() or otherwise derive a safe default before
returning.
🪄 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: Pro Plus
Run ID: 534d2019-f67f-4645-a4e7-f47d937d9821
📒 Files selected for processing (6)
pkg/cmd/render/certs.gopkg/cmd/render/certs_test.gopkg/cmd/render/render.gopkg/operator/etcdcertsigner/etcdcertsignercontroller.gopkg/operator/etcdcertsigner/etcdcertsignercontroller_test.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go
- pkg/operator/starter.go
- pkg/cmd/render/certs_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/operator/etcdcertsigner/etcdcertsignercontroller.go (1)
171-179:⚠️ Potential issue | 🟠 MajorRegister the PKI informer when you use the lister-backed provider.
NewClusterPKIProfileProvider(configInformer.Config().V1alpha1().PKIs().Lister())depends on the PKI informer cache, butWithInformers()never includesPKIs().Informer(). That makes the first sync race cache warm-up, and PKI CR changes only show up on the 1-minute resync. Please append the PKI informer conditionally in the cluster-backed path.#!/bin/bash set -euo pipefail sed -n '171,245p' pkg/operator/etcdcertsigner/etcdcertsignercontroller.go rg -n -C2 'PKIs\(\)\.(Lister|Informer)\(' pkg/operator/etcdcertsigner/etcdcertsignercontroller.goAlso applies to: 236-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller.go` around lines 171 - 179, The PKI lister-backed provider is being created via NewClusterPKIProfileProvider(configInformer.Config().V1alpha1().PKIs().Lister()) but the PKI informer (configInformer.Config().V1alpha1().PKIs().Informer()) is not being registered with WithInformers(), causing cache warm-up and missed updates; when staticPKIProvider is nil and featureGates.Enabled(features.FeatureGateConfigurablePKI) is true, add the PKI informer to the controller informers list (i.e., append configInformer.Config().V1alpha1().PKIs().Informer() in the same branch where you construct the NewClusterPKIProfileProvider) so the informer cache is started and synced for the lister-backed provider.pkg/cmd/render/render.go (2)
833-867:⚠️ Potential issue | 🟠 MajorDon't truncate install-config PKI down to
signerCertificatesonly.This helper ignores
defaults,peerCertificates,servingCertificates, andclientCertificates. Valid configs that only set those sections either fail on an empty signer algorithm or render with the wrong effective profile. Unmarshal the full PKI shape and merge the provided sections ontopki.DefaultPKIProfile()instead of rebuilding onlySignerCertificates.#!/bin/bash set -euo pipefail sed -n '833,867p' pkg/cmd/render/render.go rg -n -A40 'type PKIProfile struct' vendor/github.com/openshift/api/config/v1alpha1/types_pki.go rg -n 'Defaults|SignerCertificates|ServingCertificates|ClientCertificates|PeerCertificates' \ vendor/github.com/openshift/api/config/v1alpha1/types_pki.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/render/render.go` around lines 833 - 867, The current code only unmarshals a tiny localPKI (signerCertificates) and reconstructs profile.SignerCertificates, which drops defaults and other sections (defaults, peerCertificates, servingCertificates, clientCertificates); instead unmarshal the full PKI shape into a struct matching the complete PKI profile (use configv1alpha1.PKIProfile or a local struct with all top-level PKI fields), then start with profile := pki.DefaultPKIProfile() and merge/overwrite only the provided fields from the unmarshaled object onto profile (preserving defaults for missing fields) before returning; update usages of pkiJSON/localPKI/keyConfig to read from the merged profile and keep the same validation/error handling for unsupported algorithms.
821-825:⚠️ Potential issue | 🟠 MajorReturn
DefaultPKIProfile()whenpkiis omitted.When
FeatureGateConfigurablePKIis enabled, Line 825 returnsnil, so bootstrap rendering skips the static provider path. The in-cluster provider resolves that case topki.DefaultPKIProfile(), which can leave bootstrap certs on a different profile than the steady-state controller.Suggested fix
if !found { - // no PKI config specified, let library-go choose defaults - return nil, nil + profile := pki.DefaultPKIProfile() + return &profile, nil }#!/bin/bash set -euo pipefail sed -n '821,867p' pkg/cmd/render/render.go rg -n -C3 'DefaultPKIProfile|NewStaticPKIProfileProvider|pkiProfile != nil' \ pkg/cmd/render/certs.go \ vendor/github.com/openshift/library-go/pkg/pki/provider.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/render/render.go` around lines 821 - 825, getPKIConfig currently returns nil when installConfig["pki"] is absent which causes bootstrap to skip the static provider path; change getPKIConfig so that if the "pki" key is not found it returns the default profile (call DefaultPKIProfile() and return its value as *configv1alpha1.PKIProfile) instead of nil to match the in-cluster provider behavior; this keeps NewStaticPKIProfileProvider/pkiProfile != nil logic consistent and prevents diverging bootstrap vs steady-state PKI profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cmd/render/render.go`:
- Around line 833-867: The current code only unmarshals a tiny localPKI
(signerCertificates) and reconstructs profile.SignerCertificates, which drops
defaults and other sections (defaults, peerCertificates, servingCertificates,
clientCertificates); instead unmarshal the full PKI shape into a struct matching
the complete PKI profile (use configv1alpha1.PKIProfile or a local struct with
all top-level PKI fields), then start with profile := pki.DefaultPKIProfile()
and merge/overwrite only the provided fields from the unmarshaled object onto
profile (preserving defaults for missing fields) before returning; update usages
of pkiJSON/localPKI/keyConfig to read from the merged profile and keep the same
validation/error handling for unsupported algorithms.
- Around line 821-825: getPKIConfig currently returns nil when
installConfig["pki"] is absent which causes bootstrap to skip the static
provider path; change getPKIConfig so that if the "pki" key is not found it
returns the default profile (call DefaultPKIProfile() and return its value as
*configv1alpha1.PKIProfile) instead of nil to match the in-cluster provider
behavior; this keeps NewStaticPKIProfileProvider/pkiProfile != nil logic
consistent and prevents diverging bootstrap vs steady-state PKI profiles.
In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller.go`:
- Around line 171-179: The PKI lister-backed provider is being created via
NewClusterPKIProfileProvider(configInformer.Config().V1alpha1().PKIs().Lister())
but the PKI informer (configInformer.Config().V1alpha1().PKIs().Informer()) is
not being registered with WithInformers(), causing cache warm-up and missed
updates; when staticPKIProvider is nil and
featureGates.Enabled(features.FeatureGateConfigurablePKI) is true, add the PKI
informer to the controller informers list (i.e., append
configInformer.Config().V1alpha1().PKIs().Informer() in the same branch where
you construct the NewClusterPKIProfileProvider) so the informer cache is started
and synced for the lister-backed provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dcd118e-fa23-42e6-9291-90b8cbc23a93
📒 Files selected for processing (6)
pkg/cmd/render/certs.gopkg/cmd/render/certs_test.gopkg/cmd/render/render.gopkg/operator/etcdcertsigner/etcdcertsignercontroller.gopkg/operator/etcdcertsigner/etcdcertsignercontroller_test.gopkg/operator/starter.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/render/certs_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/starter.go
- pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go
- pkg/cmd/render/certs.go
rh-roman
force-pushed
the
pki-config-with-render
branch
from
e456c5c to
cb9bcfd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/cmd/render/render.go (2)
822-826:⚠️ Potential issue | 🟠 MajorReturn the default PKI profile when
pkiis omitted.Line 824 still returns
nil, which meanspkg/cmd/render/certs.go:31-47will skip creating a static PKI provider during bootstrap render. WithFeatureGateConfigurablePKIenabled, that makes render-time cert generation diverge from the in-cluster path, which resolves the default profile rather than “unmanaged”. Returnpki.DefaultPKIProfile()here so both paths stay aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/render/render.go` around lines 822 - 826, The current branch returns nil when "pki" is absent (pkiData not found), causing downstream code (certs.go bootstrap static PKI provider creation) to skip default profile handling and diverge under FeatureGateConfigurablePKI; update the branch that checks installConfig["pki"] (where pkiData and found are used) to return pki.DefaultPKIProfile() instead of nil so render-time cert generation uses the same default PKI profile as the in-cluster path, leaving the error/second return value as before.
833-867:⚠️ Potential issue | 🟠 MajorParse the full PKI profile, not just
signerCertificates.This helper only unmarshals
signerCertificates.key. As written, valid configs that setdefaults,servingCertificates, orclientCertificatesare ignored, and a profile that omitssignerCertificatesentirely will hit Line 856 with an empty algorithm and fail as unsupported. Unmarshal the full PKI profile and merge it ontopki.DefaultPKIProfile()instead of narrowing the install-config to signer-only fields.
🧹 Nitpick comments (1)
pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go (1)
608-625: Add at least one test that enables configurable PKI.This shared setup now hard-disables
FeatureGateConfigurablePKIand passesnilfor both provider inputs, so the suite still only covers the legacy branch inNewEtcdCertSignerController. Since this PR adds provider-resolution behavior, it would be good to add one focused test variant with the gate enabled and a static or informer-backed provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go` around lines 608 - 625, The test setup currently hard-disables FeatureGateConfigurablePKI via disabledFeatureGates and passes nil for configInformer and staticPKIProvider to NewEtcdCertSignerController; add a separate test case that enables FeatureGateConfigurablePKI (include it in enabledFeatureGates or remove it from disabledFeatureGates), construct a non-nil provider (e.g., a static PKI provider mock or an informer-backed provider) and/or a configInformer mock, and pass that provider/configInformer into NewEtcdCertSignerController so the provider-resolution branch is exercised (refer to disabledFeatureGates, featureGateAccessor, NewEtcdCertSignerController, staticPKIProvider, configInformer, FeatureGateConfigurablePKI).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cmd/render/render.go`:
- Around line 822-826: The current branch returns nil when "pki" is absent
(pkiData not found), causing downstream code (certs.go bootstrap static PKI
provider creation) to skip default profile handling and diverge under
FeatureGateConfigurablePKI; update the branch that checks installConfig["pki"]
(where pkiData and found are used) to return pki.DefaultPKIProfile() instead of
nil so render-time cert generation uses the same default PKI profile as the
in-cluster path, leaving the error/second return value as before.
---
Nitpick comments:
In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go`:
- Around line 608-625: The test setup currently hard-disables
FeatureGateConfigurablePKI via disabledFeatureGates and passes nil for
configInformer and staticPKIProvider to NewEtcdCertSignerController; add a
separate test case that enables FeatureGateConfigurablePKI (include it in
enabledFeatureGates or remove it from disabledFeatureGates), construct a non-nil
provider (e.g., a static PKI provider mock or an informer-backed provider)
and/or a configInformer mock, and pass that provider/configInformer into
NewEtcdCertSignerController so the provider-resolution branch is exercised
(refer to disabledFeatureGates, featureGateAccessor,
NewEtcdCertSignerController, staticPKIProvider, configInformer,
FeatureGateConfigurablePKI).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d24bc686-1e98-4d14-b684-f918ec80822e
⛔ Files ignored due to path filters (15)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/pkg/controller/controllercmd/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/client_cert_rotation_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/target.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/v1helpers/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/profile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/resolve.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (9)
go.modpkg/cmd/render/certs.gopkg/cmd/render/certs_test.gopkg/cmd/render/render.gopkg/operator/etcdcertsigner/etcdcertsignercontroller.gopkg/operator/etcdcertsigner/etcdcertsignercontroller_test.gopkg/operator/starter.gopkg/tlshelpers/target_cert_creator_test.gopkg/tlshelpers/tlshelpers.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/operator/starter.go
- pkg/tlshelpers/tlshelpers.go
- pkg/cmd/render/certs_test.go
- pkg/tlshelpers/target_cert_creator_test.go
- pkg/cmd/render/certs.go
- pkg/operator/etcdcertsigner/etcdcertsignercontroller.go
rh-roman
force-pushed
the
pki-config-with-render
branch
from
cb9bcfd to
93fe36e
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-roman 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
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cmd/render/render.go (1)
830-859:⚠️ Potential issue | 🟠 MajorParse the full PKI profile here and fail with an error, not a panic.
This helper still only reads
pki.signerCertificates.keyvia chained type assertions. Valid configs that only setdefaults,peerCertificates,servingCertificates, orclientCertificatesare dropped or rejected, and malformed nested data will panic the render path instead of surfacing a normal validation error. Unmarshal the whole PKI section into a typed profile and merge it ontopki.DefaultPKIProfile()instead of indexing throughmap[string]any.#!/bin/bash # Verify that PKIProfile exposes more sections than this helper currently parses, # then compare that with the implementation under review. rg -n 'type PKIProfile struct|Defaults|SignerCertificates|PeerCertificates|ServingCertificates|ClientCertificates' \ vendor/github.com/openshift/api/config/v1alpha1/types_pki.go sed -n '819,862p' pkg/cmd/render/render.go
🧹 Nitpick comments (1)
pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go (1)
609-624: Add one enabled-ConfigurablePKIcontroller case.This fixture hard-disables the new gate and passes
nilforpkiProfileProvider, so the controller changes inpkg/operator/etcdcertsigner/etcdcertsignercontroller.goare still only exercised through the legacy path. One case with a static non-default profile would cover the new signer and leaf certificate plumbing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go` around lines 609 - 624, Add a test case that enables the ConfigurablePKI feature and supplies a non-nil static pkiProfileProvider so the new configurable-PKI path in NewEtcdCertSignerController is exercised: create an enabledFeatureGates set containing features.FeatureGateConfigurablePKI (instead of only creating disabledFeatureGates), construct featureGateAccessor with that enabled set, and pass a simple static implementation of the pkiProfileProvider (returning a non-default profile) into NewEtcdCertSignerController (the pkiProfileProvider parameter currently set to nil in the fixture) so the controller uses the new signer/leaf certificate plumbing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/etcdcertsigner/etcdcertsignercontroller_test.go`:
- Around line 609-624: Add a test case that enables the ConfigurablePKI feature
and supplies a non-nil static pkiProfileProvider so the new configurable-PKI
path in NewEtcdCertSignerController is exercised: create an enabledFeatureGates
set containing features.FeatureGateConfigurablePKI (instead of only creating
disabledFeatureGates), construct featureGateAccessor with that enabled set, and
pass a simple static implementation of the pkiProfileProvider (returning a
non-default profile) into NewEtcdCertSignerController (the pkiProfileProvider
parameter currently set to nil in the fixture) so the controller uses the new
signer/leaf certificate plumbing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b946aafd-3c60-49af-9ad3-8eec9f604b2a
⛔ Files ignored due to path filters (29)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/library-go/pkg/controller/controllercmd/cmd.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/cert_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/keygen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/client_cert_rotation_controller.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/signer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/certrotation/target.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/operator/v1helpers/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/profile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/provider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/resolve.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/pki/types.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (10)
go.modpkg/cmd/render/certs.gopkg/cmd/render/certs_test.gopkg/cmd/render/render.gopkg/cmd/render/render_test.gopkg/operator/etcdcertsigner/etcdcertsignercontroller.gopkg/operator/etcdcertsigner/etcdcertsignercontroller_test.gopkg/operator/starter.gopkg/tlshelpers/target_cert_creator_test.gopkg/tlshelpers/tlshelpers.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/tlshelpers/tlshelpers.go
- pkg/operator/starter.go
- pkg/cmd/render/certs.go
Contributor
|
/test e2e-aws-etcd-certrotation |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
|
/test e2e-aws-etcd-certrotation |
Contributor
|
/payload-aggregate e2e-aws-etcd-certrotation 5 |
Contributor
|
@hasbro17: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
hasbro17
reviewed
Apr 24, 2026
rh-roman
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rh-roman: This pull request references CNTRLPLANE-2847 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 the "5.0.0" version, but no target version was set. 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. |
rh-roman
force-pushed
the
pki-config-with-render
branch
from
93fe36e to
2569e92
Compare
Contributor
|
/test e2e-aws-etcd-certrotation Not sure why e2e-operator is failing bootstrap. |
rh-roman
force-pushed
the
pki-config-with-render
branch
from
2569e92 to
3832b5f
Compare
Contributor
|
/test e2e-aws-etcd-certrotation |
sanchezl
suggested changes
Apr 30, 2026
Updates library-go vendor to pick up certrotation interface changes: - PKIProfileProvider exported on component structs - TargetCertCreator gains CertificateType() and keyGen parameter - New PeerRotation type for dual client+server auth certificates - NewCertRotationController no longer takes featureGates/configInformer
rh-roman
force-pushed
the
pki-config-with-render
branch
from
3832b5f to
735ae42
Compare
Contributor
Author
|
/retest |
sanchezl
suggested changes
May 1, 2026
rh-roman
force-pushed
the
pki-config-with-render
branch
from
735ae42 to
230f463
Compare
Enterprise customers in regulated industries require specific cryptographic
parameters (larger RSA keys, ECDSA curves) for PKI to meet compliance
requirements. This PR adds etcd-operator support for the ConfigurablePKI
feature.
Add support in both operational and installer paths:
- Thread PKIProfileProvider through EtcdCertSignerController for runtime
certificate rotation, reading configuration from cluster PKI CR
- Add getPKIProfileProvider in render command to extract PKI config from
install-config for bootstrap certificate generation
Switch createCertForNode from ServingRotation with a manual
CertificateExtensionFn to PeerRotation, which handles dual
ClientAuth+ServerAuth ExtKeyUsages internally. PeerRotation also
enables proper PKI profile resolution using CertificateTypePeer
(stronger of serving and client key configs).
When ConfigurablePKI is disabled or unconfigured, falls back to library-go
defaults.
Co-authored-by: Luis Sanchez <sanchezl@redhat.com>
rh-roman
force-pushed
the
pki-config-with-render
branch
from
230f463 to
972f944
Compare
Contributor
|
@rh-roman: 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. |
sanchezl
suggested changes
May 2, 2026
Comment on lines
669
to
672
| dynamicInformers.Start(ctx.Done()) | ||
| operatorInformers.Start(ctx.Done()) | ||
| clusterInformers.Start(ctx.Done()) | ||
| kubeInformersForNamespaces.Start(ctx.Done()) |
Contributor
There was a problem hiding this comment.
Add configInformers.Start(ctx.Done()) here.
| if fg.Enabled(features.FeatureGateConfigurablePKI) { | ||
| pkiProfileProvider = pki.NewClusterPKIProfileProvider(configInformers.Config().V1alpha1().PKIs().Lister()) | ||
| additionalInformers = []factory.Informer{ | ||
| configInformers.Config().V1alpha1().PKIs().Informer(), |
Contributor
There was a problem hiding this comment.
I think the failing jobs are because this informer is registered after the call to configInformers.Start() above. Add configInformers.Start() amongst the other informer factory starts at the end of this method.
Contributor
Author
There was a problem hiding this comment.
Will take a look on Monday, but a little worried about moving configInformers.Start() given Vadim's comment just above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for ConfigurablePKI in both operational and installer paths:
Switch
createCertForNodefromServingRotationwith a manualCertificateExtensionFntoPeerRotation, which handles dual ClientAuth+ServerAuth ExtKeyUsages internally.PeerRotationalso enables proper PKI profile resolution usingCertificateTypePeer(stronger of serving and client key configs).When ConfigurablePKI is disabled or unconfigured, falls back to library-go defaults.