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 1 commit intoopenshift:mainfrom
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCert-manager manifests are updated from v1.18.4 to v1.19.2 with CRD schema refinements (list-type annotations, description clarifications for issuerRef defaults, new protocol and resources fields). Two new CRDs are introduced: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
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.
| cert-manager-acmesolver: registry.redhat.io/cert-manager/jetstack-cert-manager-acmesolver-rhel9@sha256:3a40eb434e032fb2135f369b6fc65266ae0f409db92fd65d12cf13c7f32b5020 | ||
| cert-manager-istiocsr: registry.redhat.io/cert-manager/cert-manager-istio-csr-rhel9@sha256:0f20d178f980a3fa4527bc1613b8639385e36c367e30c93b4cbb4c4a434d3405 | ||
| controller: registry.redhat.io/cert-manager/cert-manager-operator-rhel9@sha256:8eecb53c8c81c0fa8f198260acb273759c0d12b5c01642a128923c5716df19d2 | ||
| cert-manager-trust-manager: quay.io/jetstack/trust-manager:v0.20.3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's a Red Hat equivalent image for trust-manager
rg -n "trust-manager" --glob "*.json" --glob "*.yaml" --glob "*.yml" | head -30Repository: openshift/microshift
Length of output: 5122
🏁 Script executed:
#!/bin/bash
# Get the full content of both image files to understand the pattern
echo "=== images-x86_64.yaml ===" && cat -n assets/optional/cert-manager/manager/images-x86_64.yaml && \
echo -e "\n=== images-aarch64.yaml ===" && cat -n assets/optional/cert-manager/manager/images-aarch64.yamlRepository: openshift/microshift
Length of output: 2648
🏁 Script executed:
#!/bin/bash
# Search for any Red Hat registry entries for trust-manager or similar
rg "trust-manager|registry.redhat.io" assets/optional/cert-manager/ --type yamlRepository: openshift/microshift
Length of output: 8214
🏁 Script executed:
#!/bin/bash
# Check if there are any other references to quay.io in the cert-manager assets
rg "quay\.io" assets/optional/cert-manager/ --type yaml | head -20Repository: openshift/microshift
Length of output: 910
cert-manager-trust-manager breaks the digest-pinning pattern of other images.
All other images in this ConfigMap use registry.redhat.io with @sha256: digests; cert-manager-trust-manager alone uses a tag reference (quay.io/jetstack/trust-manager:v0.20.3). This inconsistency:
- Reduces reproducibility and versioning control
- May cause issues in air-gapped environments expecting
registry.redhat.io
Either migrate to a digest-pinned Red Hat registry image (if available) or explicitly document why this image uses upstream registry without pinning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/optional/cert-manager/manager/images-x86_64.yaml` at 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.
| 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.
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
eslutsky
force-pushed
the
cert-manager-trustmanager
branch
from
fafb715 to
fb94da3
Compare
2 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_FEATURESflag