Cert-manager create ca-bundle configmap for trust-manager#6605
Draft
eslutsky wants to merge 1 commit into
Draft
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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
Contributor
WalkthroughMicroShift cert-manager deployment now integrates the system CA bundle by generating a ConfigMap from a ChangesCA Bundle Integration for Cert-Manager
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/challenges.acme.cert-manager.io-crd.yaml`:
- Around line 1360-1362: The anti-affinity scoring description is inconsistent:
one occurrence says "weight is subtracted" while another says "weights are added
per-node"; update the two matched description blocks (the paragraph that begins
"compute a sum by iterating through the elements of this field and subtracting
'weight'..." and the corresponding paragraph that currently states "weights are
added per-node") so both use the same operator wording—preferably "subtract
'weight' from the sum if the node has pods which match the corresponding
podAffinityTerm; the node(s) with the highest sum are the most preferred." Apply
this identical, unambiguous sentence to both places (the block around the
current "subtract" phrasing and the block that currently says "added") so they
are consistent.
In
`@assets/optional/cert-manager/crd/bases/clusterissuers.cert-manager.io-crd.yaml`:
- Around line 1473-1477: The two duplicated descriptions around
WeightedPodAffinityTerm are inconsistent (one says "subtracting the weight" and
the other says "weights are added"); make them consistent by changing the later
phrasing to match the earlier one: update the items.description that currently
reads "The weights of all of the matched WeightedPodAffinityTerm fields are
added per-node to find the most preferred node(s)" so it instead describes that
the weights are subtracted per-node (e.g., "The weights of all matched
WeightedPodAffinityTerm fields are subtracted per-node to find the most
preferred node(s)"), and apply the same change to the duplicated block so both
references to WeightedPodAffinityTerm/podAffinityTerm use identical wording.
In
`@assets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml`:
- Around line 1-23: The Bundle CRD (metadata name
"bundles.trust.cert-manager.io") is defined but the operator Role in
assets/optional/cert-manager/rbac/role.yaml only grants get/list/watch on the
custom resource, preventing full lifecycle management; update the Role binding
for the resource "trust.cert-manager.io/bundles" to include verbs create,
update, patch, and delete (in addition to get/list/watch and finalizers/status)
so the operator can create, modify, and remove Bundle objects and manage their
finalizers/status.
In
`@assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yaml`:
- Around line 1326-1328: The CRD's singleton validation
(x-kubernetes-validations rule requiring self.metadata.name == 'cluster')
enforces a TrustManager instance but no TrustManager/cluster resource is being
deployed; add a bootstrap TrustManager manifest named 'cluster' or implement
operator auto-create logic so the feature flag
(UNSUPPORTED_ADDON_FEATURES=TrustManager=true) actually activates—create a
minimal TrustManager resource (metadata.name: cluster) and include it in the
kustomization or add reconciler code in the operator to ensure a TrustManager
named "cluster" is created when the operator starts.
- Around line 1134-1141: The CRD lacks DNS-1123 validation for the
authorizedSecrets array items and the trustNamespace field; update the schema so
each authorizedSecrets item and trustNamespace include a DNS-1123 pattern (e.g.
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$) and length bounds (minLength: 1 and maxLength:
253) to prevent invalid names from passing admission; apply the same constraints
to the other occurrence of these fields (the trustNamespace block referenced in
the review) so both places enforce DNS-1123 validation.
In `@assets/optional/cert-manager/manager/kustomization.yaml`:
- Around line 68-83: The patch currently replaces
/spec/template/spec/containers/0/env/16 which is brittle; update the
kustomization patch for the Deployment named controller-manager to use a
strategic/merge (or JSON 6902 keyed by name) patch that matches the env entry by
name (UNSUPPORTED_ADDON_FEATURES) instead of using a numeric index, i.e., target
the container env list and merge/replace the entry where name ==
UNSUPPORTED_ADDON_FEATURES so the value 'TrustManager=true' is applied reliably
even if upstream reorders or adds env vars.
In `@packaging/rpm/microshift.spec`:
- Around line 576-580: Add a cert-manager-scoped RPM post scriptlet that reloads
systemd so the installed drop-in becomes active: add a %post section for the
cert-manager subpackage which runs a systemctl daemon-reload (or calls the rpm
macro that performs that, e.g. invoke %systemd_post for microshift.service from
the cert-manager subpackage) so the installed drop-in at
/etc/systemd/system/microshift.service.d/microshift-cert-manager-ca-bundle.conf
is picked up when cert-manager is installed without the main package.
In `@packaging/systemd/microshift-cert-manager-update-ca-bundle.sh`:
- Around line 1-15: The script (microshift-cert-manager-update-ca-bundle.sh)
currently silently skips when SRC
("/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem") or the target directory
for DST
("/usr/lib/microshift/manifests.d/060-microshift-cert-manager/manager/tls-ca-bundle.pem")
is missing; change it to fail fast by checking both the source file and the
destination directory and, if either is absent, print a clear error to stderr
and exit non-zero (e.g., exit 1) so ExecStartPre fails early rather than letting
kustomize fail later — update the existing conditional around SRC/DST to emit
the error and exit when the file or directory is not present.
🪄 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: Enterprise
Run ID: ec01764d-da3f-4585-b4c6-5e5460179515
📒 Files selected for processing (28)
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/featuregate_clusterrole.yamlassets/optional/cert-manager/rbac/featuregate_clusterrole_binding.yamlassets/optional/cert-manager/rbac/kustomization.yamlassets/optional/cert-manager/rbac/role.yamlassets/optional/cert-manager/release-cert-manager-aarch64.jsonassets/optional/cert-manager/release-cert-manager-x86_64.jsonpackaging/rpm/microshift.specpackaging/systemd/microshift-cert-manager-ca-bundle.confpackaging/systemd/microshift-cert-manager-update-ca-bundle.shscripts/auto-rebase/assets_cert_manager.yamlscripts/auto-rebase/last_rebase_cert_manager.shscripts/auto-rebase/rebase_cert_manager.shscripts/auto-rebase/rebase_job_entrypoint.sh
Comment on lines
+1473
to
1477
| 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 | ||
| node(s) with the highest sum are the most preferred. | ||
| items: | ||
| description: The weights of all of the matched WeightedPodAffinityTerm fields are added per-node to find the most preferred node(s) |
Contributor
There was a problem hiding this comment.
Fix contradictory anti-affinity scoring wording.
Line 1473/2684 says scoring subtracts weight, but Line 1477/2688 still says weights are added. Please make the wording internally consistent in both duplicated sections.
Suggested doc-only fix
- description: The weights of all of the matched WeightedPodAffinityTerm fields are added per-node to find the most preferred node(s)
+ description: The weights of all of the matched WeightedPodAffinityTerm fields are subtracted per-node to find the most preferred node(s)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 two duplicated descriptions around
WeightedPodAffinityTerm are inconsistent (one says "subtracting the weight" and
the other says "weights are added"); make them consistent by changing the later
phrasing to match the earlier one: update the items.description that currently
reads "The weights of all of the matched WeightedPodAffinityTerm fields are
added per-node to find the most preferred node(s)" so it instead describes that
the weights are subtracted per-node (e.g., "The weights of all matched
WeightedPodAffinityTerm fields are subtracted per-node to find the most
preferred node(s)"), and apply the same change to the duplicated block so both
references to WeightedPodAffinityTerm/podAffinityTerm use identical wording.
Comment on lines
+1326
to
+1328
| x-kubernetes-validations: | ||
| - message: TrustManager is a singleton, .metadata.name must be 'cluster' | ||
| rule: self.metadata.name == 'cluster' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether a non-CRD bootstrap TrustManager resource exists.
# Expected: at least one match outside CRD definition files.
rg -nPU --type=yaml --glob '!**/crd/**' '(?s)^\s*kind:\s*TrustManager\s*$.*?^\s*metadata:\s*$.*?^\s*name:\s*cluster\s*$'Repository: openshift/microshift
Length of output: 46
🏁 Script executed:
# Search for any TrustManager references outside CRD to understand operator behavior
rg -i 'trustmanager' --type=yaml --glob '!**/crd/**' -A 2 -B 2Repository: openshift/microshift
Length of output: 2367
🏁 Script executed:
# Check kustomization files for any create/patch operations related to TrustManager
fd -e yaml -e kustomization -path '*cert-manager*' | head -20Repository: openshift/microshift
Length of output: 233
🏁 Script executed:
# Search operator code for auto-create or bootstrap logic
find . -type f \( -name '*.go' -o -name '*.ts' -o -name '*.js' \) | xargs grep -l -i 'trustmanager\|cluster.*bootstrap' 2>/dev/null | head -10Repository: openshift/microshift
Length of output: 923
🏁 Script executed:
# Search for any reference to "cluster" name in context of TrustManager or trustmanager
rg 'cluster' --type=yaml --glob '*cert-manager*' -B 2 -A 2 | grep -A 4 -B 4 -i 'trust'Repository: openshift/microshift
Length of output: 46
🏁 Script executed:
# Check if there's any documentation or comments about TrustManager initialization
rg -i 'trustmanager.*cluster\|cluster.*trustmanager\|bootstrap.*trust\|auto.*create' --type=yaml --glob '*cert-manager*'Repository: openshift/microshift
Length of output: 46
🏁 Script executed:
# Look at the entire kustomization structure for cert-manager to understand resource generation
find assets/optional/cert-manager -name 'kustomization.yaml' -exec echo "=== {} ===" \; -exec cat {} \;Repository: openshift/microshift
Length of output: 7233
Singleton validation requires bootstrap instance, but TrustManager/cluster manifest is missing.
The CRD enforces metadata.name == 'cluster' (lines 1326-1328), yet the kustomization deploys only CRD, RBAC, and the operator Deployment with the feature flag enabled (UNSUPPORTED_ADDON_FEATURES=TrustManager=true). Without an actual TrustManager instance, the feature remains inert. Add a bootstrap manifest or operator auto-create logic.
🤖 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 1326 - 1328, The CRD's singleton validation
(x-kubernetes-validations rule requiring self.metadata.name == 'cluster')
enforces a TrustManager instance but no TrustManager/cluster resource is being
deployed; add a bootstrap TrustManager manifest named 'cluster' or implement
operator auto-create logic so the feature flag
(UNSUPPORTED_ADDON_FEATURES=TrustManager=true) actually activates—create a
minimal TrustManager resource (metadata.name: cluster) and include it in the
kustomization or add reconciler code in the operator to ensure a TrustManager
named "cluster" is created when the operator starts.
Comment on lines
+576
to
+580
| # cert-manager CA bundle update script and systemd drop-in | ||
| install -p -m755 packaging/systemd/microshift-cert-manager-update-ca-bundle.sh %{buildroot}%{_bindir}/microshift-cert-manager-update-ca-bundle | ||
| mkdir -p -m755 %{buildroot}%{_sysconfdir}/systemd/system/microshift.service.d | ||
| install -p -m644 packaging/systemd/microshift-cert-manager-ca-bundle.conf %{buildroot}%{_sysconfdir}/systemd/system/microshift.service.d/microshift-cert-manager-ca-bundle.conf | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify drop-in install exists and check for a cert-manager-scoped post scriptlet.
rg -n 'microshift-cert-manager-ca-bundle\.conf|%post cert-manager|%systemd_post microshift\.service' packaging/rpm/microshift.specRepository: openshift/microshift
Length of output: 375
🏁 Script executed:
# Get context around the lines mentioned in the review
sed -n '570,630p' packaging/rpm/microshift.spec | cat -nRepository: openshift/microshift
Length of output: 4016
🏁 Script executed:
# Get context around the "Also applies to" lines
sed -n '785,795p' packaging/rpm/microshift.spec | cat -nRepository: openshift/microshift
Length of output: 608
Add a cert-manager post scriptlet to reload systemd drop-ins.
The cert-manager subpackage installs /etc/systemd/system/microshift.service.d/microshift-cert-manager-ca-bundle.conf but lacks a cert-manager-scoped %post hook to run daemon reload. The existing %systemd_post microshift.service is part of the main package post hook and won't execute when cert-manager is installed independently, leaving the drop-in unconfigured until manual reload/reboot.
Add:
+%post cert-manager
+%systemd_post microshift.service🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packaging/rpm/microshift.spec` around lines 576 - 580, Add a
cert-manager-scoped RPM post scriptlet that reloads systemd so the installed
drop-in becomes active: add a %post section for the cert-manager subpackage
which runs a systemctl daemon-reload (or calls the rpm macro that performs that,
e.g. invoke %systemd_post for microshift.service from the cert-manager
subpackage) so the installed drop-in at
/etc/systemd/system/microshift.service.d/microshift-cert-manager-ca-bundle.conf
is picked up when cert-manager is installed without the main package.
Comment on lines
+1
to
+15
| #!/bin/bash | ||
| # Copy the system CA bundle into the cert-manager manifests directory | ||
| # so kustomize can use it to create the trusted-ca-bundle ConfigMap. | ||
| # | ||
| # This script runs as ExecStartPre before MicroShift starts. | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| SRC="/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" | ||
| DST="/usr/lib/microshift/manifests.d/060-microshift-cert-manager/manager/tls-ca-bundle.pem" | ||
|
|
||
| # Only copy if the cert-manager manifests directory exists (package installed) | ||
| if [ -d "$(dirname "${DST}")" ] && [ -f "${SRC}" ]; then | ||
| cp -f "${SRC}" "${DST}" | ||
| fi |
Contributor
There was a problem hiding this comment.
Fail fast if the CA bundle is missing.
Silently skipping the copy leaves the manifest still pointing at tls-ca-bundle.pem, so startup will fail later during kustomize render. Consider exiting non-zero here or making the ConfigMap conditional.
🔧 Suggested guard
-if [ -d "$(dirname "${DST}")" ] && [ -f "${SRC}" ]; then
- cp -f "${SRC}" "${DST}"
+if [ -d "$(dirname "${DST}")" ]; then
+ if [ -f "${SRC}" ]; then
+ cp -f "${SRC}" "${DST}"
+ else
+ echo "Missing CA bundle at ${SRC}" >&2
+ exit 1
+ fi
fi📝 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.
Suggested change
| #!/bin/bash | |
| # Copy the system CA bundle into the cert-manager manifests directory | |
| # so kustomize can use it to create the trusted-ca-bundle ConfigMap. | |
| # | |
| # This script runs as ExecStartPre before MicroShift starts. | |
| set -euo pipefail | |
| SRC="/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" | |
| DST="/usr/lib/microshift/manifests.d/060-microshift-cert-manager/manager/tls-ca-bundle.pem" | |
| # Only copy if the cert-manager manifests directory exists (package installed) | |
| if [ -d "$(dirname "${DST}")" ] && [ -f "${SRC}" ]; then | |
| cp -f "${SRC}" "${DST}" | |
| fi | |
| #!/bin/bash | |
| # Copy the system CA bundle into the cert-manager manifests directory | |
| # so kustomize can use it to create the trusted-ca-bundle ConfigMap. | |
| # | |
| # This script runs as ExecStartPre before MicroShift starts. | |
| set -euo pipefail | |
| SRC="/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem" | |
| DST="/usr/lib/microshift/manifests.d/060-microshift-cert-manager/manager/tls-ca-bundle.pem" | |
| # Only copy if the cert-manager manifests directory exists (package installed) | |
| if [ -d "$(dirname "${DST}")" ]; then | |
| if [ -f "${SRC}" ]; then | |
| cp -f "${SRC}" "${DST}" | |
| else | |
| echo "Missing CA bundle at ${SRC}" >&2 | |
| exit 1 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packaging/systemd/microshift-cert-manager-update-ca-bundle.sh` around lines 1
- 15, The script (microshift-cert-manager-update-ca-bundle.sh) currently
silently skips when SRC ("/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem") or
the target directory for DST
("/usr/lib/microshift/manifests.d/060-microshift-cert-manager/manager/tls-ca-bundle.pem")
is missing; change it to fail fast by checking both the source file and the
destination directory and, if either is absent, print a clear error to stderr
and exit non-zero (e.g., exit 1) so ExecStartPre fails early rather than letting
kustomize fail later — update the existing conditional around SRC/DST to emit
the error and exit when the file or directory is not present.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eslutsky
force-pushed
the
cert-manager-systemd-ca-bundle
branch
from
732d912 to
7c30936
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
assets/optional/cert-manager/manager/kustomization.yaml (1)
72-73: ConfigMap updates won't trigger pod restarts.With
disableNameSuffixHash: true, the ConfigMap name stays constant. Whentls-ca-bundle.pemchanges, the ConfigMap updates in-place but pods won't automatically restart to pick up the new CA bundle. Ensure there's a mechanism (e.g., sidecar, rolling restart, or volume auto-refresh) to propagate CA bundle updates to running pods.🤖 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 `@assets/optional/cert-manager/manager/kustomization.yaml` around lines 72 - 73, The kustomization uses generatorOptions.disableNameSuffixHash: true which prevents ConfigMap name changes so updates to tls-ca-bundle.pem won't trigger pod restarts; revert disableNameSuffixHash to false (or remove that option) so ConfigMap names include the hash, or implement an alternative propagation mechanism (e.g., add a sidecar that watches tls-ca-bundle.pem, add an annotation-driven rolling restart controller, or mount the CA via a projected volume with auto-refresh) and ensure the ConfigMap/volume update triggers pod reloads; update the kustomization generatorOptions and/or add the chosen propagation mechanism accordingly.
🤖 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.
Nitpick comments:
In `@assets/optional/cert-manager/manager/kustomization.yaml`:
- Around line 72-73: The kustomization uses
generatorOptions.disableNameSuffixHash: true which prevents ConfigMap name
changes so updates to tls-ca-bundle.pem won't trigger pod restarts; revert
disableNameSuffixHash to false (or remove that option) so ConfigMap names
include the hash, or implement an alternative propagation mechanism (e.g., add a
sidecar that watches tls-ca-bundle.pem, add an annotation-driven rolling restart
controller, or mount the CA via a projected volume with auto-refresh) and ensure
the ConfigMap/volume update triggers pod reloads; update the kustomization
generatorOptions and/or add the chosen propagation mechanism accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 942dedf3-bb69-4de6-8629-35cfd8400b02
📒 Files selected for processing (1)
assets/optional/cert-manager/manager/kustomization.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores