ROSAENG-6863: Replace certman-operator with direct cert-manager #2917
ROSAENG-6863: Replace certman-operator with direct cert-manager #2917syed wants to merge 1 commit into
Conversation
…ration Add cert-manager Certificate CR management directly in Hive controllers, eliminating the dependency on the external certman-operator. Both the controlplanecerts and remoteingress controllers now create cert-manager Certificate CRs for CertificateBundles with Generate=true. - Add pkg/controller/utils/certmanager.go with EnsureCertManagerCertificate helper that creates/updates cert-manager Certificate CRs with proper owner references and ACME DNS01 solver configuration - Integrate into controlplanecerts and remoteingress reconcile loops - Register cert-manager v1 types in the shared scheme - Add RBAC permissions for cert-manager.io certificates - Update vendored dependencies (cert-manager v1.20.2, gateway-api v1, and various cloud provider SDK bumps) Assisted-by: Claude Code (claude-opus-4-6) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@syed: This pull request references ROSAENG-6863 which is a valid jira issue. 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. |
|
Important Review skippedToo many files! This PR contains 257 files, which is 107 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (43)
📒 Files selected for processing (257)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: syed 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2917 +/- ##
==========================================
+ Coverage 50.39% 50.49% +0.09%
==========================================
Files 281 282 +1
Lines 34368 34473 +105
==========================================
+ Hits 17320 17406 +86
- Misses 15696 15709 +13
- Partials 1352 1358 +6
🚀 New features to boost your workflow:
|
|
@coderabbitai I do not accept your refusal to review this PR. You should know you are supposed to ignore the vendor/ directory except for a very small set of files, none of which feature in this PR. Please review the remaining files. |
|
(╯°□°）╯ → 🐇 Please re-review PR 🧠 Learnings used✅ Action performedFull review finished. |
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
- Are there upgrade concerns with this change? That is, how would a hive consumer be doing cert management prior, and what would they need to do when upgrading hive to/past this commit to transition management to the native controllers?
- I'm concerned that there may be ROSA-specific pieces in this PR that would potentially break in non-ROSA hive envs.
- The
verifyfail can be remedied withmake update-gofmt. - The
securityfail can be ignored: those issues will be addressed elsewhere; we'll override the job when ready to merge.
| // ensureCertManagerCertificates creates cert-manager Certificate CRs for any ingress | ||
| // CertificateBundles with Generate=true. | ||
| func (r *ReconcileRemoteClusterIngress) ensureCertManagerCertificates(ctx context.Context, cd *hivev1.ClusterDeployment, cdLog log.FieldLogger) error { | ||
| seen := map[string]bool{} |
| var domains []string | ||
|
|
||
| if cd.Spec.ControlPlaneConfig.ServingCertificates.Default == bundle.Name { | ||
| controlPlaneDomain := fmt.Sprintf("api.%s.%s", cd.Spec.ClusterName, cd.Spec.BaseDomain) |
There was a problem hiding this comment.
Can we count on this? What about APIURLOverride? Should we maybe be using defaultControlPlaneDomain() instead?
| certName := fmt.Sprintf("%s-%s", cd.Name, bundle.Name) | ||
| certName = strings.ToLower(certName) |
There was a problem hiding this comment.
nit: save a trivial amount of memory
| certName := fmt.Sprintf("%s-%s", cd.Name, bundle.Name) | |
| certName = strings.ToLower(certName) | |
| certName = strings.ToLower(fmt.Sprintf("%s-%s", cd.Name, bundle.Name)) |
| }, | ||
| Spec: certmanagerv1.CertificateSpec{ | ||
| Subject: &certmanagerv1.X509Subject{ | ||
| Organizations: []string{"Red Hat - Open Cluster Manager"}, |
There was a problem hiding this comment.
I'm not well-educated in this space, but this seems like it would not be appropriate for self-managed clusters?
| CertManagerIssuerName = "public-issuer" | ||
| CertManagerIssuerKind = "ClusterIssuer" | ||
| certDuration = 90 * 24 * time.Hour | ||
| certRenewBefore = 30 * 24 * time.Hour |
There was a problem hiding this comment.
I need to understand these hardcoded values.
(Slight aside: any reason the first two are exported?)
There was a problem hiding this comment.
these are moved from certman-operator. I will remove them and use lib provided values
There was a problem hiding this comment.
We are still moving old certman-operator logic to hive? We not let cert-mgr determine these duration/renewal lifetimes?
|
Thank you for the review!
Prior to this change, cert management was done via certman-operator. This is a custom ACME client which runs on the hive shards. We had an outage last week because Let's Encrypt started using a different root cert and there was a bug in certman-operator where we were not passing the full cert chain which caused TLS errors on the API endpoints. This is the primary motivation from us to move to cert-manager as it will remove the maintenance burden of a bespoke operator. The way certman-operator works is it watches
My apologies, I'm not familiar with other non-ROSA envs. One potential issue I see is DNS validation. |
So yeah, in addition to managed openshift (which btw is also more than just ROSA -- there are non-ROSA OSD spokes on AWS and GCP managed from those same shards) hive is also bundled with ACM, and available standalone on OperatorHub. It supports ~ half a dozen infrastructure providers, including on-prem as well as cloud models. And I'm afraid I don't know enough about cert management to understand all the ramifications of bringing it "in house" like this. This work implies that, today, Certificate CRs are managed externally in hive hub environments. I assume there's more than one way that can be done -- e.g. on-prem vsphere under ACM probably doesn't use openshift/certman-operator. And what happens when hive upgrades through this path on a hub where some other controller (e.g. gitops) expects to have ownership of those CRs? Do we end up thrashing them? If nothing else, I would expect owner references to be contended. I'm starting to suspect that this thing may want to be conditioned on a HiveConfig and/or ClusterDeployment knob, such that the default behavior doesn't change on a hive upgrade. And if my concerns about hardcoded values (like the Organization and the renewal times) turn out to be valid, exposing those as configurables in the same place may be necessary/appropriate. And wherever we go, I'm guessing we'll want doc changes to explain it. I sure would like to bring in some more expertise in both certs in general and how they're managed/used with hive. I'm not sure who that would be. @coderabbitai do you have thoughts on the above? |
|
@syed: 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. |
|
I also do not know enough about cert management across the whole org to give a definite answer. Let me talk to a few folks and get back |
|
/hold for more discussion/strategy around the design/implementation |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Update from ARO Classic, they don't use the |
I can't see that hive does anything with that field today. @coderabbitai can you tell whether it has any effect? Or maybe if we offload the bundle contents to some other agent that may act on it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== CertificateBundleSpec type definition ==="
rg -n "Generate" --type go -B2 -A2 apis/config/
echo ""
echo "=== All usages of .Generate in controllers ==="
rg -n "\.Generate" --type go -B3 -A3 pkg/controller/
echo ""
echo "=== CertificateBundleSpec struct ==="
rg -n "CertificateBundleSpec" --type go -B2 -A10 apis/
echo ""
echo "=== How Generate is used in controlplanecerts ==="
rg -n "Generate" --type go -B3 -A3 pkg/controller/controlplanecerts/
echo ""
echo "=== How Generate is used in remoteingress ==="
rg -n "Generate" --type go -B3 -A3 pkg/controller/remoteingress/
echo ""
echo "=== Look for certman-operator references ==="
rg -rn "certman" --type go -B2 -A2 pkg/
echo ""
echo "=== Look at CertificateBundle handling in controlplanecerts ==="
rg -n "CertificateBundle\|certificateBundle" --type go -B2 -A5 pkg/controller/controlplanecerts/
echo ""
echo "=== Look at CertificateBundle handling in remoteingress ==="
rg -n "CertificateBundle\|certificateBundle" --type go -B2 -A5 pkg/controller/remoteingress/Length of output: 36150
Pre-PR behaviour of In the current codebase (before this PR's additions), hive itself does nothing with What this PR changes: This PR makes hive unconditionally act on The consequence: For any hive hub where cert-manager is not installed (standalone hive, ACM with on-prem spokes, non-ROSA/OSD environments), reconciliation of any This strongly supports the concern that this needs to be gated — e.g. a new (´• ω •`) 🐇 |
|
Thanks for the info. As I understand |
This sounds reasonable on the surface, but I don't really have the background to understand all the implications. Would need to push an RFE through The Process™. |
4 participants
Add cert-manager Certificate CR management directly in Hive controllers, eliminating the dependency on the external certman-operator. Both the controlplanecerts and remoteingress controllers now create cert-manager Certificate CRs for CertificateBundles with Generate=true.
Assisted-by: Claude Code (claude-opus-4-6)