Cert-manager create ca-bundle configmap for trust-manager#6605
Conversation
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
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}
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
| 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) |
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.
| --- | ||
| apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| metadata: | ||
| name: "bundles.trust.cert-manager.io" | ||
| annotations: | ||
| helm.sh/resource-policy: keep | ||
| labels: | ||
| app.kubernetes.io/name: cert-manager-trust-manager | ||
| app.kubernetes.io/instance: cert-manager-trust-manager | ||
| app.kubernetes.io/version: "v0.20.3" | ||
| app.kubernetes.io/managed-by: cert-manager-operator | ||
| app.kubernetes.io/part-of: cert-manager-operator | ||
| spec: | ||
| group: trust.cert-manager.io | ||
| names: | ||
| kind: Bundle | ||
| listKind: BundleList | ||
| plural: bundles | ||
| singular: bundle | ||
| scope: Cluster | ||
| versions: | ||
| - additionalPrinterColumns: |
There was a problem hiding this comment.
Bundle CRD is added, but operator RBAC appears insufficient for Bundle lifecycle.
Cross-file context (assets/optional/cert-manager/rbac/role.yaml, Lines 282-305) currently grants only get/list/watch on trust.cert-manager.io/bundles (plus finalizers/status subresources). Without create/update/patch/delete on bundles, the operator cannot fully manage Bundle objects.
Suggested RBAC adjustment
- apiGroups:
- trust.cert-manager.io
resources:
- bundles
verbs:
- get
- list
- watch
+ - create
+ - update
+ - patch
+ - delete🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@assets/optional/cert-manager/crd/bases/customresourcedefinition_bundles.trust.cert-manager.io.yml`
around lines 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.
| 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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant sections
wc -l assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlRepository: openshift/microshift
Length of output: 149
🏁 Script executed:
# Read lines around 1134-1141
sed -n '1130,1145p' assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlRepository: openshift/microshift
Length of output: 863
🏁 Script executed:
# Read lines around 1210-1220
sed -n '1205,1225p' assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlRepository: openshift/microshift
Length of output: 1051
🌐 Web query:
Kubernetes DNS-1123 subdomain pattern validation rules
💡 Result:
Kubernetes DNS-1123 subdomain pattern validation rules, used for most resource names (e.g., Deployments, Services, namespaces), are defined in the Kubernetes source code as follows: - Maximum length: 253 characters. - Regex pattern: ^[a-z0-9?(\.a-z0-9?)*]$ - Allowed characters: lowercase alphanumeric (a-z, 0-9), hyphen (-), dot (.). - Must start and end with an alphanumeric character (a-z0-9). - Each label (segment between dots) must start and end with alphanumeric, contain only lowercase alphanumerics or -, max 63 chars per label implicitly. - No uppercase letters, underscores, or other special characters. - Examples of valid: "example.com", "a-1.b2.c", "my-service-1.default.svc.cluster.local" (up to 253 chars). - Examples of invalid: "Example.com" (uppercase), "a_b.com" (underscore), "-start.com", "end-.com", "a." (ends with dot), strings >253 chars. This is a stricter subset of actual RFC 1123, enforced for Kubernetes object names that form DNS subdomains. Note: Distinct from DNS-1123 label (no dots, max 63 chars, used for Pod hostnames, single labels), and label keys (prefix is DNS subdomain + / + name segment).
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 2: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 3: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/content/dns_test.go
- 4: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/util/validation/validation_test.go
🌐 Web query:
Kubernetes secret naming conventions DNS-1123
💡 Result:
Kubernetes Secret names must follow DNS-1123 subdomain naming conventions. This means: - Maximum 253 characters in length - Lowercase alphanumeric characters (a-z, 0-9), hyphens (-), and periods (.) only - Must start and end with an alphanumeric character - Can consist of multiple DNS labels separated by periods, where each label is at most 63 characters (though Kubernetes validates the overall subdomain format) These rules apply because Secret metadata.name requires a valid DNS subdomain name as defined in RFC 1123, consistent with most Kubernetes resource types.
Citations:
- 1: https://kubernetes.io/docs/concepts/configuration/secret
- 2: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 3: https://kubernetes.io/docs/concepts/configuration/secret/
🏁 Script executed:
# Check if there are other validation rules or CEL rules for these fields
rg -A 10 "authorizedSecrets" assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yaml | head -40Repository: openshift/microshift
Length of output: 2115
🏁 Script executed:
# Look for any other validation mechanisms in the file
rg "x-kubernetes-validations" assets/optional/cert-manager/crd/bases/operator.openshift.io_trustmanagers.yamlRepository: openshift/microshift
Length of output: 190
Add DNS-1123 name validation for authorizedSecrets[] and trustNamespace.
Both fields currently lack pattern constraints. Invalid names like "InvalidName", "-start", or "end-" pass CRD admission and only fail when Kubernetes attempts to create/update objects. Add the following constraints:
Suggested schema changes
secretTargets:
properties:
authorizedSecrets:
items:
+ maxLength: 253
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
minLength: 1
type: string
@@
trustNamespace:
maxLength: 63
minLength: 1
+ pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$
type: stringAlso applies to: 1210-1220
🤖 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 - 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.
| x-kubernetes-validations: | ||
| - message: TrustManager is a singleton, .metadata.name must be 'cluster' | ||
| rule: self.metadata.name == 'cluster' |
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.
| # 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 | ||
|
|
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.
| #!/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 |
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.
| #!/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
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
1 participant
Summary by CodeRabbit
Release Notes
New Features
Chores