CM-966: Apply cluster TLS security profile to cert-manager operands#409
CM-966: Apply cluster TLS security profile to cert-manager operands#409arun717 wants to merge 9 commits intoopenshift:masterfrom
Conversation
Add new package to map OpenShift TLS security profiles to cert-manager CLI flags. This enables centralized TLS configuration from the cluster APIServer resource. Features: - EffectiveSpec resolves TLS profiles (Old, Intermediate, Modern, Custom) - Defaults to Intermediate profile when nil or empty - CertManagerWebhookTLSArgs generates flags for webhook main + metrics listeners - CertManagerOperandMetricsTLSArgs generates flags for controller/cainjector metrics - Converts OpenSSL cipher names to IANA format using library-go Note: Curve preferences are not yet configurable due to upstream cert-manager limitations - operands inherit Go's default curve ordering. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement deployment hook to fetch and apply TLS configuration from the cluster APIServer resource to cert-manager operand deployments. The hook: - Fetches apiserver.config.openshift.io/cluster resource - Resolves TLS profile using tlsprofile.EffectiveSpec - Applies appropriate TLS flags based on deployment type: * cert-manager-webhook: main TLS + metrics TLS flags * cert-manager (controller): metrics TLS flags only * cert-manager-cainjector: metrics TLS flags only - Merges TLS args into existing container args with override semantics - Skips deployments with != 1 container Includes comprehensive unit tests covering: - All TLS profile types (Old, Intermediate, Modern, Custom) - Nil profile handling (defaults to Intermediate) - Flag override behavior - Multi-container deployment skipping - Unknown deployment handling - Error handling when APIServer resource not found Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Register withClusterTLSProfileFromAPIServer hook in the generic deployment controller for cert-manager operands. The hook is only registered when infraInformers.Applicable() returns true (OpenShift clusters), ensuring the operator remains platform-agnostic and doesn't break on non-OpenShift environments. Adds APIServer informer to the controller's watch list to trigger reconciliation when the cluster TLS profile changes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add permissions to read apiservers resources from config.openshift.io API group. This is required for the TLS profile hook to fetch the cluster APIServer configuration. Changes: - Add apiservers to kubebuilder RBAC annotation in certmanager_controller.go - Update config/rbac/role.yaml with generated permissions - Update bundle CSV with generated permissions Without these permissions, the operator cannot fetch the cluster TLS profile and will fail to configure cert-manager operands correctly on OpenShift clusters. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add documentation for the --certificate-request-minimum-backoff-duration controller flag in the CertManagerSpec ControllerConfig examples. Also add the flag to the list of supported controller args in the validation hook to allow users to configure initial backoff duration when CertificateRequests fail. This flag controls the cert-manager trigger controller's backoff behavior, with backoff doubling on consecutive failures up to 32h. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add period to updateWebhookClientConfig function comment to comply with godoc style guidelines. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@arun717: This pull request references CM-966 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
needs-rebase
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds OpenShift TLS security profile support: new tlsprofile package, deployment hook to inject TLS CLI flags into cert-manager deployments, RBAC rule to allow reading APIServer, helper utilities for merging container args, and comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant Controller as rgba(52, 152, 219, 0.5) Controller
participant APIServerLister as rgba(46, 204, 113, 0.5) APIServer Informer/Lister
participant TLSProfile as rgba(155, 89, 182, 0.5) tlsprofile Package
participant Deployment as rgba(241, 196, 15, 0.5) Cert-Manager Deployment
Controller->>APIServerLister: Get cluster APIServer
APIServerLister-->>Controller: APIServer (TLSSecurityProfile)
Controller->>TLSProfile: EffectiveSpec(profile)
TLSProfile-->>Controller: TLSProfileSpec
Controller->>TLSProfile: CertManagerWebhookTLSArgs / OperandMetricsTLSArgs(spec)
TLSProfile-->>Controller: CLI args (--tls-*, --metrics-tls-*)
Controller->>Deployment: mergeContainerArgs(existingArgs, generatedArgs)
Deployment-->>Controller: merged args
Controller->>Deployment: update Deployment template
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arun717 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 |
|
@arun717: This pull request references CM-966 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. |
![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/deployment/tls_profile_hook.go (1)
20-22: Avoid silent TLS-profile bypass on multi-container pod specs.Returning
nilhere can hide future deployment-shape drift and skip TLS args silently. Consider selecting the target container explicitly (by known name/image) or emitting a clear error/log.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/deployment/tls_profile_hook.go` around lines 20 - 22, The current early return when len(deployment.Spec.Template.Spec.Containers) != 1 silently skips TLS arg injection; instead, change the logic in tls_profile_hook to explicitly handle multi-container PodSpecs: attempt to select the intended container by a known identifier (e.g., match container.Name or container.Image against a configured targetContainerName/targetContainerImage) and operate on that container, and if no match is found emit a clear error or warning (using the controller logger) rather than returning nil silently; ensure the code references deployment.Spec.Template.Spec.Containers and the chosen selection variables (targetContainerName/targetContainerImage) so future shape drift is surfaced.
🤖 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/deployment/certmanager_controller.go`:
- Line 52: The RBAC kubebuilder marker on the certmanager controller grants
mutating verbs to the apiservers resource; change the marker so that the
apiservers entry only includes read-only verbs (get;list;watch) while leaving
other resources (certmanagers, clusteroperators, clusteroperators/status,
infrastructures) unchanged—update the kubebuilder:rbac comment line that
contains apiservers to restrict it to verbs=get;list;watch.
---
Nitpick comments:
In `@pkg/controller/deployment/tls_profile_hook.go`:
- Around line 20-22: The current early return when
len(deployment.Spec.Template.Spec.Containers) != 1 silently skips TLS arg
injection; instead, change the logic in tls_profile_hook to explicitly handle
multi-container PodSpecs: attempt to select the intended container by a known
identifier (e.g., match container.Name or container.Image against a configured
targetContainerName/targetContainerImage) and operate on that container, and if
no match is found emit a clear error or warning (using the controller logger)
rather than returning nil silently; ensure the code references
deployment.Spec.Template.Spec.Containers and the chosen selection variables
(targetContainerName/targetContainerImage) so future shape drift is surfaced.
🪄 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: f4a6a32d-6323-4660-b3a5-b4716acf4959
📒 Files selected for processing (11)
api/operator/v1alpha1/certmanager_types.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/rbac/role.yamlpkg/controller/deployment/certmanager_controller.gopkg/controller/deployment/deployment_overrides_validation.gopkg/controller/deployment/generic_deployment_controller.gopkg/controller/deployment/tls_profile_hook.gopkg/controller/deployment/tls_profile_hook_test.gopkg/controller/trustmanager/webhooks.gopkg/tlsprofile/tlsprofile.gopkg/tlsprofile/tlsprofile_test.go
openshift-ci
bot
removed
the
needs-rebase
|
@arun717: This pull request references CM-966 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controller/certmanager/certmanager_controller.go (1)
52-52:⚠️ Potential issue | 🟠 MajorRestrict
apiserversRBAC to read-only verbs.Line 52 still grants mutating verbs on
config.openshift.io/apiservers, but this flow only needs read access forapiserver.config.openshift.io/cluster.🔒 Suggested RBAC tightening
-//+kubebuilder:rbac:groups="config.openshift.io",resources=certmanagers;clusteroperators;clusteroperators/status;infrastructures;apiservers,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups="config.openshift.io",resources=certmanagers;clusteroperators;clusteroperators/status;infrastructures,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups="config.openshift.io",resources=apiservers,verbs=get;list;watch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/certmanager_controller.go` at line 52, Update the kubebuilder RBAC marker comment that currently grants mutating verbs to config.openshift.io/apiservers: locate the kubebuilder marker line containing groups="config.openshift.io" and resources=...apiservers... and change the verbs for the apiservers resource to read-only (get;list;watch) while leaving other resources' verbs unchanged; ensure the updated marker still follows the exact //+kubebuilder:rbac:... format so codegen picks up the tighter permission.
🧹 Nitpick comments (1)
pkg/controller/certmanager/tls_profile_hook.go (1)
24-41: Filter by deployment name before querying APIServer.Right now the hook reads
apiserver.config.openshift.io/clustereven for deployments that are later ignored. Moving the deployment-name gate first avoids unnecessary informer reads and avoids erroring on unsupported deployments due to unrelated APIServer lookup failures.♻️ Suggested restructuring
- apiServer, err := apiServerInformer.Lister().Get("cluster") - if err != nil { - return fmt.Errorf("failed to get apiserver.config.openshift.io/cluster: %w", err) - } - - effective, err := tlsprofile.EffectiveSpec(apiServer.Spec.TLSSecurityProfile) - if err != nil { - return err - } - var extra []string switch deployment.Name { case certmanagerWebhookDeployment: - extra = tlsprofile.CertManagerWebhookTLSArgs(effective) case certmanagerControllerDeployment, certmanagerCAinjectorDeployment: - extra = tlsprofile.CertManagerOperandMetricsTLSArgs(effective) default: return nil } + + apiServer, err := apiServerInformer.Lister().Get("cluster") + if err != nil { + return fmt.Errorf("failed to get apiserver.config.openshift.io/cluster: %w", err) + } + effective, err := tlsprofile.EffectiveSpec(apiServer.Spec.TLSSecurityProfile) + if err != nil { + return err + } + switch deployment.Name { + case certmanagerWebhookDeployment: + extra = tlsprofile.CertManagerWebhookTLSArgs(effective) + case certmanagerControllerDeployment, certmanagerCAinjectorDeployment: + extra = tlsprofile.CertManagerOperandMetricsTLSArgs(effective) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/certmanager/tls_profile_hook.go` around lines 24 - 41, Move the deployment-name filter to run before any APIServer informer lookup: first check deployment.Name against certmanagerWebhookDeployment, certmanagerControllerDeployment, and certmanagerCAinjectorDeployment and compute the required extra args (tlsprofile.CertManagerWebhookTLSArgs or tlsprofile.CertManagerOperandMetricsTLSArgs) or return nil for others; only after deciding you need TLS info call apiServerInformer.Lister().Get("cluster") and then call tlsprofile.EffectiveSpec on the retrieved apiServer.Spec.TLSSecurityProfile, so you avoid unnecessary apiServerInformer.Lister().Get calls and errors for ignored deployments.
🤖 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/certmanager/tls_profile_hook.go`:
- Line 1: The package declaration is wrong: change the package line in
tls_profile_hook.go (and the test file tls_profile_hook_test.go) from "package
deployment" to "package certmanager" so that symbols like
withClusterTLSProfileFromAPIServer are in the same package as the other files
under pkg/controller/certmanager and the unqualified call in
generic_deployment_controller.go resolves correctly.
---
Duplicate comments:
In `@pkg/controller/certmanager/certmanager_controller.go`:
- Line 52: Update the kubebuilder RBAC marker comment that currently grants
mutating verbs to config.openshift.io/apiservers: locate the kubebuilder marker
line containing groups="config.openshift.io" and resources=...apiservers... and
change the verbs for the apiservers resource to read-only (get;list;watch) while
leaving other resources' verbs unchanged; ensure the updated marker still
follows the exact //+kubebuilder:rbac:... format so codegen picks up the tighter
permission.
---
Nitpick comments:
In `@pkg/controller/certmanager/tls_profile_hook.go`:
- Around line 24-41: Move the deployment-name filter to run before any APIServer
informer lookup: first check deployment.Name against
certmanagerWebhookDeployment, certmanagerControllerDeployment, and
certmanagerCAinjectorDeployment and compute the required extra args
(tlsprofile.CertManagerWebhookTLSArgs or
tlsprofile.CertManagerOperandMetricsTLSArgs) or return nil for others; only
after deciding you need TLS info call apiServerInformer.Lister().Get("cluster")
and then call tlsprofile.EffectiveSpec on the retrieved
apiServer.Spec.TLSSecurityProfile, so you avoid unnecessary
apiServerInformer.Lister().Get calls and errors for ignored deployments.
🪄 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: 6a97dc14-52f8-4667-894e-957150445cd0
📒 Files selected for processing (5)
bundle/manifests/cert-manager-operator.clusterserviceversion.yamlpkg/controller/certmanager/certmanager_controller.gopkg/controller/certmanager/generic_deployment_controller.gopkg/controller/certmanager/tls_profile_hook.gopkg/controller/certmanager/tls_profile_hook_test.go
✅ Files skipped from review due to trivial changes (1)
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
| @@ -0,0 +1,49 @@ | |||
| package deployment | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify package-name consistency in pkg/controller/certmanager
rg -n '^package\s+' --type=go pkg/controller/certmanager
# Expected: all non-_test.go files in this directory use the same package name.Repository: openshift/cert-manager-operator
Length of output: 1920
🏁 Script executed:
# Search for imports of the 'deployment' package from pkg/controller/certmanager
rg 'from.*pkg/controller/certmanager|import.*certmanager' --type=go -A2 -B2Repository: openshift/cert-manager-operator
Length of output: 57
🏁 Script executed:
# Check if withClusterTLSProfileFromAPIServer is defined in tls_profile_hook.go
rg -n 'withClusterTLSProfileFromAPIServer' pkg/controller/certmanager/tls_profile_hook.goRepository: openshift/cert-manager-operator
Length of output: 298
🏁 Script executed:
# Search for any calls to withClusterTLSProfileFromAPIServer to see how it's used
rg -n 'withClusterTLSProfileFromAPIServer' --type=goRepository: openshift/cert-manager-operator
Length of output: 1165
🏁 Script executed:
# Check the context around the withClusterTLSProfileFromAPIServer call in generic_deployment_controller.go
sed -n '60,80p' pkg/controller/certmanager/generic_deployment_controller.goRepository: openshift/cert-manager-operator
Length of output: 814
Change package declaration from deployment to certmanager.
This file is in pkg/controller/certmanager/ where all non-test files use package certmanager. The incorrect package deployment declaration breaks the build: generic_deployment_controller.go:71 calls withClusterTLSProfileFromAPIServer() unqualified, which fails with undefined symbol error when this function is in a different package.
🛠️ Minimal fix
-package deployment
+package certmanagerAlso fix the test file tls_profile_hook_test.go which has the same issue.
📝 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.
| package deployment | |
| package certmanager |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/certmanager/tls_profile_hook.go` at line 1, The package
declaration is wrong: change the package line in tls_profile_hook.go (and the
test file tls_profile_hook_test.go) from "package deployment" to "package
certmanager" so that symbols like withClusterTLSProfileFromAPIServer are in the
same package as the other files under pkg/controller/certmanager and the
unqualified call in generic_deployment_controller.go resolves correctly.
Move tls_profile_hook.go and tls_profile_hook_test.go from pkg/controller/certmanager/ back to pkg/controller/deployment/ to fix package mismatch and compilation errors. Changes: - Move TLS profile hook files to deployment package - Export WithClusterTLSProfileFromAPIServer for use by certmanager package - Add tls_helpers.go with deployment name constants and mergeContainerArgs - Update generic_deployment_controller.go to import and use deploymentpkg - Update all test references to use capitalized function name This fixes the "found packages certmanager and deployment" error that occurred after the directory structure was changed during rebase. All unit tests now pass ✅ - pkg/controller/deployment: 94.4% coverage - pkg/controller/certmanager: 41.1% coverage - pkg/tlsprofile: 63.6% coverage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@arun717: This pull request references CM-966 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. |
Change the RBAC permissions for the apiservers resource from full mutating verbs (get;list;watch;create;update;patch;delete) to read-only (get;list;watch). The operator only needs to read the cluster APIServer resource to fetch the TLS security profile. It does not need to create or modify the APIServer resource, so granting mutating permissions violates the principle of least privilege. Changes: - Split the config.openshift.io RBAC marker into two separate rules - apiservers: get;list;watch (read-only) - certmanagers, clusteroperators, clusteroperators/status, infrastructures: get;list;watch;create;update;patch;delete (full access) - Regenerated config/rbac/role.yaml - Regenerated bundle/manifests CSV - Regenerated CRD manifests All unit tests pass ✅ Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@arun717: This pull request references CM-966 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. |
|
@arun717: 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 |
2 participants
CM-966: Implement centralized TLS profile fetching and application
Summary
Implements centralized TLS configuration for cert-manager operands by fetching and applying TLS settings from the cluster APIServer resource (
apiserver.config.openshift.io/cluster). This ensures cert-manager components honor the cluster-wide TLS security profile.Fixes: CM-966
Related: CM-954
Changes
Core Implementation
TLS Profile Package (
pkg/tlsprofile/)TLS Profile Hook (
pkg/controller/deployment/tls_profile_hook.go)cert-manager-webhook: main TLS + metrics TLS flagscert-manager(controller): metrics TLS flags onlycert-manager-cainjector: metrics TLS flags onlyIntegration
RBAC
apiserversfromconfig.openshift.ioAPI groupTesting
Known Limitations
Test Plan
Unit Tests
Manual Testing (OpenShift Cluster)
oc patch apiserver cluster --type=merge -p '{"spec":{"tlsSecurityProfile":{"type":"Modern"}}}'Verification
Commit Structure
The changes are organized into logical commits:
feat(tlsprofile): Add TLS profile mapping packagefeat(controller): Add TLS profile hook for cert-manager deploymentsfeat(controller): Integrate TLS profile hook into deployment controllerchore(rbac): Add APIServer resource permissionsdocs(api): Add certificate-request-minimum-backoff-duration flag examplestyle(trustmanager): Fix comment formatting🤖 Generated with Claude Code via
/jira:solve [CM-966](https://redhat.atlassian.net/browse/CM-966)Summary by CodeRabbit
New Features
Documentation
Chores
Tests