CM-1225: Apply cluster TLS profile to trust-manager, operator metrics, and operand HTTPS metrics - #466
CM-1225: Apply cluster TLS profile to trust-manager, operator metrics, and operand HTTPS metrics#466arun717 wants to merge 8 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
|
@arun717: This pull request references CM-954 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "cert-manager-1.20" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test tls-scanner |
|
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:
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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe operator now applies cluster TLS profiles to serving endpoints and trust-manager. Cert-manager operand metrics use dynamic serving certificates and HTTPS scraping. OpenShift serving certificate mounts and annotations are configured. ChangesCluster TLS and serving
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OperatorCommand
participant APIServer
participant TLSProfile
participant TrustManager
participant CertManagerController
participant OperandDeployment
OperatorCommand->>APIServer: fetch cluster TLS profile
APIServer-->>TLSProfile: return profile configuration
TLSProfile-->>OperatorCommand: apply serving settings
APIServer->>TrustManager: emit matching APIServer event
TrustManager->>APIServer: resolve effective TLS profile
APIServer-->>TrustManager: return effective profile
TrustManager->>OperandDeployment: apply webhook TLS arguments
CertManagerController->>OperandDeployment: apply dynamic metrics TLS
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/tlsprofile/tlsprofile.go (1)
60-78: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReject TLS profiles with unmappable cipher names.
OpenSSLToIANACipherSuitesdrops unknown names. The three non-TLS 1.3 argument helpers can therefore emit empty cipher flags. Return an error fromjoinIANACiphersand propagate it through the cert-manager and trust-manager callers, matchingApplyToHTTPServingInfo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tlsprofile/tlsprofile.go` around lines 60 - 78, Update joinIANACiphers to return an error when any cipher name cannot be mapped instead of silently dropping it, and propagate that error through CertManagerWebhookTLSArgs and the corresponding trust-manager argument helper. Match the error-returning behavior and handling used by ApplyToHTTPServingInfo, while preserving the existing TLS 1.3 path and successful cipher argument generation.
🧹 Nitpick comments (1)
pkg/controller/certmanager/deployment_metrics_tls_test.go (1)
29-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing secret-namespace assertion for webhook and cainjector cases.
The webhook and cainjector
wantArgsslices omit--metrics-dynamic-serving-ca-secret-namespace=$(POD_NAMESPACE), while the controller case checks it. Add the same assertion to all three cases for symmetric coverage.🧪 Proposed fix to align test coverage across cases
{ name: "webhook", deploymentName: certmanagerWebhookDeployment, wantArgs: []string{ + "--metrics-dynamic-serving-ca-secret-namespace=$(POD_NAMESPACE)", "--metrics-dynamic-serving-ca-secret-name=cert-manager-metrics-ca", "--metrics-dynamic-serving-dns-names=cert-manager-webhook,cert-manager-webhook.$(POD_NAMESPACE),cert-manager-webhook.$(POD_NAMESPACE).svc", }, wantScheme: "https", }, { name: "cainjector", deploymentName: certmanagerCAinjectorDeployment, wantArgs: []string{ + "--metrics-dynamic-serving-ca-secret-namespace=$(POD_NAMESPACE)", "--metrics-dynamic-serving-ca-secret-name=cert-manager-metrics-ca", "--metrics-dynamic-serving-dns-names=cert-manager-cainjector,cert-manager-cainjector.$(POD_NAMESPACE),cert-manager-cainjector.$(POD_NAMESPACE).svc", }, wantScheme: "https", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/certmanager/deployment_metrics_tls_test.go` around lines 29 - 46, Update the test cases identified by the deploymentName values certmanagerControllerDeployment, certmanagerWebhookDeployment, and certmanagerCAinjectorDeployment so each wantArgs slice includes the metrics dynamic-serving CA secret namespace argument using $(POD_NAMESPACE), keeping the assertions symmetric across all three cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/operator/cmd.go`:
- Around line 57-58: Handle and propagate the errors returned by the flag
lookups in the command setup, including terminateOnFiles, listen, kubeconfig,
and namespace. Replace each ignored error with explicit error handling that
stops command initialization and returns the failure, preserving the existing
values and subsequent behavior when lookups succeed.
- Around line 119-126: Update the serving setup around
ApplyClusterProfileToHTTPServingInfo to execute the cluster TLS profile lookup
with a finite context timeout, ensuring stalled API requests cannot block
startup and preserving error propagation. Raise the RESTConfigFromKubeConfig
failure log from informational to warning level so fallback to default serving
TLS settings is visible.
In `@pkg/tlsprofile/serving.go`:
- Around line 23-61: Extract the shared cluster APIServer lookup, adherence
validation, warning, and EffectiveSpec resolution into a helper in the
tlsprofile package, parameterized for client-go REST and controller-runtime
fetchers while preserving deployment_tls.go’s correct Get error handling. In
pkg/tlsprofile/serving.go:23-61, replace the duplicated block in
ApplyClusterProfileToHTTPServingInfo with the helper and retain only
HTTPServingInfo application. In
pkg/controller/trustmanager/deployment_tls.go:18-18 and its
applyClusterTLSProfile block, use the same helper and retain only
deployment-argument merge logic.
- Around line 36-41: Update the APIServers lookup error handling in the serving
configuration function around ConfigV1().APIServers().Get: retain the nil
fallback and informational log only when apierrors.IsNotFound(err) is true, and
return other errors to surface startup failures. Follow the established pattern
in the sibling deployment TLS reconciler and reuse the existing Kubernetes API
error handling conventions.
---
Outside diff comments:
In `@pkg/tlsprofile/tlsprofile.go`:
- Around line 60-78: Update joinIANACiphers to return an error when any cipher
name cannot be mapped instead of silently dropping it, and propagate that error
through CertManagerWebhookTLSArgs and the corresponding trust-manager argument
helper. Match the error-returning behavior and handling used by
ApplyToHTTPServingInfo, while preserving the existing TLS 1.3 path and
successful cipher argument generation.
---
Nitpick comments:
In `@pkg/controller/certmanager/deployment_metrics_tls_test.go`:
- Around line 29-46: Update the test cases identified by the deploymentName
values certmanagerControllerDeployment, certmanagerWebhookDeployment, and
certmanagerCAinjectorDeployment so each wantArgs slice includes the metrics
dynamic-serving CA secret namespace argument using $(POD_NAMESPACE), keeping the
assertions symmetric across all three cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e8c1dbbb-52b2-48d7-9958-28a9f622da7e
📒 Files selected for processing (26)
bindata/cert-manager-deployment/cainjector/cert-manager-cainjector-deployment.yamlbindata/cert-manager-deployment/controller/cert-manager-deployment.yamlbindata/cert-manager-deployment/controller/cert-manager-metrics-dynamic-serving-rb.yamlbindata/cert-manager-deployment/controller/cert-manager-metrics-dynamic-serving-role.yamlbindata/cert-manager-deployment/webhook/cert-manager-webhook-deployment.yamlbundle/manifests/cert-manager-operator-controller-manager-metrics-service_v1_service.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/manager/manager.yamlconfig/manifests/bases/cert-manager-operator.clusterserviceversion.yamlconfig/rbac/auth_proxy_service.yamlpkg/cmd/operator/cmd.gopkg/controller/certmanager/cert_manager_controller_deployment.gopkg/controller/certmanager/deployment_metrics_tls.gopkg/controller/certmanager/deployment_metrics_tls_test.gopkg/controller/certmanager/generic_deployment_controller.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/deployment_tls.gopkg/controller/trustmanager/deployment_tls_test.gopkg/controller/trustmanager/deployments.gopkg/operator/assets/bindata.gopkg/tlsprofile/serving.gopkg/tlsprofile/serving_test.gopkg/tlsprofile/tlsprofile.gopkg/tlsprofile/tlsprofile_test.gotest/e2e/tls_profile_test.gotest/e2e/utils_test.go
arun717
changed the title
|
@arun717: This pull request references CM-1225 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tlsprofile/cluster.go`:
- Around line 34-38: Update the documentation comment for the ObjectGetter
interface to accurately reflect that it matches common.CtrlClient rather than
claiming it matches both client.Reader and this repo's CtrlClient. Correct the
comment to state the actual interface it represents based on the production
usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 735a854a-9db9-4acf-a83c-146317149c3b
📒 Files selected for processing (7)
pkg/cmd/operator/cmd.gopkg/controller/trustmanager/controller.gopkg/controller/trustmanager/deployment_tls.gopkg/controller/trustmanager/deployment_tls_test.gopkg/tlsprofile/cluster.gopkg/tlsprofile/cluster_test.gopkg/tlsprofile/serving.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/trustmanager/controller.go
- pkg/controller/trustmanager/deployment_tls.go
- pkg/cmd/operator/cmd.go
|
/test tls-scanner |
Honor apiserver tlsAdherence and inject --tls-min-version / --tls-cipher-suites onto the trust-manager Deployment when required.
Apply apiserver tlsSecurityProfile to library-go HTTPServingInfo at startup when tlsAdherence requires it, and wire serving-cert mounts for the metrics Service.
Turn on dynamic metrics serving certificates for controller, webhook, and cainjector so --metrics-tls-* profile flags apply to real TLS on :9402, with RBAC for the shared metrics CA secret.
Set features.operators.openshift.io/tls-profiles to true now that operand, operator metrics, and trust-manager webhook TLS are wired.
Keep the bundled ClusterServiceVersion aligned with the manifests base tls-profiles feature claim.
Operator cmd imports k8s.io/apiserver/pkg/server for ServingInfo TLS wiring; tidy expects it as a direct require so verify-deps stays clean.
Expand ResolveHonoredTLSProfile and trust-manager TLS tests for adherence, error propagation, and serving guards; clarify ObjectGetter matches CtrlClient.
473a6f6 to
01c94a9
Compare
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
|
/test tls-scanner |
|
@arun717: The following test 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. |
2 participants
Summary
:8443)tls-profilesfeature annotationTest plan
/test tls-scanner/test e2e-operator-tech-preview(TLS profile e2e)/test unit/test verifySummary by CodeRabbit
New Features
Bug Fixes