OCPBUGS-78211,OCPBUGS-78455: Synchronize From Upstream Repositories#687
OCPBUGS-78211,OCPBUGS-78455: Synchronize From Upstream Repositories#687camilamacedo86 wants to merge 89 commits intoopenshift:mainfrom
Conversation
Change GetDeploymentConfig() to return (*DeploymentConfig, error) instead of map[string]any, eliminating the intermediate convertToDeploymentConfig() function in provider.go. The caller was immediately converting the map to a DeploymentConfig anyway, so this simplifies the API and removes unnecessary indirection. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…after upgrade (#2578)
Bumps [marocchino/sticky-pull-request-comment](https://github.com/marocchino/sticky-pull-request-comment) from 2 to 3. - [Release notes](https://github.com/marocchino/sticky-pull-request-comment/releases) - [Commits](marocchino/sticky-pull-request-comment@v2...v3) --- updated-dependencies: - dependency-name: marocchino/sticky-pull-request-comment dependency-version: '3' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Externalize CER phase objects into Secret refs Add support for storing ClusterExtensionRevision phase objects in content-addressable immutable Secrets instead of inline in the CER spec. This removes the etcd object size limit as a constraint on bundle size. API changes: - Add ObjectSourceRef type with name, namespace, and key fields - Make ClusterExtensionRevisionObject.Object optional (omitzero) - Add optional Ref field with XValidation ensuring exactly one is set - Add RefResolutionFailed condition reason - Add RevisionNameKey label for ref Secret association Applier (boxcutter.go): - Add SecretPacker to bin-pack serialized objects into Secrets with gzip compression for objects exceeding 800KiB - Add createExternalizedRevision with crash-safe three-step sequence: create Secrets, create CER with refs, patch ownerReferences - Externalize desiredRevision before SSA comparison so the patch compares refs-vs-refs instead of inline-vs-refs - Add ensureSecretOwnerReferences for crash recovery - Pass SystemNamespace to Boxcutter from main.go CER controller: - Add resolveObjectRef to fetch and decompress objects from Secrets - Handle ref resolution in buildBoxcutterPhases - Add RBAC for Secret get/list/watch E2e tests: - Add scenario verifying refs, immutability, labels, and ownerRefs - Add step definitions for ref Secret validation - Fix listExtensionRevisionResources and ClusterExtensionRevisionObjectsNotFoundOrNotOwned to resolve refs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR #2595 review feedback - Fix duplicate key size inflation in SecretPacker by only incrementing size for new content hash keys - Add io.LimitReader (10 MiB cap) for gzip decompression to prevent gzip bombs in controller and e2e helpers - Add doc comment clarifying ObjectSourceRef.Namespace defaults to OLM system namespace during ref resolution - Fix docs: orphan cleanup uses ownerReference GC, ref resolution failures are retried (not terminal) - Remove unused ClusterExtensionRevisionReasonRefResolutionFailed constant - Add default error branch in e2e listExtensionRevisionResources for objects missing both ref and inline content Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Change gzipThreshold from 800 KiB to 900 KiB Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Adds a new large-crd-operator test bundle containing a ~1MB CRD to verify that the Boxcutter runtime correctly handles large bundle installations. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
… (#2589) Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Update generate-manifests to handle new directory The `default` directory was renamed `base`. Signed-off-by: Todd Short <todd.short@me.com> The `base` directory was moved to `base\operator-controller`. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Drop commitchecker Signed-off-by: Alexander Greene <greene.al1991@gmail.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART Reconciling with https://github.com/openshift/ocp-build-data/tree/4022cd290f00a44d667dda03f2d78d84a488c7ed/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: update owners * Remove alumni from owners * Add m1kola to approvers Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Add pointer to tooling README UPSTREAM: <carry>: Disable Validating Admission Policy APIs downstream Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.16 Reconciling with https://github.com/openshift/ocp-build-data/tree/6250d54c4686a708ca5985afb73080e8ca9a1f7f/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Enable Validating Admission Policy APIs downstream * This reverts commit 3f079c4. * Includes Validating Admission Policy manifests Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: manifests: set required-scc for openshift workloads UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.17 Reconciling with https://github.com/openshift/ocp-build-data/tree/4c1326094222f9209876f06833179a1b9178faf7/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: add everettraven to approvers+reviewers Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: add openshift kustomize overlay to enable TLS communication with catalogd. Configure the CA certs using the configmap injection method via service-ca-operator Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: Add tmshort to approvers Also `s/runtime/framework/g` in the DOWNSTREAM_OWNERS Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.18 Reconciling with https://github.com/openshift/ocp-build-data/tree/dd68246f3237db5db458127566fc7b05b55e1660/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Properly copy and call kustomize Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: manifests: add hostPath mount for /etc/containers Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add test-e2e target for downstream Makefile to be run by openshift/release. Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Add downstream verify makefile target Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: openshift: template log verbosity to be managed by cluster-olm-operator Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add global-pull-secret flag Pass global-pull-secret to the manager container. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Update openshift CAs to operator-controller The /run/secrets/kubernetes.io/serviceaccount/ directory is projected into the pod and contains the following CA certificates: * configmap/kube-root-ca.crt as ca.crt * configmap/openshift-service-ca.crt as service-ca.crt Update the --ca-certs-dir argument to reference the directory. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add HowTo for origin tests Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add e2e registry Dockerfile Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: add nodeSelector and tolerations to operator-controller deployment via kustomize patch Signed-off-by: everettraven <everettraven@gmail.com> UPSTREAM: <carry>: namespace: use privileged PSA for audit and warn levels Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Enable downstream e2e Signed-off-by: dtfranz <dfranz@redhat.com> UPSTREAM: <carry>: Remove m1kola from owners Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.19 Reconciling with https://github.com/openshift/ocp-build-data/tree/a39508c86497b4e5e463d7b2c78e51e577be9e7d/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: generate and mount service-ca server cert Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add support for proxy trustedCAs Just map the list of trusted ca certs into the deployment Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Fix error to build the image Copy correct (new) executable name for operator-controller Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Fix make verify for mac os envs Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Move operator-controller openshift files to its own dir UPSTREAM: <carry>: Upgrade OCP images from 4.18 to 4.19 UPSTREAM: <carry>: Add Openshift's catalogd manifests - Move to openshift/catalogd the specific manifest under: https://github.com/openshift/operator-framework-catalogd/tree/main/openshift - Add call to generate catalogd manifest to 'make manifest'. Make verify test is now done for catalogd and operator-controller Openshift's manifests UPSTREAM: <carry>: resolve issue with pre-mature mounting of trusted CA configmap Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Add /etc/docker to the operator-controller and catalogd deployments This allows for use of the any image.config.openshift.io trusted CAs Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: fixup catalogd.Dockerfile paths Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Resolve issue with pre-mature mounting of service CA configmap Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Revert "UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations" This reverts commit 548caa4. UPSTREAM: <carry>: use projected volume for CAs to avoid subPath limitations Signed-off-by: Joe Lanford <joe.lanford@gmail.com> UPSTREAM: <carry>: Remove vet from openshift verify The `vet` target was removed upstream. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Skip another upstream test Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Cleanup openshift/Makefile by removing no longer required comments regards catalogd e2e tests UPSTREAM: <carry>: Enable OCP metrics collection by default Enables OCP to collect Prometheus metrics for both catalogd and operator-controller by default. This is accomplished via ServiceMonitor CRs which are now created for both projects. UPSTREAM: <carry>: Fix catalogd.Dockerfile to use new paths The root catalogd directory has been removed Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update DOWNSTREAM_OWNERS_ALIASES Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add openshift node selector annotation Signed-off-by: Catherine Chan-Tse <cchantse@redhat.com> (cherry picked from commit 9b4a113) UPSTREAM: <carry>: Add caalogd-cas-dir option to op-con Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: set the SElinux type Signed-off-by: Jian Zhang <jiazha@redhat.com> UPSTREAM: <carry>: Add initial stack to run tests to validate the catalogs UPSTREAM: <carry>: Add vendor files for the catalog-sync tests UPSTREAM: <carry>: Bump catalog versions to 4.19 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: revert "Bump catalog versions to 4.19" This reverts commit a98980b. UPSTREAM: <carry>: Update HOWTO-origin-tests techpreview is no longer a required option. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [DefaultCatalogTests]: Allow to pass auth path for docker credentials" UPSTREAM: <carry>: fix: set NoLchown=true to allow image unpack on OCPci UPSTREAM: <carry>: [DefaultCatalogTests]: Moving parse of ENVVAR to the caller (follow-up 345) UPSTREAM: <carry>: [Default Catalog]: Create tmp dir to extract layers with right permissions to avoid issues scenarios UPSTREAM: <carry>: [Default Catalog](cleanp) Remove hack directory which is not used UPSTREAM: <carry>: Change code implementation to extract layers in OCP env UPSTREAM: <carry>: Add vendor files for change in the extract code implementation UPSTREAM: <carry>: [Default Catalog Tests]: Final cleanups and enhancements of initial implementation UPSTREAM: <carry>: SELinux type for operator-controller Signed-off-by: Jian Zhang <jiazha@redhat.com> UPSTREAM: <carry>: Bump catalog versions to 4.19 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [Default Catalog Consistency Test] (feat) add check for executable files in filesystem Checks if given paths exist and point to executable files or valid symlinks. UPSTREAM: <carry>: [Default Catalog Consistency Test]: fix junit output format to allow generate xml UPSTREAM: <carry>: [Default Catalog Consistency Test] (feat) add check to validate multi-arch support UPSTREAM: <carry>: [Default Catalog Consistency Test]: Enable CatalogChecks UPSTREAM: <carry>: [Default Catalog Consistency Test]: Rename Tests suite and small cleanups UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.20 Reconciling with https://github.com/openshift/ocp-build-data/tree/dfb5c7d531490cfdc61a3b88bc533702b9624997/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Updating ose-olm-catalogd-container image to be consistent with ART for 4.20 Reconciling with https://github.com/openshift/ocp-build-data/tree/dfb5c7d531490cfdc61a3b88bc533702b9624997/images/ose-olm-catalogd.yml UPSTREAM: <carry>: Update e2e registry to use 1.24/4.20 Update the e2e registry Dockerfile to use golang 1.24/OCP 4.20 Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [Catalog Default Tests]: Upgrade go version to 1.24.3, dependencies and fix new lint issue UPSTREAM: <carry>: Add structure to allow move the orgin tests using OTE This commit introduces a binary and supporting structure to enable the execution of OpenShift origin (olmv1) tests using the Open Test Environment (OTE). It lays the groundwork for moving origin test in openshift/origin to be executed from this repository using OTE. UPSTREAM: <carry>: Add support for experimental manifests Update the openshift kustomize configuration for both operator-controller and catalogd. Update the manifest generation scripts to put the core generation code into a function (ignore-whitespace will help with the review), so that it can be called twice; once for standard, and once for experimental. Move around some of the kustomization directives to * Create a patch kustomization (Component) file and move the patch directives from olmv1-ns there. This allows it to be referenced from a different directory. * Add a kustomization file for tusted-ca. This allows it to be referenced from a different directory. * Move the setting of the namePrefix for operator-controller; this makes the generation compatible with upstream feature components. * Define experimental kustomization files that reference existing components. * Reference the correct CRDs (standard or experimental). * Add references to upstream feature components into the experimental manifests. This *will* add `--feature-gates` options from the upstream feature components to the experimental manifests. The cluster-olm-operator will strip those arguments from the deployments before adding the enabled feature gates. Update the Dockerfiles to include the experimental manifests and a copy script (`cp-manifests`) into the image containers. The complexity of having multiple sets of manifests mean that the simple initContainer copy mechanism found in cluster-olm-operator is no longer sufficient. This attempts to keep backwards compatibility with older versions of cluster-olm-operator, specifically by keeping the original (standard) manifests in the original location, and adding the experimental manifests in a new directory. The new `cp-manifests` script is used by newer versions of cluster-olm-operator. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] - chore: follow up openshift#383 – remove unreachable target call UPSTREAM: <carry>: Remove build of test image registry Upstream now uses a different image Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add test-experimental-e2e target to openshift Makefile This adds a test-experimental-e2e target to allow the CI to run the experimental e2e test. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE]: Add binary in the operator controller image to allow proper integration with OCP tests UPSTREAM: <carry>: Fix experimental manifest copying The standard manifest was being copied rather than the experimental manifest. This meant that the expected feature-flags are not present. This is failing now that we are doing a check for those feature-flags. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update manifest generation for upstream rbac/webhooks Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] - Add tracking mechanism UPSTREAM: <carry>: Update OTE dep to get fix UPSTREAM: <carry>: [OTE] Add Readme UPSTREAM: <carry>: set GIT_COMMIT env from SOURCE_GIT_COMMIT in Dockerfiles for operator-controller and catalogd Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com> UPSTREAM: <carry>: add openshift specific build target to pass commit info downstream Signed-off-by: Ankita Thomas <ankithom@redhat.com> UPSTREAM: <carry>: add source commit into binaries when linking - Removes extra GIT_COMMIT set - fixup Dockerfiles after rebase - consider "" unset so build-info can fill commit/date - double quote go flags & honor GIT_COMMIT if set - improve robustness of build-info parsing - Trim whitespace on all version fields - isUnset and valueOrUnknown now call strings.TrimSpace - Avoid clobbering values injected via ldflags - set repoState from build-info only when repoState is still unset - set version from build-info only when unset and build-info value is non-empty UPSTREAM: <carry>: OTE add first test from openshift/origin olmv1.go UPSTREAM: <carry>: Migrate tasks from openshift/origin olm v1.go file which are remaining This commit moves the final OLMv1 tests from openshift/origin/test/extended/olm/olmv1.go to their proper location in this repository. This migration is part of a larger effort to streamline development by co-locating tests with the component they validate. This will reduce CI overhead and allow for faster, more atomic changes. Assisted-by: Gemini UPSTREAM: <carry>: OTE - How to test locally with OCP instances UPSTREAM: <carry>: [OTE] Refac: refac helper and olmv1 test to create namespace instead to use pre-existent UPSTREAM: <carry>: [OTE] add webhook tests Migrates OLMv1 webhook operator tests from using external YAML files to defining resources in Go structs. This change removes file dependencies, improving test reliability and simplifying test setup. The migration is a refactoring of code from openshift/origin#30059. The new code uses better naming conventions and adapts the tests to work with a controller-runtime client, enhancing test consistency and maintainability. The migration covers all core test scenarios: - Validating, mutating, and conversion webhooks. - Certificate and secret rotation tolerance. Assisted-by: Gemini UPSTREAM: <carry>: OTE: rewrite the upgrade incompatible operator test This test replaces the existing upgrade incompatible test. The main change is that operator and catalog bundles are created on-the-fly to support OCP 4.20. This means we are no longer dependent on public operators for this test. This creates new bundles in the OCP ImageRegistry, this requires using a number of OCP APIs, including using a raw API URL to invoke the build. This is done by invoking an external k8s client (either `oc` or `kubectl`), and passing it a tarball of the bundle to be created. So, it can't be done by the golang k8sClient normally available (i.e. the create input is a tarball not a YAML file). This introduces the use of go-bindata to store the bundle contents. It also pulls in openshift mage, buld and operator APIs. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Handle service-ca cert availability/rotation There is problem when the service-ca certificate is not available at pod start. This is an issue because the SystemCertPool is created from SSL_CERT_DIR, which may include the empty service-ca. The SystemCertPool is never regenerated during the lifetime of the program execution, so it will never get updated when the service-ca is filled. Thus, we need to use --pull-cas-dir to reference the CAs that we want to use. This will also allow OLMv1 to reload the service-ca when it is reloaded (after 2 years, mind you). Removing the SSL_CERT_DIR setting, and adding the --pull-cas-dir flag ought to be equivalent to what we have now (i.e. SSL_CERT_DIR and no --pull-cas-dir), except that rotation will be handled better. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] add webhook tests Revert "UPSTREAM: <carry>: [OTE] add webhook tests" This reverts commit 9963614. UPSTREAM: <carry>: Upgrade OCP Catalog images from 4.19 to 4.20 UPSTREAM: <carry>: Remove bindata generation from build Using go-bindata is causing problems with ART builds. This removes the use of go-bindata from the builds. This will subsequently require that users MANUALLY run the `bindata` target to refresh the bindata, or use the `build-update` target. This is a quickfix to put out the fire. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: [OTE] Add webhook tests - Add dumping of container logs and `kubectl describe pods` output for better diagnostics. - Include targeted certificate details dump (`tls.crt` parse) when failures occur. - Add additional check to verify webhook responsiveness after certificate rotation. This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini UPSTREAM: <carry>: OTE add logs and dumps for olmv1 test and fix helper for clusterextensions UPSTREAM: <carry>: [OTE] Migrate preflight checks from openshift/origin Migrated OLMv1 operator preflight checks from using external YAML files to defining ClusterRole permissions directly in Go structs. This improves test reliability and simplifies test setup by removing file dependencies. The changes ensure precise replication of original test scenarios, including specific permission omissions for services, create verbs, ClusterRoleBindings, ConfigMap resourceNames, and escalate/bind verbs. Assisted-by: Gemini UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation This change is a refactor of code from openshift/origin#30059. Assisted-by: Gemini UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests UPSTREAM: <carry>: remove obsolete owners Signed-off-by: grokspawn <jordan@nimblewidget.com> UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin This commit migrates the olmv1_catalog set of tests from openshift/origin to OTE as part the broad effort to migrate all tests. Assisted-by: Gemini UPSTREAM: <carry>: Migrate single/own namespace tests This commit migrates the OLMv1 single and own namespace watch mode tests from openshift/origin/test/extended/olm/olmv1-singleownnamespace.go to this repository. This is part of the effort to move component-specific tests into their respective downstream locations. Assisted-by: Gemini UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. This reverts commit 0bb1953. UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation This reverts commit e9e3220. UPSTREAM: <carry>: Ensure unique name for bad-catalog tests UPSTREAM: <carry>: Revert "Handle service-ca cert availability/rotation" This reverts commit 9cc13d8. UPSTREAM: <carry>: grant QE approver permission for OTE UPSTREAM: <carry>: Update webhook ote tests to use latest webhook-operator Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> UPSTREAM: <carry>: update operator-controller to v1.5.1 UPSTREAM: <carry>: configure watchnamespace using spec.config for OTE tests UPSTREAM: <carry>: add jiazha to approvers UPSTREAM: <carry>: Create combined manifests for comparison Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Use Helm charts for openshift manifests Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: add support for tests-private cases and add the case UPSTREAM: <carry>: Fix cp-manifests copying of helm charts The method used to copy the helm charts is including an extra `helm` directory in the destination path, that is making the cluster-olm-operator code just a bit more complicated than it needs to be. This fixes the copy location. Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Remove kustomize manifests from images and repo Now that helm manifests are being used to dynamically generate the manifests, the pre-generated manifests are no longer needed. So, we can remove them from the repo and the images. However, because we still want to verify the manifests are "good", we are still creating a "single-file" version of the manifests for verification purposes, and to allow us to see what changes are happening to the manifests (from upstream and/or downstream sources). Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Add pedjak and trgeiger as reviewers UPSTREAM: <carry>: migrate more cases from tests-private and enhance suites with filters UPSTREAM: <carry>: Updating ose-olm-operator-controller-container image to be consistent with ART for 4.21 Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/ose-olm-operator-controller.yml UPSTREAM: <carry>: Updating ose-olm-catalogd-container image to be consistent with ART for 4.21 Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/ose-olm-catalogd.yml UPSTREAM: <carry>: OTE: Enable disconnected environment and build test operator controller image Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com> UPSTREAM: <carry>: for incompatible test add func to wait builder and deployer SA creation by OCP controller UPSTREAM: <carry>: Fix VERSION replacement in catalog bindata Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: check kubeconfig only run-test and run-suite UPSTREAM: <carry>: Clean up cp-manifests There is no longer a need to copy conditionally Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: Update does-not-exist and simple install to work in a disconnected environment Signed-off-by: Todd Short <todd.short@me.com> UPSTREAM: <carry>: support webhook case in disconnected UPSTREAM: <carry>: Consolidate build API This consolidates the in-cluster building of a bundle and catalog. The catalog and bundle bindata are inputs, along with a set of replacements so that catalog and bundle templates can be used to create the images. This can be done in the BeforeEach() for a set of tests that use the same data. Signed-off-by: Todd Short <todd.short@me.com>
…images from openshift/catalogd/manifests.yaml
Signed-off-by: Todd Short <todd.short@me.com>
…oss to avoid flakes
Signed-off-by: Todd Short <todd.short@me.com>
…uess and waiting for k8s cleanups Co-Author: kuiwang@redhat.com
…nts ( Follow-Up of: 714977c )
… uninstall Assisted-by: Cursor
… format Fix k8s.io/kubernetes replace version from v1.30.1-0... to v0.0.0-... format to resolve bumper tool verification failures. Add hack/ocp-replace.sh script to manage OCP fork replaces properly. Assisted-by: Cursor
…row job for migrated qe cases
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The current pod simply does a `sleep 1000`, which means that the startup, liveness and readiness probes all fail. Use a busybox containter to run a simple script and httpd server to emulate the probes.
openshift-ci-robot
added
the
jira/valid-bug
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-78211, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-78455, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
@camilamacedo86: This pull request references Jira Issue OCPBUGS-78211, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-78455, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
…t in OTE tests Update all remaining references to ClusterExtensionRevision in openshift/tests-extension to use ClusterObjectSet, matching the upstream rename in operator-framework/operator-controller#2589. Files updated: - test/qe/specs/olmv1_ce.go: RBAC resource names and comments - test/olmv1-preflight.go: scenario constants, test names, RBAC rules - .openshift-tests-extension/openshift_payload_olmv1.json: test name - pkg/bindata/qe/bindata.go: embedded RBAC templates - test/qe/testdata/olm/sa-nginx-limited-boxcutter.yaml: RBAC resources - test/qe/testdata/olm/sa-nginx-insufficient-operand-rbac-boxcutter.yaml: RBAC resources Signed-off-by: Camila Macedo <cmacedo@redhat.com> Made-with: Cursor
camilamacedo86
force-pushed
the
tmshort-synchronize
branch
from
3297c37 to
15dd2c4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
helm/olmv1/templates/crds/customresourcedefinition-clusterobjectsets.olm.operatorframework.io.yml (1)
3-3: Fix the GA placeholder path typo now.The commented GA path has an extra
s(clusterobjectsetss.yaml), which is easy to miss when this is eventually enabled.Proposed fix
-{{- /* Add when GA: tpl (.Files.Get "base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsetss.yaml") . */}} +{{- /* Add when GA: tpl (.Files.Get "base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsets.yaml") . */}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/olmv1/templates/crds/customresourcedefinition-clusterobjectsets.olm.operatorframework.io.yml` at line 3, The GA placeholder contains a typo in the tpl call: it references "base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsetss.yaml" (extra "s"); update the tpl invocation to point to the correct filename "base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsets.yaml" so the commented line reads {{- /* Add when GA: tpl (.Files.Get "base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsets.yaml") . */}}; ensure only the filename is corrected and surrounding templating (the tpl and .Files.Get usage) remains unchanged.internal/operator-controller/applier/boxcutter_test.go (1)
1309-1315: Pin the storage namespace in migrator tests too.The
Boxcutterfixtures in this file already make the storage namespace explicit, but theseBoxcutterStorageMigratorsetups still rely on the zero value. With secret-backed revision storage, that can let wrong-namespace regressions slip through these tests.Also applies to: 1376-1382, 1431-1437, 1499-1505, 1577-1583, 1660-1666, 1746-1752, 1779-1785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator-controller/applier/boxcutter_test.go` around lines 1309 - 1315, The migrator tests instantiate applier.BoxcutterStorageMigrator but omit an explicit storage namespace, allowing the zero-value namespace to hide regressions; update each BoxcutterStorageMigrator construction (e.g., the instance with RevisionGenerator brb, ActionClientGetter mag, Client client, Scheme testScheme, FieldOwner "test-owner") to set StorageNamespace to the same explicit namespace used by the Boxcutter fixtures (pin the namespace string), and apply the same change to the other listed occurrences (around the constructors at the other locations) so all migrator tests use the explicit storage namespace.internal/operator-controller/controllers/boxcutter_reconcile_steps.go (1)
113-115: Refresh revision state afterapplybefore mirroring status.
applycan create or update aClusterObjectSet, but Lines 128-158 still buildext.Statusfromstate.revisionStates, which was captured before Line 114. On a new rollout,activeRevisions, mirrored Progressing, and install metadata stay one reconcile behind.Also applies to: 128-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator-controller/controllers/boxcutter_reconcile_steps.go` around lines 113 - 115, After apply returns, refresh the cached revision state before building ext.Status: the code currently calls apply(...) and then immediately builds ext.Status from state.revisionStates (used to mirror Progressing/activeRevisions), which was captured before apply and causes status to lag; fix by reloading/recomputing state.revisionStates right after apply succeeds (call the same helper you used earlier to populate revisionStates or add a small helper like refreshRevisionStates(ctx, state)) so that the subsequent ext.Status construction uses the up-to-date ClusterObjectSet/revision information (references: apply, state.revisionStates, ext.Status).internal/operator-controller/controllers/resolve_ref_test.go (1)
71-87: Consider verifying resolved object content in happy path tests.The test verifies that reconciliation succeeds without error, but doesn't assert that the resolved object was correctly passed to the revision engine. The
mockEngine.reconcilediscards the revision without inspection.This could hide bugs where ref resolution succeeds but produces incorrect object content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator-controller/controllers/resolve_ref_test.go` around lines 71 - 87, The test's happy path doesn't assert that the resolved Revision passed into mockRevisionEngine.reconcile is correct; update the mockEngine in resolve_ref_test.go so its reconcile implementation captures the incoming machinerytypes.Revision parameter (e.g., store it in a local variable or channel) and then add assertions after reconciler.Reconcile(...) to validate the resolved Revision's object content/fields (for example inspect .Request, .ResolvedObjects or specific object metadata) to ensure the ref resolution produced the expected object before returning nil error.internal/operator-controller/applier/secretpacker.go (1)
20-29: Note:gzipThresholdequalsmaxSecretDataSize.Both constants are set to 900 KiB. This means compression only kicks in when an object is already at the size limit. This is likely intentional (compress only when necessary to fit), but worth confirming this is the desired behavior since objects just under 900 KiB won't be compressed even if they would benefit from compression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/operator-controller/applier/secretpacker.go` around lines 20 - 29, The gzipThreshold currently equals maxSecretDataSize (both 900*1024), which means compression only occurs at the size limit; either lower gzipThreshold so objects below the Secret chunk size are compressed (e.g., set gzipThreshold to a smaller constant or derive it from maxSecretDataSize like maxSecretDataSize/2), or explicitly document the intentional equality; update the constant gzipThreshold or add a clarifying comment near maxSecretDataSize and gzipThreshold so future readers know whether compression should be triggered before hitting the Secret size limit (referencing the symbols maxSecretDataSize and gzipThreshold).test/e2e/steps/steps.go (2)
785-804: Make the ref-Secretchecks eventual.These helpers do a single
listRefSecrets/assert pass even though their docstrings say "Polls with timeout". If a scenario calls them right afterClusterObjectSet is applied, they race the controller'sSecretcreation/labeling and can flake.Also applies to: 806-838, 840-876
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/steps/steps.go` around lines 785 - 804, The current ClusterObjectSetRefSecretsAreImmutable (and the sibling ref-Secret check helpers referenced around lines 806-876) perform a single list/assert pass but their docstrings promise polling; change them to poll until success or timeout instead of returning immediately. Replace the direct listRefSecrets + immediate assertions in ClusterObjectSetRefSecretsAreImmutable and the other ref-Secret helpers with a retry loop (e.g., wait.PollImmediate or equivalent) that repeatedly calls listRefSecrets(ctx, revisionName), checks that len(secrets) > 0 and that every secret s has s.Immutable != nil && *s.Immutable, and only returns nil when the condition is met or a timeout/context deadline is reached; on timeout return a descriptive error including the last observed state. Ensure you use the same revisionName substitution logic (substituteScenarioVars) and preserve context cancellation.
1613-1650: Share the ref-resolution helper with the controller.This is now a second copy of the gzip detection, decompression limit, and JSON decoding path from
ClusterObjectSetReconciler.resolveObjectRef. Keeping them separate will drift over time and can make E2E validate a different contract than the reconciler.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/steps/steps.go` around lines 1613 - 1650, Duplicate gzip-detection, decompression limit, and JSON decoding logic in test/e2e/steps.resolveObjectRef should be extracted and reused by the controller to avoid drift; create a single exported helper (e.g., package util.ResolveObjectRef or ResolveObjectFromSecret) that implements the logic (kubectl/get secret fetch, gzip auto-detect by magic bytes, max decompressed size check, JSON unmarshal into unstructured) and have both test/e2e/steps.resolveObjectRef and ClusterObjectSetReconciler.resolveObjectRef call that new helper instead of duplicating the code; update imports and call sites to use the shared function so the controller and E2E test share the exact same behavior and limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/operator-controller/controllers/clusterobjectset_controller.go`:
- Around line 348-359: The UpdateFunc in skipProgressDeadlineExceededPredicate
currently blocks all non-delete updates for ClusterObjectSet once
ProgressDeadlineExceeded is set, which prevents later generation/spec changes
like spec.lifecycleState=Archived from being processed; modify the predicate
(skipProgressDeadlineExceededPredicate.UpdateFunc) to still return true when the
update includes meaningful spec/generation changes by comparing e.ObjectOld and
e.ObjectNew (e.g., rev.Generation differs or rev.Spec.LifecycleState changed) or
when any non-status fields changed, and only suppress updates that are purely
status churn when meta.FindStatusCondition(rev.Status.Conditions,
ClusterObjectSetTypeProgressing) indicates ReasonProgressDeadlineExceeded and
the update is status-only; keep deletion handling as-is.
- Around line 546-549: Before reading the Secret payload in the ClusterObjectSet
reconcile, reject mutable Secrets by checking the Secret.Immutable field: if
secret.Immutable is nil or not true, return an error (or requeue) indicating the
Secret must be immutable; place this check immediately before the existing
lookup that reads secret.Data[ref.Key] (the block that declares data, ok :=
secret.Data[ref.Key]) so the controller never consumes a Secret whose content
can change without ClusterObjectSet generation changes.
- Line 68: The resolveObjectRef implementation uses the controller's cached
client (c.Client.Get) to read Secret refs which forces a cluster-wide Secret
informer; add an uncached reader field APIReader to the
ClusterObjectSetReconciler struct, wire it in main.go by assigning
mgr.GetAPIReader() to the reconciler before starting the manager, and change
resolveObjectRef to use r.APIReader.Get(...) for Secret (corev1.Secret) reads
instead of c.Client.Get so those lookups bypass the cache and avoid creating a
Secret informer.
In `@openshift/operator-controller/manifests-experimental.yaml`:
- Around line 579-582: The bundle is missing the ClusterObjectSet CRD and RBAC
rules referenced by the ClusterExtension docs and used by the controller; add a
CRD manifest for clusterobjectsets.olm.operatorframework.io and corresponding
RBAC (ClusterRole/Role and RoleBinding/ClusterRoleBinding) entries granting
verbs for resources clusterobjectsets, clusterobjectsets/status and
clusterobjectsets/finalizers so the controller can list/create/update/watch
them. Ensure the manifests in manifests-experimental.yaml include the
ClusterObjectSet CRD definition and that the controller's ServiceAccount (used
by the controller reconcile loop and feature gate BoxcutterRuntime) is bound to
a ClusterRole that contains those rules so code paths that call list/create on
ClusterObjectSet succeed at runtime.
In `@openshift/tests-extension/test/olmv1-preflight.go`:
- Around line 188-191: The preflight RBAC rule for
"clusterobjectsets/finalizers" uses ResourceNames: []string{ceName} but QE
expects Boxcutter-created revision-scoped names (first revision includes a "-1"
suffix); update the ResourceNames entry to use ceName + "-1" (i.e., append "-1"
to the ceName variable) so the ClusterObjectSet finalizer permission matches
actual runtime naming and fixes negative-test false failures referencing ceName,
particularly where ceName is used in the RBAC rule within the preflight setup.
In
`@testdata/images/bundles/large-crd-operator/v1.0.0/manifests/largecrdoperator.clusterserviceversion.yaml`:
- Around line 57-92: The YAML places serviceAccountName inside the container
definition (under the container named busybox-httpd-container) instead of at the
pod spec level; move the serviceAccountName key out of the containers list so it
is a sibling of containers, volumes, and terminationGracePeriodSeconds in the
spec block (i.e., immediately under spec, not under the busybox-httpd-container
entry) so Kubernetes will honor the service account setting.
---
Nitpick comments:
In
`@helm/olmv1/templates/crds/customresourcedefinition-clusterobjectsets.olm.operatorframework.io.yml`:
- Line 3: The GA placeholder contains a typo in the tpl call: it references
"base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsetss.yaml"
(extra "s"); update the tpl invocation to point to the correct filename
"base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsets.yaml"
so the commented line reads {{- /* Add when GA: tpl (.Files.Get
"base/operator-controller/crd/standard/olm.operatorframework.io_clusterobjectsets.yaml")
. */}}; ensure only the filename is corrected and surrounding templating (the
tpl and .Files.Get usage) remains unchanged.
In `@internal/operator-controller/applier/boxcutter_test.go`:
- Around line 1309-1315: The migrator tests instantiate
applier.BoxcutterStorageMigrator but omit an explicit storage namespace,
allowing the zero-value namespace to hide regressions; update each
BoxcutterStorageMigrator construction (e.g., the instance with RevisionGenerator
brb, ActionClientGetter mag, Client client, Scheme testScheme, FieldOwner
"test-owner") to set StorageNamespace to the same explicit namespace used by the
Boxcutter fixtures (pin the namespace string), and apply the same change to the
other listed occurrences (around the constructors at the other locations) so all
migrator tests use the explicit storage namespace.
In `@internal/operator-controller/applier/secretpacker.go`:
- Around line 20-29: The gzipThreshold currently equals maxSecretDataSize (both
900*1024), which means compression only occurs at the size limit; either lower
gzipThreshold so objects below the Secret chunk size are compressed (e.g., set
gzipThreshold to a smaller constant or derive it from maxSecretDataSize like
maxSecretDataSize/2), or explicitly document the intentional equality; update
the constant gzipThreshold or add a clarifying comment near maxSecretDataSize
and gzipThreshold so future readers know whether compression should be triggered
before hitting the Secret size limit (referencing the symbols maxSecretDataSize
and gzipThreshold).
In `@internal/operator-controller/controllers/boxcutter_reconcile_steps.go`:
- Around line 113-115: After apply returns, refresh the cached revision state
before building ext.Status: the code currently calls apply(...) and then
immediately builds ext.Status from state.revisionStates (used to mirror
Progressing/activeRevisions), which was captured before apply and causes status
to lag; fix by reloading/recomputing state.revisionStates right after apply
succeeds (call the same helper you used earlier to populate revisionStates or
add a small helper like refreshRevisionStates(ctx, state)) so that the
subsequent ext.Status construction uses the up-to-date ClusterObjectSet/revision
information (references: apply, state.revisionStates, ext.Status).
In `@internal/operator-controller/controllers/resolve_ref_test.go`:
- Around line 71-87: The test's happy path doesn't assert that the resolved
Revision passed into mockRevisionEngine.reconcile is correct; update the
mockEngine in resolve_ref_test.go so its reconcile implementation captures the
incoming machinerytypes.Revision parameter (e.g., store it in a local variable
or channel) and then add assertions after reconciler.Reconcile(...) to validate
the resolved Revision's object content/fields (for example inspect .Request,
.ResolvedObjects or specific object metadata) to ensure the ref resolution
produced the expected object before returning nil error.
In `@test/e2e/steps/steps.go`:
- Around line 785-804: The current ClusterObjectSetRefSecretsAreImmutable (and
the sibling ref-Secret check helpers referenced around lines 806-876) perform a
single list/assert pass but their docstrings promise polling; change them to
poll until success or timeout instead of returning immediately. Replace the
direct listRefSecrets + immediate assertions in
ClusterObjectSetRefSecretsAreImmutable and the other ref-Secret helpers with a
retry loop (e.g., wait.PollImmediate or equivalent) that repeatedly calls
listRefSecrets(ctx, revisionName), checks that len(secrets) > 0 and that every
secret s has s.Immutable != nil && *s.Immutable, and only returns nil when the
condition is met or a timeout/context deadline is reached; on timeout return a
descriptive error including the last observed state. Ensure you use the same
revisionName substitution logic (substituteScenarioVars) and preserve context
cancellation.
- Around line 1613-1650: Duplicate gzip-detection, decompression limit, and JSON
decoding logic in test/e2e/steps.resolveObjectRef should be extracted and reused
by the controller to avoid drift; create a single exported helper (e.g., package
util.ResolveObjectRef or ResolveObjectFromSecret) that implements the logic
(kubectl/get secret fetch, gzip auto-detect by magic bytes, max decompressed
size check, JSON unmarshal into unstructured) and have both
test/e2e/steps.resolveObjectRef and ClusterObjectSetReconciler.resolveObjectRef
call that new helper instead of duplicating the code; update imports and call
sites to use the shared function so the controller and E2E test share the exact
same behavior and limits.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fbf2050-5c7a-4d34-ac56-57d4dc957793
⛔ Files ignored due to path filters (3)
openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterextension_types.gois excluded by!**/vendor/**openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/clusterobjectset_types.gois excluded by!**/vendor/**openshift/tests-extension/vendor/github.com/operator-framework/operator-controller/api/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**
📒 Files selected for processing (78)
.claude/commands/list-e2e-steps.mdAGENTS.mdapi/v1/clusterextension_types.goapi/v1/clusterextensionrevision_types_test.goapi/v1/clusterobjectset_types.goapi/v1/clusterobjectset_types_test.goapi/v1/validation_test.goapi/v1/zz_generated.deepcopy.goapplyconfigurations/api/v1/clusterextensionrevisionstatus.goapplyconfigurations/api/v1/clusterextensionstatus.goapplyconfigurations/api/v1/clusterobjectset.goapplyconfigurations/api/v1/clusterobjectsetobject.goapplyconfigurations/api/v1/clusterobjectsetphase.goapplyconfigurations/api/v1/clusterobjectsetspec.goapplyconfigurations/api/v1/clusterobjectsetstatus.goapplyconfigurations/api/v1/objectsourceref.goapplyconfigurations/api/v1/revisionstatus.goapplyconfigurations/utils.gocmd/operator-controller/main.gocommitchecker.yamlconfig/samples/olm_v1_clusterextension.yamldocs/api-reference/crd-ref-docs-gen-config.yamldocs/api-reference/olmv1-api-reference.mddocs/concepts/large-bundle-support.mdhack/tools/update-crds.shhelm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yamlhelm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterobjectsets.yamlhelm/olmv1/templates/crds/customresourcedefinition-clusterobjectsets.olm.operatorframework.io.ymlhelm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.ymlinternal/operator-controller/applier/boxcutter.gointernal/operator-controller/applier/boxcutter_test.gointernal/operator-controller/applier/externalize_test.gointernal/operator-controller/applier/phase.gointernal/operator-controller/applier/phase_test.gointernal/operator-controller/applier/provider.gointernal/operator-controller/applier/provider_test.gointernal/operator-controller/applier/secretpacker.gointernal/operator-controller/applier/secretpacker_test.gointernal/operator-controller/authorization/rbac.gointernal/operator-controller/authorization/rbac_test.gointernal/operator-controller/config/config.gointernal/operator-controller/config/config_test.gointernal/operator-controller/controllers/boxcutter_reconcile_steps.gointernal/operator-controller/controllers/clusterextension_reconcile_steps.gointernal/operator-controller/controllers/clusterobjectset_controller.gointernal/operator-controller/controllers/clusterobjectset_controller_internal_test.gointernal/operator-controller/controllers/clusterobjectset_controller_test.gointernal/operator-controller/controllers/common_controller.gointernal/operator-controller/controllers/common_controller_test.gointernal/operator-controller/controllers/resolve_ref_test.gointernal/operator-controller/controllers/revision_engine_factory.gointernal/operator-controller/labels/labels.gointernal/shared/util/test/artifacts.gomanifests/experimental-e2e.yamlmanifests/experimental.yamlopenshift/operator-controller/manifests-experimental.yamlopenshift/tests-extension/.openshift-tests-extension/openshift_payload_olmv1.jsonopenshift/tests-extension/pkg/bindata/qe/bindata.goopenshift/tests-extension/test/olmv1-preflight.goopenshift/tests-extension/test/qe/specs/olmv1_ce.goopenshift/tests-extension/test/qe/testdata/olm/sa-nginx-insufficient-operand-rbac-boxcutter.yamlopenshift/tests-extension/test/qe/testdata/olm/sa-nginx-limited-boxcutter.yamltest/e2e/README.mdtest/e2e/features/install.featuretest/e2e/features/revision.featuretest/e2e/features/status.featuretest/e2e/features/update.featuretest/e2e/steps/hooks.gotest/e2e/steps/steps.gotest/e2e/steps/testdata/olm-sa-boxcutter-no-create-rbac-template.yamltest/e2e/steps/testdata/olm-sa-boxcutter-rbac-template.yamltest/e2e/steps/testdata/pvc-probe-sa-boxcutter-rbac-template.yamltest/e2e/steps/testdata/rbac-template-for-all-extensions.yamltestdata/images/bundles/large-crd-operator/v1.0.0/manifests/largecrd.operatorframework.io_largecrdtests.yamltestdata/images/bundles/large-crd-operator/v1.0.0/manifests/largecrdoperator.clusterserviceversion.yamltestdata/images/bundles/large-crd-operator/v1.0.0/manifests/script.configmap.yamltestdata/images/bundles/large-crd-operator/v1.0.0/metadata/annotations.yamltestdata/images/catalogs/test-catalog/v1/configs/catalog.yaml
💤 Files with no reviewable changes (2)
- api/v1/clusterextensionrevision_types_test.go
- applyconfigurations/api/v1/clusterextensionrevisionstatus.go
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets,verbs=get;list;watch;update;patch;create;delete | ||
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets/status,verbs=update;patch | ||
| //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterobjectsets/finalizers,verbs=update | ||
| //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ClusterObjectSetReconciler construction:"
rg -nC4 'ClusterObjectSetReconciler\{' --type=go
echo
echo "Client/APIReader sources:"
rg -nC2 '\bGet(APIReader|Client)\s*\(' --type=go
echo
echo "Manager cache configuration relevant to Secret reads:"
rg -nC3 'ReaderFailOnMissingInformer|DisableFor|ByObject|corev1\.Secret' --type=goRepository: openshift/operator-framework-operator-controller
Length of output: 50405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== resolveObjectRef Secret read path ==="
rg -nA10 "func.*resolveObjectRef" internal/operator-controller/controllers/clusterobjectset_controller.go | head -50
echo
echo "=== Secret Get calls in the file ==="
rg -n "Secret.*Get\|\.Get.*Secret" internal/operator-controller/controllers/clusterobjectset_controller.go
echo
echo "=== Manager cache configuration in main.go ==="
rg -nB5 -A15 "NewManager\|New(" cmd/operator-controller/main.go | grep -A15 "cache\|Cache\|Secret\|APIReader\|DisableFor" | head -80
echo
echo "=== Check if APIReader is passed to reconciler ==="
rg -n "APIReader" internal/operator-controller/controllers/clusterobjectset_controller.go cmd/operator-controller/main.go
echo
echo "=== ClusterObjectSetReconciler struct definition ==="
rg -nA20 "type ClusterObjectSetReconciler struct" internal/operator-controller/controllers/clusterobjectset_controller.go | head -30Repository: openshift/operator-framework-operator-controller
Length of output: 734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Manager setup in main.go ==="
sed -n '650,750p' cmd/operator-controller/main.go | grep -n "NewManager\|cache\|Cache\|Secret"
echo
echo "=== Full manager config around NewManager ==="
rg -n "mgr, err := ctrl.NewManager" cmd/operator-controller/main.go -A30
echo
echo "=== APIReader field in ClusterObjectSetReconciler ==="
sed -n '1,100p' internal/operator-controller/controllers/clusterobjectset_controller.go | rg -n "type ClusterObjectSetReconciler" -A15
echo
echo "=== RBAC comment at line 68 ==="
sed -n '65,72p' internal/operator-controller/controllers/clusterobjectset_controller.goRepository: openshift/operator-framework-operator-controller
Length of output: 3924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== cacheOptions definition ==="
rg -n "cacheOptions\s*:=" cmd/operator-controller/main.go -B5 -A10
echo
echo "=== Check for any cache.ByObject or DisableFor configuration ==="
rg -n "ByObject\|DisableFor\|crcache" cmd/operator-controller/main.go -B2 -A5
echo
echo "=== Verify line 68 in the file ==="
sed -n '66,75p' internal/operator-controller/controllers/clusterobjectset_controller.go | cat -nRepository: openshift/operator-framework-operator-controller
Length of output: 1039
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full cacheOptions block ==="
sed -n '249,280p' cmd/operator-controller/main.go
echo
echo "=== Check if Secret appears in cache config ==="
rg -n "Secret" cmd/operator-controller/main.go | head -20
echo
echo "=== Line 68 context (around RBAC Secret comment) ==="
sed -n '65,72p' internal/operator-controller/controllers/clusterobjectset_controller.go | cat -n
echo
echo "=== Verify resolveObjectRef uses c.Client, not APIReader ==="
sed -n '539,560p' internal/operator-controller/controllers/clusterobjectset_controller.go | cat -nRepository: openshift/operator-framework-operator-controller
Length of output: 4398
Use an uncached reader for ref Secrets.
The resolveObjectRef method reads Secrets via c.Client.Get(), which routes through the cached delegating client. Since the manager's cache configuration does not explicitly exclude corev1.Secret, controller-runtime will create a cluster-wide Secret informer to satisfy these point reads—a significant memory and PII footprint for this functionality. The ClusterObjectSetReconciler does not currently have an APIReader field. Add one, wire it from the manager in main.go (via mgr.GetAPIReader()), and use it for ref reads to keep Secret access uncached and scoped to point lookups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/operator-controller/controllers/clusterobjectset_controller.go` at
line 68, The resolveObjectRef implementation uses the controller's cached client
(c.Client.Get) to read Secret refs which forces a cluster-wide Secret informer;
add an uncached reader field APIReader to the ClusterObjectSetReconciler struct,
wire it in main.go by assigning mgr.GetAPIReader() to the reconciler before
starting the manager, and change resolveObjectRef to use r.APIReader.Get(...)
for Secret (corev1.Secret) reads instead of c.Client.Get so those lookups bypass
the cache and avoid creating a Secret informer.
| skipProgressDeadlineExceededPredicate := predicate.Funcs{ | ||
| UpdateFunc: func(e event.UpdateEvent) bool { | ||
| rev, ok := e.ObjectNew.(*ocv1.ClusterExtensionRevision) | ||
| rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet) | ||
| if !ok { | ||
| return true | ||
| } | ||
| // allow deletions to happen | ||
| if !rev.DeletionTimestamp.IsZero() { | ||
| return true | ||
| } | ||
| if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { | ||
| if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { | ||
| return false |
There was a problem hiding this comment.
Don't block archive transitions after a progress timeout.
Once ProgressDeadlineExceeded is set, this predicate drops every later non-delete update. That includes later generation changes such as spec.lifecycleState=Archived, so a timed-out ClusterObjectSet can miss its archive/teardown path entirely.
💡 Keep status-only churn suppressed, but allow generation changes
skipProgressDeadlineExceededPredicate := predicate.Funcs{
UpdateFunc: func(e event.UpdateEvent) bool {
rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet)
if !ok {
return true
}
// allow deletions to happen
if !rev.DeletionTimestamp.IsZero() {
return true
}
+ if oldRev, ok := e.ObjectOld.(*ocv1.ClusterObjectSet); ok && rev.Generation != oldRev.Generation {
+ return true
+ }
if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded {
return false
}
return true
},
}📝 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.
| skipProgressDeadlineExceededPredicate := predicate.Funcs{ | |
| UpdateFunc: func(e event.UpdateEvent) bool { | |
| rev, ok := e.ObjectNew.(*ocv1.ClusterExtensionRevision) | |
| rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet) | |
| if !ok { | |
| return true | |
| } | |
| // allow deletions to happen | |
| if !rev.DeletionTimestamp.IsZero() { | |
| return true | |
| } | |
| if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { | |
| if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { | |
| return false | |
| skipProgressDeadlineExceededPredicate := predicate.Funcs{ | |
| UpdateFunc: func(e event.UpdateEvent) bool { | |
| rev, ok := e.ObjectNew.(*ocv1.ClusterObjectSet) | |
| if !ok { | |
| return true | |
| } | |
| // allow deletions to happen | |
| if !rev.DeletionTimestamp.IsZero() { | |
| return true | |
| } | |
| if oldRev, ok := e.ObjectOld.(*ocv1.ClusterObjectSet); ok && rev.Generation != oldRev.Generation { | |
| return true | |
| } | |
| if cnd := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing); cnd != nil && cnd.Status == metav1.ConditionFalse && cnd.Reason == ocv1.ReasonProgressDeadlineExceeded { | |
| return false |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/operator-controller/controllers/clusterobjectset_controller.go`
around lines 348 - 359, The UpdateFunc in skipProgressDeadlineExceededPredicate
currently blocks all non-delete updates for ClusterObjectSet once
ProgressDeadlineExceeded is set, which prevents later generation/spec changes
like spec.lifecycleState=Archived from being processed; modify the predicate
(skipProgressDeadlineExceededPredicate.UpdateFunc) to still return true when the
update includes meaningful spec/generation changes by comparing e.ObjectOld and
e.ObjectNew (e.g., rev.Generation differs or rev.Spec.LifecycleState changed) or
when any non-status fields changed, and only suppress updates that are purely
status churn when meta.FindStatusCondition(rev.Status.Conditions,
ClusterObjectSetTypeProgressing) indicates ReasonProgressDeadlineExceeded and
the update is status-only; keep deletion handling as-is.
| data, ok := secret.Data[ref.Key] | ||
| if !ok { | ||
| return nil, fmt.Errorf("key %q not found in Secret %s/%s", ref.Key, ref.Namespace, ref.Name) | ||
| } |
There was a problem hiding this comment.
Reject mutable ref Secrets.
Without checking secret.Immutable, the effective rollout contents can change later without any ClusterObjectSet spec/generation change. The controller will then consume the new payload on some unrelated reconcile.
🛡️ Minimal guard before reading the payload
+ if secret.Immutable == nil || !*secret.Immutable {
+ return nil, fmt.Errorf("Secret %s/%s referenced by object ref must be immutable", ref.Namespace, ref.Name)
+ }
+
data, ok := secret.Data[ref.Key]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/operator-controller/controllers/clusterobjectset_controller.go`
around lines 546 - 549, Before reading the Secret payload in the
ClusterObjectSet reconcile, reject mutable Secrets by checking the
Secret.Immutable field: if secret.Immutable is nil or not true, return an error
(or requeue) indicating the Secret must be immutable; place this check
immediately before the existing lookup that reads secret.Data[ref.Key] (the
block that declares data, ok := secret.Data[ref.Key]) so the controller never
consumes a Secret whose content can change without ClusterObjectSet generation
changes.
| spec: | ||
| terminationGracePeriodSeconds: 0 | ||
| volumes: | ||
| - name: scripts | ||
| configMap: | ||
| name: httpd-script | ||
| defaultMode: 0755 | ||
| containers: | ||
| - name: busybox-httpd-container | ||
| image: busybox:1.37 | ||
| command: ["/scripts/httpd.sh"] | ||
| ports: | ||
| - containerPort: 80 | ||
| volumeMounts: | ||
| - name: scripts | ||
| mountPath: /scripts | ||
| readOnly: true | ||
| startupProbe: | ||
| httpGet: | ||
| path: /started | ||
| port: 80 | ||
| failureThreshold: 30 | ||
| periodSeconds: 10 | ||
| livenessProbe: | ||
| httpGet: | ||
| path: /live | ||
| port: 80 | ||
| failureThreshold: 1 | ||
| periodSeconds: 2 | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /ready | ||
| port: 80 | ||
| initialDelaySeconds: 1 | ||
| periodSeconds: 1 | ||
| serviceAccountName: simple-bundle-manager |
There was a problem hiding this comment.
serviceAccountName is incorrectly nested inside container spec.
The serviceAccountName field (line 92) is placed inside the container definition, but it should be at the pod spec level (sibling to containers, volumes, terminationGracePeriodSeconds). This will cause the field to be ignored.
Proposed fix
containers:
- name: busybox-httpd-container
image: busybox:1.37
command: ["/scripts/httpd.sh"]
ports:
- containerPort: 80
volumeMounts:
- name: scripts
mountPath: /scripts
readOnly: true
startupProbe:
httpGet:
path: /started
port: 80
failureThreshold: 30
periodSeconds: 10
livenessProbe:
httpGet:
path: /live
port: 80
failureThreshold: 1
periodSeconds: 2
readinessProbe:
httpGet:
path: /ready
port: 80
initialDelaySeconds: 1
periodSeconds: 1
- serviceAccountName: simple-bundle-manager
+ serviceAccountName: simple-bundle-manager📝 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.
| spec: | |
| terminationGracePeriodSeconds: 0 | |
| volumes: | |
| - name: scripts | |
| configMap: | |
| name: httpd-script | |
| defaultMode: 0755 | |
| containers: | |
| - name: busybox-httpd-container | |
| image: busybox:1.37 | |
| command: ["/scripts/httpd.sh"] | |
| ports: | |
| - containerPort: 80 | |
| volumeMounts: | |
| - name: scripts | |
| mountPath: /scripts | |
| readOnly: true | |
| startupProbe: | |
| httpGet: | |
| path: /started | |
| port: 80 | |
| failureThreshold: 30 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| httpGet: | |
| path: /live | |
| port: 80 | |
| failureThreshold: 1 | |
| periodSeconds: 2 | |
| readinessProbe: | |
| httpGet: | |
| path: /ready | |
| port: 80 | |
| initialDelaySeconds: 1 | |
| periodSeconds: 1 | |
| serviceAccountName: simple-bundle-manager | |
| spec: | |
| terminationGracePeriodSeconds: 0 | |
| volumes: | |
| - name: scripts | |
| configMap: | |
| name: httpd-script | |
| defaultMode: 0755 | |
| containers: | |
| - name: busybox-httpd-container | |
| image: busybox:1.37 | |
| command: ["/scripts/httpd.sh"] | |
| ports: | |
| - containerPort: 80 | |
| volumeMounts: | |
| - name: scripts | |
| mountPath: /scripts | |
| readOnly: true | |
| startupProbe: | |
| httpGet: | |
| path: /started | |
| port: 80 | |
| failureThreshold: 30 | |
| periodSeconds: 10 | |
| livenessProbe: | |
| httpGet: | |
| path: /live | |
| port: 80 | |
| failureThreshold: 1 | |
| periodSeconds: 2 | |
| readinessProbe: | |
| httpGet: | |
| path: /ready | |
| port: 80 | |
| initialDelaySeconds: 1 | |
| periodSeconds: 1 | |
| serviceAccountName: simple-bundle-manager |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@testdata/images/bundles/large-crd-operator/v1.0.0/manifests/largecrdoperator.clusterserviceversion.yaml`
around lines 57 - 92, The YAML places serviceAccountName inside the container
definition (under the container named busybox-httpd-container) instead of at the
pod spec level; move the serviceAccountName key out of the containers list so it
is a sibling of containers, volumes, and terminationGracePeriodSeconds in the
spec block (i.e., immediately under spec, not under the busybox-httpd-container
entry) so Kubernetes will honor the service account setting.
|
/test e2e-aws-techpreview-olmv1-ext |
|
The Is this something that needs to be fixed in this PR? (i.e. will #682 not pass CI?) |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, tmshort 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
|
@camilamacedo86: The following test 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. |
…s ClusterObjectSet The upstream rename of ClusterExtensionRevision to ClusterObjectSet (operator-framework/operator-controller#2589) breaks the incompatible operator detection in cluster-olm-operator. The cluster-olm-operator binary still reads ClusterExtensionRevision resources to find operators with olm.maxOpenShiftVersion, so after the rename it never detects incompatible operators and InstalledOLMOperatorsUpgradeable stays True. Skip this test when NewOLMBoxCutterRuntime feature gate is enabled until cluster-olm-operator is updated to read ClusterObjectSet. Signed-off-by: Camila Macedo <cmacedo@redhat.com> Made-with: Cursor
camilamacedo86
force-pushed
the
tmshort-synchronize
branch
from
123b41c to
83003ad
Compare
| @@ -1,4 +1,4 @@ | |||
| expectedMergeBase: 253feaef5babbdabfb4b63d1cb3a60455947be3a | |||
| expectedMergeBase: 2ca035038988bd2e5d35a71ec7ffcd25d05d6b3f | |||
There was a problem hiding this comment.
can you try removing this line and just let commit checker figure it out?
There was a problem hiding this comment.
The bumper tooling does that, so even if we remove it, it will be added in the next bumper run; so this is not the appropriate place to do it.
See: https://github.com/openshift/operator-framework-tooling
There was a problem hiding this comment.
@sanchezl
The bumper PR is: openshift/operator-framework-tooling#95
Can you please remove your request for changes, since this is not the appropriate place to remove it.
|
/test openshift-e2e-aws |
|
/lgtm |
|
@tmshort: This PR was included in a payload test run from openshift/cluster-olm-operator#191
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/41148420-2d3b-11f1-8a55-a6f097e8a2f8-0 |
|
@tmshort: This PR was included in a payload test run from openshift/cluster-olm-operator#191
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/74cf0dbc-2d3b-11f1-9bff-7a440c1b998b-0 |
16 participants
This PR is: #682
With the extra commits:
UPSTREAM: : Rename ClusterExtensionRevision to ClusterObjectSet in OTE tests
camilamacedo86
UPSTREAM: : Skip incompatible operator test when Boxcutter uses ClusterObjectSet
To do the renames in the OTE tests.
/label tide/merge-method-merge
c/c @tmshort