Conversation
Refresh Helm bindata and the Bundle CRD, apply the cluster-view ClusterRole, and pin related images. Needed so --filter-non-ca-certs is available for the follow-up API PR.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@arun717: This pull request references CM-1367 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set. 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. |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (26)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe change upgrades trust-manager to v0.25.0. It updates Kubernetes assets, strengthens Bundle CRD validation, changes certificate and webhook CA injection behavior, removes the ChangesTrust-manager v0.25.0 upgrade
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant RBACReconciler
participant AssetStore
participant KubernetesAPI
participant ClusterReader
RBACReconciler->>AssetStore: Load cluster-view ClusterRole asset
RBACReconciler->>KubernetesAPI: Get and server-side apply ClusterRole
KubernetesAPI->>ClusterReader: Aggregate Bundle read access
KubernetesAPI-->>RBACReconciler: Return reconciliation result
Merge Risk: ⚪ Minimal · up to The trust-manager upgrade assets, injection configuration, CRD validation, and read-only cluster-view role show no identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 9 files. (20 skipped: 20 unsupported.) Full details: Test Structure And QualityExplanation The changed Ginkgo code adds and modifies assertions without meaningful failure messages. The webhook annotation assertions at the changed spec lines call Resolution Add diagnostic messages to every new or modified assertion. For example, state that the webhook must use ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arun717 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Refresh Helm bindata and pin related images. Align webhook CA injection with Helm 0.25 (inject-ca-from-secret on the TLS Secret).
arun717
changed the title
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
Regarding test/go.mod stays github.com/cert-manager/trust-manager v0.20.3. It should be fine updating the k8s packages to v0.36.4 and also the openshift/api package to release-5.0 commit.
| }) | ||
| } | ||
|
|
||
| const clusterReaderAggregateLabel = "rbac.authorization.k8s.io/aggregate-to-cluster-reader" |
There was a problem hiding this comment.
Static manifest already has this label added, what would be need for adding it here again?
There was a problem hiding this comment.
UpdateResourceLabels replaces the object's labels (SetLabels) rather than merging them, so the aggregate label from the static manifest is dropped when we apply getResourceLabels().
We set rbac.authorization.k8s.io/aggregate-to-cluster-reader=true again afterward so cluster-reader still aggregates Bundle get/list/watch. Without this, the applied ClusterRole would lose that label.
| app.kubernetes.io/instance: cert-manager-trust-manager | ||
| app.kubernetes.io/version: "v0.25.0" | ||
| app.kubernetes.io/managed-by: cert-manager-operator | ||
| rbac.authorization.k8s.io/aggregate-to-cluster-reader: "true" |
There was a problem hiding this comment.
for this to work, we also need to create deploy/charts/trust-manager/templates/clusterrole-aggregate.yaml, which otherwise has value I think.
There was a problem hiding this comment.
Also the whole aggregation is an opt-in feature using the aggregateClusterRoles helm value, shouldn't we be also doing the same?
There was a problem hiding this comment.
Looks like we do not need another copy of clusterrole-aggregate.yaml.
That Helm file is this ClusterRole:
- name: trust-manager-cluster-view
- rules: get/list/watch on trust.cert-manager.io/bundles
- label: rbac.authorization.k8s.io/aggregate-to-cluster-reader: "true"
The operator already has that as clusterrole_trust-manager-cluster-view.yml and applies it in createOrApplyClusterViewClusterRole.
There is no extra aggregator object in the chart, just this labeled ClusterRole.
Aggregation is performed by OpenShift’s existing cluster-reader, which already selects that label. Creating a second YAML with the same content would be a duplicate.
Also, the aggregation part is configurable but it is not opt-in in the usual sense.
Upstream Helm gates the ClusterRole with global.rbac.aggregateClusterRoles. That value defaults to true, so aggregation is on unless you turn it off (opt-out).
The template only skips clusterrole-aggregate.yaml when you set it to false (or disable RBAC entirely with global.rbac.create: false).
Please do let me know what you think. As of now not making any code change related to this in the current PR.
…t-ca-from stripping. Detect PrivateKey.RotationPolicy drift without comparing cert-manager-defaulted key fields, and stop deleting inject-ca-from from user webhook annotations.
…er v0.25.0 test types. Bump operator and e2e modules to k8s v0.36.4 and openshift/api release-5.0 so test/go.mod can use trust-manager v0.25.0 Bundle APIs.
Fix verify by committing make generate output and unifying duplicate dependency versions across root, tools, and test modules.
The v0.25.0 operand ClusterRole creates events.k8s.io events, and Kubernetes blocks that grant unless the operator already holds it.
Webhook override e2e failed when static-resources Degraded stayed False while deployment Degraded was True, because Any still failed on order.
|
@arun717: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
RELATED_IMAGE/ CSV, Helm bindata, Bundle CRD).trust-manager-cluster-view(read-onlybundles+aggregate-to-cluster-reader). This stays a separate ClusterRole so cluster-reader does not inherit operand write rules.cert-manager.io/inject-ca-from-secretontrust-manager-tls, plus CertificatesecretTemplatecert-manager.io/allow-direct-injection: "true".release-5.0(31af9f93e31e), with matchingopenshift/client-go,library-go, and controller-runtime v0.24.1.test/go.modto trust-manager v0.25.0 and update e2e Bundle builders for the v0.25 API (InLinestring, valueTargetMetadata). Upstream trust-manager still requires k8s 0.37; the test module replace-pins k8s 0.36.4 for OpenShift 5.0.This is the operand half of CM-1367. The TrustManager
filterNonCACertsAPI lives in #494. Merge this PR first:--filter-non-ca-certsexists only from v0.21.0, so 494’s Enabled path will CrashLoop a v0.20.3 operand.Jira: https://issues.redhat.com/browse/CM-1367
Notes for reviewers
make bundle. Content is the upstream Bundle schema (CEL, length limits, descriptions), not a YAML reformat. Helm output stays inconfig/crd/bases/; do not copy it ontobundle/manifests/.test/go.modusesgithub.com/cert-manager/trust-manager v0.25.0. That module requires k8s.io/* v0.37.0; replace directives keep the selected Kubernetes/client stack at v0.36.4 to match the operator and OpenShift 5.0.images/ci/trustmanager.DockerfilesetsRELEASE_BRANCH=v0.25.0and clonesopenshift/cert-manager-trust-manager. That fork’s latest tag is still v0.20.3; the Dockerfile clone will fail until midstream has the tag. CSV/RELATED_IMAGEusesquay.io/jetstack/trust-manager:v0.25.0.UpdateResourceLabelsreplaces labels, so the aggregate label is set again after that.Test plan
quay.io/jetstack/trust-manager:v0.25.0--filter-non-ca-certs)cert-manager.io/inject-ca-from-secret=cert-manager/trust-manager-tlsoc get clusterrole trust-manager-cluster-viewand cluster-reader aggregationgo.modpinsk8s.io/* v0.36.4andgithub.com/openshift/apirelease-5.0go test -tags e2e -count=0 ./test/...)Summary by CodeRabbit
New Features
Validation