CNTRLPLANE-3212: Enable configurable PKI for cert rotation#5842
CNTRLPLANE-3212: Enable configurable PKI for cert rotation#5842sanchezl wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sanchezl: This pull request references CNTRLPLANE-3212 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 sub-task to target the "4.22.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. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe cert-rotation controller New(...) now accepts a shared external config informer factory; cachesToSync and PKIProfileProvider wiring are conditional on FeatureGateConfigurablePKI. Call site and tests were updated to the new arity. go.mod bumps an openshift library-go pseudo-version. ChangesCert rotation controller
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sanchezl
force-pushed
the
CNTRLPLANE-3212-configurable-pki
branch
2 times, most recently
from
fd94f6b to
5ff69a5
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: 1
🧹 Nitpick comments (1)
pkg/controller/certrotation/certrotation_controller_test.go (1)
114-115: Please add one test for the enabledConfigurablePKIpath.This helper only exercises the legacy
nil, nilbranch. A focused test with a real config informer factory plus a stubFeatureGatesHandlerreturningtruewould protect the new PKI provider/cache-sync wiring from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certrotation/certrotation_controller_test.go` around lines 114 - 115, Add a test exercising the ConfigurablePKI branch by calling New (the constructor used in certrotation_controller_test.go) with a real config informer factory and a stub FeatureGatesHandler that returns true instead of the current nil, nil args; instantiate a fake config informer (or shared informer factory) and wire it into the New call, provide a FeatureGatesHandler implementation that always enables the configurable PKI, then assert New returns no error and the controller uses the ConfigurablePKI provider/cache-sync path (e.g., by checking the returned controller/fields or that relevant informers/handlers were initialized) to protect the new PKI provider/cache-sync wiring from regressions.
🤖 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/controller/certrotation/certrotation_controller.go`:
- Around line 140-147: The CertificateName value is using the cluster-scoped
"machine-config.*" identifier so configurable PKI profiles keyed under the
operator-scoped names are never matched; update the CertificateName fields in
the cert rotation setup (the entries that set CertificateName:
"machine-config.machine-config-server-signer" and the other similar occurrences
around lines 174-179) to use the operator-scoped names (e.g.,
"machine-config-operator.machine-config-server-signer" or the correct
operator-prefixed identifiers) so the PKIProfileProvider resolves the intended
profile; keep the surrounding parameters (Validity, Refresh, PKIProfileProvider,
Informer mcoSecretInformer, Lister c.mcoSecretLister, Client
kubeClient.CoreV1(), EventRecorder recorder) unchanged.
---
Nitpick comments:
In `@pkg/controller/certrotation/certrotation_controller_test.go`:
- Around line 114-115: Add a test exercising the ConfigurablePKI branch by
calling New (the constructor used in certrotation_controller_test.go) with a
real config informer factory and a stub FeatureGatesHandler that returns true
instead of the current nil, nil args; instantiate a fake config informer (or
shared informer factory) and wire it into the New call, provide a
FeatureGatesHandler implementation that always enables the configurable PKI,
then assert New returns no error and the controller uses the ConfigurablePKI
provider/cache-sync path (e.g., by checking the returned controller/fields or
that relevant informers/handlers were initialized) to protect the new PKI
provider/cache-sync wiring from regressions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d652483c-e5a6-4b64-a91e-6443f5df86f3
⛔ Files ignored due to path filters (78)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_dns.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_dnses-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.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/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/types_pacemakercluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_20_kube-apiserver_01_kubeapiservers.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_csi-driver_01_clustercsidrivers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/quota/v1/generated.protois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/quota/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/quota/v1/zz_generated.featuregated-crd-manifests.yamlis 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/k8s.io/code-generator/generate-groups.shis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/code-generator/generate-internal-groups.shis excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
cmd/machine-config-controller/start.gogo.modpkg/controller/certrotation/certrotation_controller.gopkg/controller/certrotation/certrotation_controller_test.go
sanchezl
force-pushed
the
CNTRLPLANE-3212-configurable-pki
branch
from
5ff69a5 to
1949b40
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/certrotation/certrotation_controller_test.go (1)
114-114: Add test coverage for the ConfigurablePKI feature-gated pathLine 114 passes
nil, nilforconfigInformerFactoryandfeatureGatesHandler. The constructor has explicit nil-guards (see line 102-105 in certrotation_controller.go), so this is safe. However, the ConfigurablePKI code path—which initializes the PKI profile provider and syncs the PKI informer cache—is never exercised in this test fixture. Add a test case that provides a fake informer factory and enables the FeatureGateConfigurablePKI gate to ensure the feature-gated initialization logic is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certrotation/certrotation_controller_test.go` at line 114, Add a new test case in certrotation_controller_test.go that exercises the ConfigurablePKI feature-gated path by calling New(...) with a fake configInformerFactory and a featureGatesHandler that returns FeatureGateConfigurablePKI enabled instead of passing nil for those parameters; ensure you construct a fake informer factory that exposes the PKI-related informer used by the controller (so the PKI profile provider and informer cache sync code in the New constructor runs) and assert any side-effect or state change the constructor produces (e.g., that the PKI provider was initialized or the informer cache has been synced) to cover the feature-gated initialization logic referenced in New and the nil-guarded code paths.
🤖 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/controller/certrotation/certrotation_controller_test.go`:
- Line 114: Add a new test case in certrotation_controller_test.go that
exercises the ConfigurablePKI feature-gated path by calling New(...) with a fake
configInformerFactory and a featureGatesHandler that returns
FeatureGateConfigurablePKI enabled instead of passing nil for those parameters;
ensure you construct a fake informer factory that exposes the PKI-related
informer used by the controller (so the PKI profile provider and informer cache
sync code in the New constructor runs) and assert any side-effect or state
change the constructor produces (e.g., that the PKI provider was initialized or
the informer cache has been synced) to cover the feature-gated initialization
logic referenced in New and the nil-guarded code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86cfe2b3-f60c-4429-ae30-36feff48de2b
📒 Files selected for processing (3)
cmd/machine-config-controller/start.gopkg/controller/certrotation/certrotation_controller.gopkg/controller/certrotation/certrotation_controller_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/certrotation/certrotation_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/machine-config-controller/start.go
|
/retest |
|
@sanchezl: This pull request references CNTRLPLANE-3212 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 sub-task to target the "4.22.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. |
|
@sanchezl: This pull request references CNTRLPLANE-3212 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 sub-task to target the "4.22.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. |
|
/payload-job-with-prs periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-techpreview #5842 |
|
@sanchezl: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/retest-required |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-4.22-e2e-aws-ovn-techpreview #5842 |
|
@sanchezl: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/48d7f230-34db-11f1-8b17-3ebe6e6b3dc7-0 |
|
@sanchezl: This PR was included in a payload test run from #5842
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/48d7f230-34db-11f1-8b17-3ebe6e6b3dc7-0 |
|
/test e2e-gcp-op-ocl |
|
/test e2e-gcp-op-ocl-part2 |
Verification: Cert Rotation Controller with Configurable PKIAnalyzed the passing Cert Rotation Controller — RunningCertificates — RSA 2048-bit (expected default)
ConfigurablePKI Feature Gate — Disabled (correct)Feature gate logs confirm
No Rotation Triggered (expected)Bootstrap created certs at |
|
/test e2e-gcp-op-single-node |
|
/test e2e-hypershift |
1 similar comment
|
/test e2e-hypershift |
|
/retest-required |
|
Hi @cheesesashimi @umohnani8 — could you review this when you have a moment? This integrates the Cert rotation controller verified working in e2e (see verification comment above). CI retests are running now. |
|
@sanchezl: 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. |
|
/retest |
openshift-ci
Bot
added
the
needs-rebase
sanchezl
force-pushed
the
CNTRLPLANE-3212-configurable-pki
branch
from
1949b40 to
4b3fa69
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sanchezl 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 |
|
@sanchezl: This pull request references CNTRLPLANE-3212 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 sub-task 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. |
2 participants
Summary
ConfigurablePKIfeature gate is enabled, resolve key algorithms from the clusterPKIresourcenilprovider preserves legacy RSA-2048Details
configInformerFactoryandfeatureGatesHandlerparameters to the cert rotation controller constructorPKIProfileProviderusingpki.NewClusterPKIProfileProvider()when theConfigurablePKIfeature gate is enabledCertificateNameandPKIProfileProvideronRotatedSigningCASecretandRotatedSelfSignedCertKeySecret:machine-config.machine-config-server-signermachine-config.machine-config-server-servingReferences
Summary by CodeRabbit
New Features
Chores