USHIFT-6596: introduce trust-manager operand to the cert-manager#6373
USHIFT-6596: introduce trust-manager operand to the cert-manager#6373eslutsky wants to merge 3 commits intoopenshift:mainfrom
Conversation
WalkthroughCert-manager bumped to v1.19.2 with extensive CRD OpenAPI schema updates (list-type/map annotations, issuerRef defaults, RFC2136 protocol, HTTP01 pod resources). Two new CRDs (Bundle, TrustManager) added. Images, kustomizations, RBAC, packaging, and a new Trusted CABundle manager controller were introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant ServiceManager
participant TrustedCABundleManager
participant KubeAPI
participant FileSystem
ServiceManager->>TrustedCABundleManager: Start()
TrustedCABundleManager->>FileSystem: Read /etc/pki/.../tls-ca-bundle.pem
FileSystem-->>TrustedCABundleManager: bundle contents
TrustedCABundleManager->>KubeAPI: List ConfigMaps(label=inject-trusted-cabundle=true)
KubeAPI-->>TrustedCABundleManager: ConfigMap list
TrustedCABundleManager->>KubeAPI: Update ConfigMaps' data.ca-bundle.crt (if changed)
KubeAPI-->>TrustedCABundleManager: Update results
TrustedCABundleManager->>FileSystem: Watch file + directory (fsnotify)
FileSystem-->>TrustedCABundleManager: Write/Create events
TrustedCABundleManager->>KubeAPI: Re-sync on events/periodic tick (only if hash changed)
TrustedCABundleManager-->>ServiceManager: Mark ready / handle shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/apparentlymart/go-cidr@v1.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20190321100706-95778dfbb74e: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/miekg/dns@v1.1.63: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/api@v0.0.0-20260309155933-45fd88d185dd: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/build-machinery-go@v0.0.0-20251023084048-5d77c1a5e5af: is explicitly required in go.mod, but not marked as explicit ... [truncated 29518 characters] ... belet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky 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
eslutsky
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@eslutsky: This pull request references USHIFT-6596 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. |
|
@eslutsky: This pull request references USHIFT-6596 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml`:
- Around line 526-531: The CRD schema for the RFC2136 "protocol" field claims a
default but doesn't declare it; update the "protocol" schema entry (the mapping
that contains description, enum and type) to include the key default: UDP so
CRD-level defaulting matches the description (keep the existing enum/TCP/UDP and
type: string), or alternatively remove the "(default)" text from the
description—prefer adding default: UDP to the "protocol" field to be explicit.
In `@assets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yaml`:
- Around line 1016-1024: The descriptions for issuerRef.group and issuerRef.kind
state default values but the CRD schema lacks default entries; either add schema
defaults (add default: "cert-manager.io" under the issuerRef.group schema and
default: "Issuer" under the issuerRef.kind schema) so the contract matches the
docs, or remove the "Defaults to ..." text from those description fields if no
defaulting is applied elsewhere; update the issuerRef.group and issuerRef.kind
schema nodes accordingly and keep the description consistent with the chosen
approach.
In `@assets/optional/cert-manager/manager/images-x86_64.yaml`:
- Line 13: The entry cert-manager-trust-manager currently uses an unpinned
upstream tag (quay.io/jetstack/trust-manager:v0.20.3) which breaks the
digest-pinning pattern; either replace that value with a digest-pinned image
from the Red Hat mirror (registry.redhat.io@sha256:<digest>) matching the same
version or, if a Red Hat digest is unavailable, update images-x86_64.yaml to use
a sha256 digest from quay.io (quay.io/jetstack/trust-manager@sha256:<digest>)
and add an inline comment explaining why this image is an upstream exception
(air-gap/mirroring constraints) so the divergence from
registry.redhat.io@sha256: for other images is explicit and reproducible.
In `@assets/optional/cert-manager/manager/kustomization.yaml`:
- Around line 24-39: The strategic merge patch adds a duplicate
RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER env var for the controller-manager
Deployment/container (name: cert-manager-operator); remove this redundant block
so the env var is only set once (keep the existing JSON6902 patch that already
adds RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER) by deleting the strategic-merge
patch targeting Deployment/controller-manager (container name
cert-manager-operator) or consolidate the two patches into a single patch that
defines the env var exactly once.
- Around line 12-22: The JSON6902 patch entries under the patch block have
incorrect indentation for the mapping keys (op/path/value); fix each operation
so that op:, path:, and value: are aligned as sibling keys (no extra leading
spaces) and ensure the value mapping for the env entries correctly nests name:
and value: under value:, e.g. the two ops that add env vars
UNSUPPORTED_ADDON_FEATURES and RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER should
be proper YAML mappings with consistent indentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 64b48416-6b32-4c17-b2dc-46dcc5bb7cdb
📒 Files selected for processing (18)
assets/optional/cert-manager/crd/bases/certificaterequests.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/certificates.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlassets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlassets/optional/cert-manager/crd/bases/orders.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/kustomization.yamlassets/optional/cert-manager/manager/images-aarch64.yamlassets/optional/cert-manager/manager/images-x86_64.yamlassets/optional/cert-manager/manager/kustomization.yamlassets/optional/cert-manager/manager/manager.yamlassets/optional/cert-manager/rbac/role.yamlpackaging/rpm/microshift.specscripts/auto-rebase/assets_cert_manager.yaml
| protocol: | ||
| description: Protocol to use for dynamic DNS update queries. Valid values are (case-sensitive) ``TCP`` and ``UDP``; ``UDP`` (default). | ||
| enum: | ||
| - TCP | ||
| - UDP | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
LOCAL_FILE="assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml"
UPSTREAM_URL="https://github.com/cert-manager/cert-manager/releases/download/v1.19.2/cert-manager.crds.yaml"
echo "Local RFC2136 protocol block:"
sed -n '518,531p' "$LOCAL_FILE"
echo
echo "Upstream RFC2136 protocol block:"
curl -fsSL "$UPSTREAM_URL" \
| awk 'BEGIN{RS="---"; ORS=""} /name: issuers.cert-manager.io/ {print}' \
| sed -n '/rfc2136:/,/tsigAlgorithm:/p'Repository: openshift/microshift
Length of output: 1098
🏁 Script executed:
#!/bin/bash
set -euo pipefail
LOCAL_FILE="assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml"
UPSTREAM_URL="https://github.com/cert-manager/cert-manager/releases/download/v1.19.2/cert-manager.crds.yaml"
echo "=== Local file: searching for 'protocol:' near 'rfc2136' ==="
grep -n "rfc2136" "$LOCAL_FILE" || echo "rfc2136 not found"
echo
echo "=== Local protocol field and context ==="
sed -n '520,532p' "$LOCAL_FILE"
echo
echo "=== Local: checking for 'default:' in rfc2136 section ==="
sed -n '500,550p' "$LOCAL_FILE" | grep -E "(rfc2136|protocol:|default:)" || echo "No default: found"
echo
echo "=== Upstream file: search for rfc2136 ==="
curl -fsSL "$UPSTREAM_URL" \
| grep -A 20 "rfc2136:" | head -30 || echo "rfc2136 not found in upstream"Repository: openshift/microshift
Length of output: 3521
Schema default for RFC2136 protocol is undeclared.
The protocol field description claims (default) but the schema lacks a default: UDP key. This matches upstream v1.19.2, so no drift occurred—but the ambiguity remains: CRD-level defaulting won't apply unless you add default: UDP to the schema. Since cert-manager's code likely handles the fallback, either add the schema key for completeness or remove the (default) text to avoid misleading API consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml`
around lines 526 - 531, The CRD schema for the RFC2136 "protocol" field claims a
default but doesn't declare it; update the "protocol" schema entry (the mapping
that contains description, enum and type) to include the key default: UDP so
CRD-level defaulting matches the description (keep the existing enum/TCP/UDP and
type: string), or alternatively remove the "(default)" text from the
description—prefer adding default: UDP to the "protocol" field to be explicit.
| description: |- | ||
| Group of the issuer being referred to. | ||
| Defaults to 'cert-manager.io'. | ||
| type: string | ||
| kind: | ||
| description: Kind of the resource being referred to. | ||
| description: |- | ||
| Kind of the issuer being referred to. | ||
| Defaults to 'Issuer'. | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm schema currently lacks explicit defaults in this CRD.
rg -n -C2 'issuerRef:|group:|kind:|default:' assets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yaml
# 2) Search for IstioCSR defaulters/mutating logic in Go sources.
rg -n --type=go -C3 'type\s+IstioCSR\b|func\s*\(.*\)\s*Default\s*\(|issuerRef|cert-manager\.io|ClusterIssuer|Issuer'
# Expected:
# - If no defaulting code is found and no schema defaults exist, docs should not claim defaults.
# - If defaulting exists, keep docs and consider linking/defaulting behavior explicitly in API docs.Repository: openshift/microshift
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search specifically for IstioCSR defaulting implementations
echo "=== Searching for IstioCSR Default() method implementations ==="
rg -n 'func.*\(.*IstioCSR.*\).*Default' --type=go 2>/dev/null || echo "No IstioCSR Default methods found"
echo -e "\n=== Searching for istio-csr issuerRef defaulting logic ==="
rg -n 'issuerRef' --type=go -A5 -B5 | head -100
echo -e "\n=== Checking if IstioCSR is in vendor or pkg subdirectories ==="
find . -type f -name "*.go" | xargs grep -l "type IstioCSR struct" 2>/dev/null | head -20Repository: openshift/microshift
Length of output: 221
Default values are documented but not declared in schema.
issuerRef.group and issuerRef.kind claim "Defaults to …", yet this CRD segment has no default: schema fields for either—other nearby fields explicitly show default: values. No defaulting code exists in the codebase. This creates a broken API contract that will confuse users.
Add default: cert-manager.io for group and default: Issuer for kind to the schema if these values are actually applied elsewhere; otherwise, remove the "Defaults to" language from descriptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yaml`
around lines 1016 - 1024, The descriptions for issuerRef.group and
issuerRef.kind state default values but the CRD schema lacks default entries;
either add schema defaults (add default: "cert-manager.io" under the
issuerRef.group schema and default: "Issuer" under the issuerRef.kind schema) so
the contract matches the docs, or remove the "Defaults to ..." text from those
description fields if no defaulting is applied elsewhere; update the
issuerRef.group and issuerRef.kind schema nodes accordingly and keep the
description consistent with the chosen approach.
| patch: |- | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/env/- | ||
| value: | ||
| name: UNSUPPORTED_ADDON_FEATURES | ||
| value: '' | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/env/- | ||
| value: | ||
| name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER | ||
| value: '' |
There was a problem hiding this comment.
Invalid YAML indentation in JSON patch.
The path and value keys have extra leading spaces that break YAML structure. In a JSON6902 patch list, each operation should have consistent indentation:
🐛 Proposed fix
patch: |-
- op: add
- path: /spec/template/spec/containers/0/env/-
- value:
- name: UNSUPPORTED_ADDON_FEATURES
- value: ''
+ path: /spec/template/spec/containers/0/env/-
+ value:
+ name: UNSUPPORTED_ADDON_FEATURES
+ value: ''
- op: add
- path: /spec/template/spec/containers/0/env/-
- value:
- name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER
- value: ''
+ path: /spec/template/spec/containers/0/env/-
+ value:
+ name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER
+ value: ''📝 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.
| patch: |- | |
| - op: add | |
| path: /spec/template/spec/containers/0/env/- | |
| value: | |
| name: UNSUPPORTED_ADDON_FEATURES | |
| value: '' | |
| - op: add | |
| path: /spec/template/spec/containers/0/env/- | |
| value: | |
| name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER | |
| value: '' | |
| patch: |- | |
| - op: add | |
| path: /spec/template/spec/containers/0/env/- | |
| value: | |
| name: UNSUPPORTED_ADDON_FEATURES | |
| value: '' | |
| - op: add | |
| path: /spec/template/spec/containers/0/env/- | |
| value: | |
| name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER | |
| value: '' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/manager/kustomization.yaml` around lines 12 -
22, The JSON6902 patch entries under the patch block have incorrect indentation
for the mapping keys (op/path/value); fix each operation so that op:, path:, and
value: are aligned as sibling keys (no extra leading spaces) and ensure the
value mapping for the env entries correctly nests name: and value: under value:,
e.g. the two ops that add env vars UNSUPPORTED_ADDON_FEATURES and
RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER should be proper YAML mappings with
consistent indentation.
| - target: | ||
| kind: Deployment | ||
| name: controller-manager | ||
| patch: |- | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: controller-manager | ||
| spec: | ||
| template: | ||
| spec: | ||
| containers: | ||
| - name: cert-manager-operator | ||
| env: | ||
| - name: RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER | ||
| value: "" |
There was a problem hiding this comment.
Duplicate patch for RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER.
This strategic merge patch (lines 24-39) adds the same RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER env var that the JSON6902 patch above already adds. This creates a redundant definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/manager/kustomization.yaml` around lines 24 -
39, The strategic merge patch adds a duplicate
RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER env var for the controller-manager
Deployment/container (name: cert-manager-operator); remove this redundant block
so the env var is only set once (keep the existing JSON6902 patch that already
adds RELATED_IMAGE_CERT_MANAGER_TRUST_MANAGER) by deleting the strategic-merge
patch targeting Deployment/controller-manager (container name
cert-manager-operator) or consolidate the two patches into a single patch that
defines the env var exactly once.
fafb715 to
fb94da3
Compare
|
/test all |
|
@eslutsky: 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. |
| cert-manager-istiocsr: registry.redhat.io/cert-manager/cert-manager-istio-csr-rhel9@sha256:78564158857ed7f1534597f3ee7faaee23b692f9ed5aa0259e19196f5400167d | ||
| controller: registry.redhat.io/cert-manager/cert-manager-operator-rhel9@sha256:9f37a838089d2e3c199a4fd97bea028cb6b66b4214e8233884716efe0a998298 | ||
| cert-manager-trust-manager: quay.io/jetstack/trust-manager:v0.20.3 | ||
| unsupported-addon-features: TrustManager=true No newline at end of file |
There was a problem hiding this comment.
Instead of hardcoding at the product level, we need to make this user configurable.
| cert-manager-acmesolver: registry.redhat.io/cert-manager/jetstack-cert-manager-acmesolver-rhel9@sha256:b001612ad6362a1b7273e178d9870ab3d020a40ee3339a0f95fef1cd3913356e | ||
| cert-manager-istiocsr: registry.redhat.io/cert-manager/cert-manager-istio-csr-rhel9@sha256:78564158857ed7f1534597f3ee7faaee23b692f9ed5aa0259e19196f5400167d | ||
| controller: registry.redhat.io/cert-manager/cert-manager-operator-rhel9@sha256:9f37a838089d2e3c199a4fd97bea028cb6b66b4214e8233884716efe0a998298 | ||
| cert-manager-trust-manager: quay.io/jetstack/trust-manager:v0.20.3 |
There was a problem hiding this comment.
It should be from registry.redhat.io
| controller: registry.redhat.io/cert-manager/cert-manager-operator-rhel9@sha256:8eecb53c8c81c0fa8f198260acb273759c0d12b5c01642a128923c5716df19d2 | ||
| cert-manager-trust-manager: quay.io/jetstack/trust-manager:v0.20.3 | ||
| unsupported-addon-features: TrustManager=true No newline at end of file |
There was a problem hiding this comment.
Wondering why TrustManager image details are not listed here.
openshift-ci
Bot
added
the
needs-rebase
|
@eslutsky: This pull request references USHIFT-6596 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 spike to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" 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. |
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
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yaml`:
- Around line 123-159: Remove the unsupported fileKeyRef schema from the
CertManager CRD: delete the fileKeyRef property definition (the object with keys
key, optional, path, volumeName, required, type, x-kubernetes-map-type) so
overrideEnv[*].valueFrom.fileKeyRef is not accepted by the CRD; also remove any
other fileKeyRef definitions elsewhere in the same CRD document where
overrideEnv or valueFrom schemas reference it so the CertManager spec cannot
include fileKeyRef fields the operator cannot materialize.
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yaml`:
- Around line 1134-1164: The authorizedSecrets allowlist currently matches only
secret basenames (authorizedSecrets and policy, and its
x-kubernetes-validations), which unintentionally permits that name in every
namespace; change authorizedSecrets to be namespace-scoped by requiring entries
in "namespace/name" form (or make authorizedSecrets an array of objects with
properties namespace and name), update the description to state the new format,
and adjust the x-kubernetes-validations rules to validate the new format (e.g.,
enforce pattern ^[^/]+/[^/]+$ for string entries or validate presence of both
namespace and name fields if using objects) and update the rules that check
has(self.authorizedSecrets) and size(...) accordingly so authorization is
properly scoped.
- Around line 1209-1222: The trustNamespace CRD field currently only has min/max
length checks; add a DNS-1123 label validation so values must be valid
Kubernetes namespace names by adding an x-kubernetes-validations rule on
trustNamespace that enforces the regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ (and keep
maxLength: 63); ensure this new validation is added alongside the existing
immutability validation (the existing x-kubernetes-validations array for
trustNamespace), so the field is both immutable and must match the
namespace/name pattern.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: defb795e-c0b1-4e2b-a8c2-ee569533971d
📒 Files selected for processing (18)
assets/optional/cert-manager/crd/bases/certificaterequests.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/certificates.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlassets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlassets/optional/cert-manager/crd/bases/orders.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/kustomization.yamlassets/optional/cert-manager/manager/images-aarch64.yamlassets/optional/cert-manager/manager/images-x86_64.yamlassets/optional/cert-manager/manager/kustomization.yamlassets/optional/cert-manager/manager/manager.yamlassets/optional/cert-manager/rbac/role.yamlpackaging/rpm/microshift.specscripts/auto-rebase/assets_cert_manager.yaml
✅ Files skipped from review due to trivial changes (5)
- assets/optional/cert-manager/crd/kustomization.yaml
- scripts/auto-rebase/assets_cert_manager.yaml
- packaging/rpm/microshift.spec
- assets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yaml
- assets/optional/cert-manager/manager/manager.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- assets/optional/cert-manager/manager/images-aarch64.yaml
- assets/optional/cert-manager/manager/kustomization.yaml
- assets/optional/cert-manager/manager/images-x86_64.yaml
- assets/optional/cert-manager/rbac/role.yaml
- assets/optional/cert-manager/crd/bases/certificaterequests.cert-manager.io-crd.yaml
- assets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml
- assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml
| fileKeyRef: | ||
| description: |- | ||
| FileKeyRef selects a key of the env file. | ||
| Requires the EnvFiles feature gate to be enabled. | ||
| properties: | ||
| key: | ||
| description: |- | ||
| The key within the env file. An invalid key will prevent the pod from starting. | ||
| The keys defined within a source may consist of any printable ASCII characters except '='. | ||
| During Alpha stage of the EnvFiles feature gate, the key size is limited to 128 characters. | ||
| type: string | ||
| optional: | ||
| default: false | ||
| description: |- | ||
| Specify whether the file or its key must be defined. If the file or key | ||
| does not exist, then the env var is not published. | ||
| If optional is set to true and the specified key does not exist, | ||
| the environment variable will not be set in the Pod's containers. | ||
|
|
||
| If optional is set to false and the specified key does not exist, | ||
| an error will be returned during Pod creation. | ||
| type: boolean | ||
| path: | ||
| description: |- | ||
| The path within the volume from which to select the file. | ||
| Must be relative and may not contain the '..' path or start with '..'. | ||
| type: string | ||
| volumeName: | ||
| description: The name of the volume mount containing | ||
| the env file. | ||
| type: string | ||
| required: | ||
| - key | ||
| - path | ||
| - volumeName | ||
| type: object | ||
| x-kubernetes-map-type: atomic |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Kubernetes/OpenShift version targeted by MicroShift main as of March 2026, is EnvVarSource.fileKeyRefsupported for Pod env vars, and does it require theEnvFiles feature gate?
💡 Result:
No, EnvVarSource.fileKeyRef is not supported in the Kubernetes/OpenShift version targeted by MicroShift main as of March 2026. MicroShift main branch in March 2026 targets OpenShift 4.18 (evidenced by releases like 4.18.36-202603201457.p0 on 2026-03-26), which uses Kubernetes 1.31. The fileKeyRef field (part of the EnvFiles feature) was introduced in alpha in Kubernetes v1.34 and promoted to beta (enabled by default) in v1.35. Since OpenShift 4.18 uses Kubernetes 1.31, which predates v1.34, the field is not present in the API and thus not supported. OpenShift, being conservative with upstream features, would not backport this alpha/beta feature. The EnvFiles feature gate is therefore irrelevant as the capability does not exist in 1.31.
Citations:
- 1: https://github.com/openshift/microshift/releases/tag/4.18.36-202603201457.p0
- 2: https://github.com/openshift/microshift
- 3: https://docs.redhat.com/documentation/red_hat_build_of_microshift/4.18/html-single/red_hat_build_of_microshift_release_notes
- 4: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/release_notes/ocp-4-18-release-notes
- 5: https://docs.openshift.com/container-platform/4.18/release_notes/ocp-4-18-release-notes.html
- 6: 238410b
- 7: KEP-3721: Support for env files kubernetes/kubernetes#132626
- 8: KEP-3721: Promote EnvFiles feature gate to Beta kubernetes/kubernetes#134414
- 9: https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/
- 10: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/3721-support-for-env-files/README.md
Remove fileKeyRef from CRD—not supported in targeted Kubernetes version.
The CRD now accepts overrideEnv[*].valueFrom.fileKeyRef, but OpenShift 4.18 (Kubernetes 1.31) does not support this field. It was introduced in Kubernetes v1.34 and promoted to beta in v1.35. Users will be able to create a CertManager spec that the operator cannot materialize into a valid Deployment.
Also applies to lines 404-440, 999-1035.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yaml`
around lines 123 - 159, Remove the unsupported fileKeyRef schema from the
CertManager CRD: delete the fileKeyRef property definition (the object with keys
key, optional, path, volumeName, required, type, x-kubernetes-map-type) so
overrideEnv[*].valueFrom.fileKeyRef is not accepted by the CRD; also remove any
other fileKeyRef definitions elsewhere in the same CRD document where
overrideEnv or valueFrom schemas reference it so the CertManager spec cannot
include fileKeyRef fields the operator cannot materialize.
| authorizedSecrets: | ||
| description: |- | ||
| authorizedSecrets is a list of specific secret names that trust-manager | ||
| is authorized to create and update. This field is only valid when policy is "Custom". | ||
| items: | ||
| minLength: 1 | ||
| type: string | ||
| minItems: 0 | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| policy: | ||
| default: Disabled | ||
| description: |- | ||
| policy controls whether and how trust-manager can write trust bundles to Secrets. | ||
| Allowed values are "Disabled" or "Custom". | ||
| "Disabled" means trust-manager cannot write trust bundles to Secrets (default behavior). | ||
| "Custom" grants trust-manager permission to create and update only the secrets listed in authorizedSecrets. | ||
| enum: | ||
| - Disabled | ||
| - Custom | ||
| type: string | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: authorizedSecrets must not be empty when policy is | ||
| Custom | ||
| rule: self.policy != 'Custom' || (has(self.authorizedSecrets) | ||
| && size(self.authorizedSecrets) > 0) | ||
| - message: authorizedSecrets must be empty when policy is not | ||
| Custom | ||
| rule: self.policy == 'Custom' || !has(self.authorizedSecrets) | ||
| || size(self.authorizedSecrets) == 0 |
There was a problem hiding this comment.
Scope authorizedSecrets by namespace and name, not name alone.
This allowlist only keys on Secret basename. For a cluster-wide bundle distributor, authorizing ca-bundle here effectively authorizes every ca-bundle Secret in every namespace, which is broader than “specific secret names” suggests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yaml`
around lines 1134 - 1164, The authorizedSecrets allowlist currently matches only
secret basenames (authorizedSecrets and policy, and its
x-kubernetes-validations), which unintentionally permits that name in every
namespace; change authorizedSecrets to be namespace-scoped by requiring entries
in "namespace/name" form (or make authorizedSecrets an array of objects with
properties namespace and name), update the description to state the new format,
and adjust the x-kubernetes-validations rules to validate the new format (e.g.,
enforce pattern ^[^/]+/[^/]+$ for string entries or validate presence of both
namespace and name fields if using objects) and update the rules that check
has(self.authorizedSecrets) and size(...) accordingly so authorization is
properly scoped.
| trustNamespace: | ||
| default: cert-manager | ||
| description: |- | ||
| trustNamespace is the namespace where trust-manager looks for trust sources | ||
| (ConfigMaps and Secrets containing CA certificates). | ||
| Defaults to "cert-manager" if not specified. | ||
| This field is immutable once set. | ||
| This field can have a maximum of 63 characters. | ||
| maxLength: 63 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: trustNamespace is immutable once set | ||
| rule: oldSelf == '' || self == oldSelf |
There was a problem hiding this comment.
Validate trustNamespace as a real namespace name.
maxLength/minLength are not enough here. Right now the CRD accepts values that are not valid Kubernetes namespace names, so an invalid CR can be stored and only fail later when the operator uses it.
Proposed fix
trustNamespace:
default: cert-manager
description: |-
trustNamespace is the namespace where trust-manager looks for trust sources
(ConfigMaps and Secrets containing CA certificates).
Defaults to "cert-manager" if not specified.
This field is immutable once set.
This field can have a maximum of 63 characters.
maxLength: 63
minLength: 1
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
type: string
x-kubernetes-validations:
- message: trustNamespace is immutable once set
rule: oldSelf == '' || self == oldSelf📝 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.
| trustNamespace: | |
| default: cert-manager | |
| description: |- | |
| trustNamespace is the namespace where trust-manager looks for trust sources | |
| (ConfigMaps and Secrets containing CA certificates). | |
| Defaults to "cert-manager" if not specified. | |
| This field is immutable once set. | |
| This field can have a maximum of 63 characters. | |
| maxLength: 63 | |
| minLength: 1 | |
| type: string | |
| x-kubernetes-validations: | |
| - message: trustNamespace is immutable once set | |
| rule: oldSelf == '' || self == oldSelf | |
| trustNamespace: | |
| default: cert-manager | |
| description: |- | |
| trustNamespace is the namespace where trust-manager looks for trust sources | |
| (ConfigMaps and Secrets containing CA certificates). | |
| Defaults to "cert-manager" if not specified. | |
| This field is immutable once set. | |
| This field can have a maximum of 63 characters. | |
| maxLength: 63 | |
| minLength: 1 | |
| pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ | |
| type: string | |
| x-kubernetes-validations: | |
| - message: trustNamespace is immutable once set | |
| rule: oldSelf == '' || self == oldSelf |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yaml`
around lines 1209 - 1222, The trustNamespace CRD field currently only has
min/max length checks; add a DNS-1123 label validation so values must be valid
Kubernetes namespace names by adding an x-kubernetes-validations rule on
trustNamespace that enforces the regex ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ (and keep
maxLength: 63); ensure this new validation is added alongside the existing
immutability validation (the existing x-kubernetes-validations array for
trustNamespace), so the field is both immutable and must match the
namespace/name pattern.
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
cert-manager-trustmanager
branch
from
543eca8 to
3be46c4
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yaml (1)
1352-1365:⚠️ Potential issue | 🟡 MinorKeep the anti-affinity scoring text consistent.
These blocks now say the scheduler subtracts weight, but the unchanged item description below still says matched weights are added per-node, and the parallel schema in
assets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yaml:1136-1140uses the same “added” wording. This will surface inconsistent docs inoc explain.📝 Suggested wording fix
- compute a sum by iterating through the elements of this field and subtracting - "weight" from the sum if the node has pods which matches the corresponding podAffinityTerm; the + compute a sum by iterating through the elements of this field and adding + "weight" to the sum if the node has pods which matches the corresponding podAffinityTerm; theAlso applies to: 2563-2576
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yaml` around lines 1352 - 1365, The anti-affinity description under preferredDuringSchedulingIgnoredDuringExecution is inconsistent: it currently says the scheduler "subtracts" weight but the items description and other CRDs (e.g., the WeightedPodAffinityTerm/items description and the parallel schema in clusterissuers.crd) state weights are "added." Edit the paragraph in the preferredDuringSchedulingIgnoredDuringExecution description so it consistently says weights are "added" (or otherwise match the wording used in the items description and WeightedPodAffinityTerm/podAffinityTerm schema) and ensure the same change is applied to the other occurrence noted (around lines 2563-2576) to keep oc explain output consistent.
♻️ Duplicate comments (1)
assets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yaml (1)
123-159:⚠️ Potential issue | 🟠 Major
fileKeyRefis still an unsupported API surface for the target platform.This remains the same compatibility concern flagged earlier: the CRD accepts
overrideEnv[*].valueFrom.fileKeyRef, but target clusters may reject/ignore it at workload API level, causing CRs that the operator cannot safely materialize.Suggested fix
- fileKeyRef: - description: |- - FileKeyRef selects a key of the env file. - Requires the EnvFiles feature gate to be enabled. - properties: - key: - ... - optional: - ... - path: - ... - volumeName: - ... - required: - - key - - path - - volumeName - type: object - x-kubernetes-map-type: atomicApply this removal in all three
overrideEnv[].valueFromsections.#!/bin/bash set -euo pipefail echo "Checking EnvVarSource fields in Kubernetes API tags..." for v in v0.31.0 v0.34.0 v0.35.0; do echo "" echo "=== kubernetes/api ${v} ===" curl -fsSL "https://raw.githubusercontent.com/kubernetes/api/${v}/core/v1/types.go" \ | awk '/type EnvVarSource struct {/,/^}/' \ | rg -n 'ConfigMapKeyRef|SecretKeyRef|FieldRef|ResourceFieldRef|FileKeyRef' || true done echo "" echo "Expected:" echo "- v0.31.0: FileKeyRef absent" echo "- newer tags (where EnvFiles exists): FileKeyRef present"Also applies to: 404-440, 999-1035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yaml` around lines 123 - 159, The CRD exposes an unsupported fileKeyRef field under fileKeyRef and overrideEnv[].valueFrom.fileKeyRef which target clusters may reject; remove fileKeyRef from all EnvVarSource-like blocks (the fileKeyRef property and its required entries) so overrideEnv[].valueFrom only uses supported fields (e.g., configMapKeyRef/secretKeyRef/fieldRef/resourceFieldRef) and update the required list where fileKeyRef is referenced; search for the symbol fileKeyRef and each overrideEnv[].valueFrom section in the CRD and delete the fileKeyRef property block and any mention of it in the corresponding required arrays across all three occurrences mentioned.
🧹 Nitpick comments (1)
assets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yaml (1)
1473-1477: Keep the anti-affinity docs consistent.The parent description now says matched terms subtract weight, but the nested
WeightedPodAffinityTermtext still says weights are added per-node. That leaveskubectl explainwith contradictory guidance.Also applies to: 2684-2688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yaml` around lines 1473 - 1477, The nested description for WeightedPodAffinityTerm contradicts the parent: update the text under items for "WeightedPodAffinityTerm" to mirror the parent description (explain that the weights are subtracted from a per-node sum when a node has matching pods and that nodes with the highest resulting sum are most preferred), and make the same edit for the other duplicate occurrence of WeightedPodAffinityTerm in the file so both places read consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/optional/cert-manager/rbac/role.yaml`:
- Line 230: Remove the incorrect RBAC rules that grant access to the Bundle
resource under the operator.openshift.io API group and instead ensure Bundle
permissions are only declared under the trust.cert-manager.io group;
specifically, delete the entries referencing "bundles" in operator.openshift.io
(the three occurrences noted) and rely on the existing rule that already grants
Bundle access under trust.cert-manager.io, keeping only the correct rule and
removing the duplicate/incorrect operator.openshift.io lines.
---
Outside diff comments:
In
`@assets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yaml`:
- Around line 1352-1365: The anti-affinity description under
preferredDuringSchedulingIgnoredDuringExecution is inconsistent: it currently
says the scheduler "subtracts" weight but the items description and other CRDs
(e.g., the WeightedPodAffinityTerm/items description and the parallel schema in
clusterissuers.crd) state weights are "added." Edit the paragraph in the
preferredDuringSchedulingIgnoredDuringExecution description so it consistently
says weights are "added" (or otherwise match the wording used in the items
description and WeightedPodAffinityTerm/podAffinityTerm schema) and ensure the
same change is applied to the other occurrence noted (around lines 2563-2576) to
keep oc explain output consistent.
---
Duplicate comments:
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yaml`:
- Around line 123-159: The CRD exposes an unsupported fileKeyRef field under
fileKeyRef and overrideEnv[].valueFrom.fileKeyRef which target clusters may
reject; remove fileKeyRef from all EnvVarSource-like blocks (the fileKeyRef
property and its required entries) so overrideEnv[].valueFrom only uses
supported fields (e.g., configMapKeyRef/secretKeyRef/fieldRef/resourceFieldRef)
and update the required list where fileKeyRef is referenced; search for the
symbol fileKeyRef and each overrideEnv[].valueFrom section in the CRD and delete
the fileKeyRef property block and any mention of it in the corresponding
required arrays across all three occurrences mentioned.
---
Nitpick comments:
In
`@assets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yaml`:
- Around line 1473-1477: The nested description for WeightedPodAffinityTerm
contradicts the parent: update the text under items for
"WeightedPodAffinityTerm" to mirror the parent description (explain that the
weights are subtracted from a per-node sum when a node has matching pods and
that nodes with the highest resulting sum are most preferred), and make the same
edit for the other duplicate occurrence of WeightedPodAffinityTerm in the file
so both places read consistently.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ee12fac-e029-4de1-9202-aa97d648fe98
📒 Files selected for processing (18)
assets/optional/cert-manager/crd/bases/certificaterequests.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/certificates.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/challenges.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.ymlassets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_certmanagers.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yamlassets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlassets/optional/cert-manager/crd/bases/orders.acme.cert-manager.io-crd.yamlassets/optional/cert-manager/crd/kustomization.yamlassets/optional/cert-manager/manager/images-aarch64.yamlassets/optional/cert-manager/manager/images-x86_64.yamlassets/optional/cert-manager/manager/kustomization.yamlassets/optional/cert-manager/manager/manager.yamlassets/optional/cert-manager/rbac/role.yamlpackaging/rpm/microshift.specscripts/auto-rebase/assets_cert_manager.yaml
✅ Files skipped from review due to trivial changes (3)
- assets/optional/cert-manager/crd/kustomization.yaml
- scripts/auto-rebase/assets_cert_manager.yaml
- assets/optional/cert-manager/manager/manager.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- packaging/rpm/microshift.spec
- assets/optional/cert-manager/manager/kustomization.yaml
- assets/optional/cert-manager/crd/bases/operator.openshift.io_istiocsrs.yaml
- assets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml
- assets/optional/cert-manager/crd/bases/issuers.cert-manager.io-crd.yaml
- assets/optional/cert-manager/manager/images-x86_64.yaml
- assets/optional/cert-manager/manager/images-aarch64.yaml
| - certmanagers/finalizers | ||
| - istiocsrs/finalizers | ||
| - trustmanagers/finalizers | ||
| - bundles/finalizers |
There was a problem hiding this comment.
Move Bundle RBAC out of operator.openshift.io.
Bundle is defined under trust.cert-manager.io, not operator.openshift.io (assets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml). These entries do not grant anything useful here, and the dedicated rule below already covers Bundle access in the correct API group.
🧹 Suggested cleanup
- apiGroups:
- operator.openshift.io
resources:
- certmanagers/finalizers
- istiocsrs/finalizers
- trustmanagers/finalizers
- - bundles/finalizers
verbs:
- update
- apiGroups:
- operator.openshift.io
resources:
- certmanagers/status
- istiocsrs/status
- trustmanagers/status
- - bundles/status
verbs:
- get
- patch
- update
- apiGroups:
- operator.openshift.io
resources:
- istiocsrs
- trustmanagers
- - bundles
verbs:
- get
- list
- patch
- update
- watchAlso applies to: 239-240, 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/rbac/role.yaml` at line 230, Remove the
incorrect RBAC rules that grant access to the Bundle resource under the
operator.openshift.io API group and instead ensure Bundle permissions are only
declared under the trust.cert-manager.io group; specifically, delete the entries
referencing "bundles" in operator.openshift.io (the three occurrences noted) and
rely on the existing rule that already grants Bundle access under
trust.cert-manager.io, keeping only the correct rule and removing the
duplicate/incorrect operator.openshift.io lines.
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
cert-manager-trustmanager
branch
from
3be46c4 to
c23bdcc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/controllers/trusted-ca-bundle-manager.go`:
- Around line 82-84: The select branch currently returns ctx.Err() when
<-ctx.Done() fires; change this to return nil to signal a clean, expected
shutdown instead of surfacing timeouts (ctx.Err()). Locate the select handling
with the ctx.Done() case (the branch that currently does "return ctx.Err()") in
trusted-ca-bundle-manager.go and replace the return value so the function
returns nil on expected shutdown, ensuring the ServiceManager doesn't treat
context.DeadlineExceeded as a service failure.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2382fdf3-934c-4316-9ba6-bf5e79dcb9e1
📒 Files selected for processing (5)
assets/optional/cert-manager/manager/images-x86_64.yamlassets/optional/cert-manager/manager/images.yamlassets/optional/cert-manager/release-cert-manager-x86_64.jsonpkg/cmd/run.gopkg/controllers/trusted-ca-bundle-manager.go
✅ Files skipped from review due to trivial changes (2)
- assets/optional/cert-manager/manager/images.yaml
- assets/optional/cert-manager/release-cert-manager-x86_64.json
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/optional/cert-manager/manager/images-x86_64.yaml
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify how ServiceManager treats non-nil Run() returns.
rg -n -C4 'ErrChan|runService|return err|service error' pkg/servicemanager --type go
# Compare shutdown patterns in other controller Run() implementations.
rg -n -C3 'case <-ctx\.Done\(\)|return ctx\.Err\(\)|return nil' pkg/controllers --type goRepository: openshift/microshift
Length of output: 25274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check ServiceManager's Run method and how it processes errors
rg -n -A20 'func.*Run.*ctx.*context' pkg/servicemanager/manager.go
# Search for any special handling of context errors in ServiceManager
rg -n 'context\.(Canceled|DeadlineExceeded)' pkg/servicemanager/ --type go
# Check if there's error wrapping or filtering
rg -n -B2 -A2 'errChan.*<-' pkg/servicemanager/ --type goRepository: openshift/microshift
Length of output: 3227
Return nil on expected shutdown instead of ctx.Err().
At line 83, ctx.Err() returns context.DeadlineExceeded if the context times out (not just context.Canceled). ServiceManager only filters context.Canceled at the error check, so timeout scenarios would incorrectly surface as service failures. Return nil for clean shutdown.
Suggested fix
- case <-ctx.Done():
- return ctx.Err()
+ case <-ctx.Done():
+ return nil📝 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.
| case <-ctx.Done(): | |
| return ctx.Err() | |
| case <-ctx.Done(): | |
| return nil | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/trusted-ca-bundle-manager.go` around lines 82 - 84, The
select branch currently returns ctx.Err() when <-ctx.Done() fires; change this
to return nil to signal a clean, expected shutdown instead of surfacing timeouts
(ctx.Err()). Locate the select handling with the ctx.Done() case (the branch
that currently does "return ctx.Err()") in trusted-ca-bundle-manager.go and
replace the return value so the function returns nil on expected shutdown,
ensuring the ServiceManager doesn't treat context.DeadlineExceeded as a service
failure.
3 participants
Introduces trust-manager as a new operand to the cert-manager operator, enabling management of trust bundles across the cluster.
When a
TrustManagerCR is created, the trust-manager operand will be deployed in the cert-manager namespace, enabling users to create and distribute trust bundles across the cluster usingBundleresources.This PR depends on openshift/cert-manager-operator#379
testing
PR_NUMBER=379 git clone https://github.com/openshift/cert-manager-operator.git cd cert-manager-operator/ git fetch origin refs/pull/${PR_NUMBER}/head:trust-manager git checkout trust-manager # build the operator locally as `openshift.io/cert-manager-operator:latest` make image-buildpatch the deployment with the local image
create trust-manager CR:
open-question
UNSUPPORTED_ADDON_FEATURESflagSummary by CodeRabbit
New Features
Updates