Cert-manager create ca-bundle configmap for trust-manager#6605
Draft
eslutsky wants to merge 8 commits intoopenshift:mainfrom
Draft
Cert-manager create ca-bundle configmap for trust-manager#6605eslutsky wants to merge 8 commits intoopenshift:mainfrom
eslutsky wants to merge 8 commits intoopenshift:mainfrom
Conversation
…v4.22 Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
Signed-off-by: Evgeny Slutsky <eslutsky@redhat.com>
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
WalkthroughThis pull request upgrades cert-manager from v1.18.4 to v1.19.4 and introduces trust-manager (v0.20.3) support by adding new CRDs, updating existing CRD schemas with Kubernetes list semantics, extending RBAC permissions, updating deployment configurations with new image digests, and adding CA bundle management automation for MicroShift integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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
Comment on lines
+1360
to
1362
| 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. |
There was a problem hiding this comment.
Anti-affinity scoring text is still internally inconsistent.
Line 1360 and Line 2571 now say weight is subtracted, but Line 1364 and Line 2575 still say weights are added per-node. Please align those descriptions to avoid operator confusion.
Suggested text 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)
...
- 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: 2571-2573
🤖 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 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.
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) |
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
+1
to
+23
| --- | ||
| 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.
Comment on lines
+1134
to
+1141
| 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.
Comment on lines
+1326
to
+1328
| 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.
Comment on lines
+68
to
+83
| patches: | ||
| - target: | ||
| kind: Deployment | ||
| name: controller-manager | ||
| patch: |- | ||
| - op: replace | ||
| path: /spec/template/spec/containers/0/env/16 | ||
| value: | ||
| name: UNSUPPORTED_ADDON_FEATURES | ||
| value: 'TrustManager=true' | ||
| configMapGenerator: | ||
| - name: trusted-ca-bundle | ||
| files: | ||
| - ca-bundle.crt=tls-ca-bundle.pem | ||
| generatorOptions: | ||
| disableNameSuffixHash: true |
There was a problem hiding this comment.
Avoid the positional env patch.
/env/16 is brittle; if upstream reorders the container env list, this will hit the wrong entry or fail. Prefer a merge patch keyed by name: UNSUPPORTED_ADDON_FEATURES.
🔧 Safer patch shape
patches:
- target:
kind: Deployment
name: controller-manager
- patch: |-
- - op: replace
- path: /spec/template/spec/containers/0/env/16
- value:
- name: UNSUPPORTED_ADDON_FEATURES
- value: 'TrustManager=true'
+ patch: |-
+ apiVersion: apps/v1
+ kind: Deployment
+ metadata:
+ name: controller-manager
+ spec:
+ template:
+ spec:
+ containers:
+ - name: cert-manager-operator
+ env:
+ - name: UNSUPPORTED_ADDON_FEATURES
+ value: TrustManager=true🤖 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 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.
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 | ||
|
|
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 |
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.
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
BundleandTrustManagercustom resources.Updates
Infrastructure