Skip to content

NO-JIRA: Remove apply client workaround for controller-runtime < 0.22#8693

Draft
smrtrfszm wants to merge 1 commit into
openshift:mainfrom
smrtrfszm:smrtrfszm/remove-apply-client-workaround
Draft

NO-JIRA: Remove apply client workaround for controller-runtime < 0.22#8693
smrtrfszm wants to merge 1 commit into
openshift:mainfrom
smrtrfszm:smrtrfszm/remove-apply-client-workaround

Conversation

@smrtrfszm

@smrtrfszm smrtrfszm commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

The temporary applyClient wrapper and Apply interface existed only to work around a controller-runtime bug that prevented server-side apply with the fake client. That bug was fixed in controller-runtime 0.22, which the repository now uses.

Drop the Apply interface and applyClient/fakeApplyClient wrappers in favor of the native client.Apply API. The field owner is now set once per client via client.WithFieldOwner, and manifests are applied through client.ApplyConfigurationFromUnstructured with client.ForceOwnership.

Tests construct the error case with interceptor.NewClient instead of the custom errorApplyClient, and the fake client is wrapped with client.WithFieldOwner to mirror production behavior.

Which issue(s) this PR fixes:

Fixes

Special notes for your reviewer:

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

Summary by CodeRabbit

  • Refactor
    • Manifest application now automatically derives field owner from the binary name, eliminating explicit options at each apply point
    • Simplified internal manifest application workflow by removing temporary interface abstractions
    • Updated function signatures to improve consistency in manifest application handling across the bootstrap process
    • Test suite refactored to validate updated manifest application patterns and client configuration

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 6, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@smrtrfszm: This pull request explicitly references no jira issue.

Details

In response to this:

What this PR does / why we need it:

The temporary applyClient wrapper and Apply interface existed only to work around a controller-runtime bug that prevented server-side apply with the fake client. That bug was fixed in controller-runtime 0.22, which the repository now uses.

Drop the Apply interface and applyClient/fakeApplyClient wrappers in favor of the native client.Apply API. The field owner is now set once per client via client.WithFieldOwner, and manifests are applied through client.ApplyConfigurationFromUnstructured with client.ForceOwnership.

Tests construct the error case with interceptor.NewClient instead of the custom errorApplyClient, and the fake client is wrapped with client.WithFieldOwner to mirror production behavior.

Which issue(s) this PR fixes:

Fixes

Special notes for your reviewer:

Checklist:

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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.

@openshift-ci

openshift-ci Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 34b897ef-e3e7-467d-8ff7-c766be257758

📥 Commits

Reviewing files that changed from the base of the PR and between a101e66 and da47673.

📒 Files selected for processing (2)
  • kas-bootstrap/kas_boostrap.go
  • kas-bootstrap/kas_boostrap_test.go

📝 Walkthrough

Walkthrough

This PR refactors the manifest apply plumbing in kas-bootstrap to centralize field ownership configuration. Rather than passing explicit client.FieldOwner(...) options at each apply call site, the code now derives a fieldOwner from the binary name once and wraps both the admin and bootstrap clients using client.WithFieldOwner. The temporary Apply interface and applyClient wrapper are removed, and applyManifest and applyBootstrapManifests now accept plain client.Client instances that already carry the configured owner. Tests are similarly updated to use new fake client helpers that replace the prior custom Apply implementations.

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references the main change: removing the apply client workaround that is no longer needed due to controller-runtime 0.22 support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed No Ginkgo tests found in the PR. Tests use standard Go testing.T with t.Run(). All test names are static strings with no dynamic content like pods, timestamps, UUIDs, nodes, namespaces, or IPs.
Test Structure And Quality ✅ Passed The custom check is scoped to "Ginkgo test code" but the modified test file uses traditional Go testing with Gomega, not Ginkgo. The check is not applicable to this PR.
Topology-Aware Scheduling Compatibility ✅ Passed PR removes internal apply workaround; applies no workloads or scheduling constraints. Manifests are CRDs and ClusterRoleBindings only—no pod specs, affinity, or topology constraints introduced.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No Ginkgo e2e tests are added in this PR. It only refactors unit tests using Go's standard testing package in kas-bootstrap, removing an applyClient workaround.
No-Weak-Crypto ✅ Passed No weak cryptographic algorithms, custom crypto implementations, or non-constant-time secret comparisons found. Changes only refactor Kubernetes client manifest application wrappers.
Container-Privileges ✅ Passed PR modifies Go code and tests only; no container manifests with privileged configs, hostPID/Network/IPC, SYS_ADMIN, or allowPrivilegeEscalation are introduced or modified.
No-Sensitive-Data-In-Logs ✅ Passed No sensitive data logging found. Logging only includes manifest file paths, status messages, and error messages without exposing passwords, tokens, credentials, PII, or internal hostnames.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot added the area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release label Jun 6, 2026
@openshift-ci

openshift-ci Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: smrtrfszm
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Jun 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 30.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.78%. Comparing base (8ea96d7) to head (e0f577c).

Files with missing lines Patch % Lines
kas-bootstrap/kas_boostrap.go 30.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8693      +/-   ##
==========================================
- Coverage   43.78%   43.78%   -0.01%     
==========================================
  Files         772      772              
  Lines       96000    96000              
==========================================
- Hits        42033    42032       -1     
- Misses      51055    51056       +1     
  Partials     2912     2912              
Files with missing lines Coverage Δ
kas-bootstrap/kas_boostrap.go 45.60% <30.00%> (-0.55%) ⬇️
Flag Coverage Δ
cmd-support 37.43% <ø> (ø)
cpo-hostedcontrolplane 45.91% <ø> (ø)
cpo-other 45.11% <ø> (ø)
hypershift-operator 54.03% <ø> (ø)
other 32.07% <30.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@openshift-ci

openshift-ci Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Stale PRs are closed after 21d of inactivity.

If this PR is still relevant, comment to refresh it or remove the stale label.
Mark the PR as fresh by commenting /remove-lifecycle stale.

If this PR is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2026
…r-runtime < 0.22

The temporary applyClient wrapper and Apply interface existed only to
work around a controller-runtime bug that prevented server-side apply
with the fake client. That bug was fixed in controller-runtime 0.22,
which the repository now uses.

Drop the Apply interface and applyClient/fakeApplyClient wrappers in
favor of the native client.Apply API. The field owner is now set once
per client via client.WithFieldOwner, and manifests are applied through
client.ApplyConfigurationFromUnstructured with client.ForceOwnership.

Tests construct the error case with interceptor.NewClient instead of the
custom errorApplyClient, and the fake client is wrapped with
client.WithFieldOwner to mirror production behavior.

Signed-off-by: Szepesi Tibor <Szepesi.Tibor@ibm.com>
Commit-Message-Assisted-by: Claude (via Claude Code)
@smrtrfszm smrtrfszm force-pushed the smrtrfszm/remove-apply-client-workaround branch from da47673 to e0f577c Compare July 13, 2026 10:48
@hypershift-jira-solve-ci

Copy link
Copy Markdown
Contributor

Now I have all the evidence I need. Let me produce the final report.

Test Failure Analysis Complete

Job Information

Test Failure Analysis

Error

Integration test for component hypershift-operator-main snapshot
hypershift-operator-20260713-111024-000 and scenario
hypershift-operator-enterprise-contract has failed

Task: verify | Duration: 19s | Status: StepFailed | Reason: StepFailed

Summary

This is a transient Konflux Enterprise Contract (EC) infrastructure failure, not caused by the PR code changes. The verify step of the hypershift-operator-enterprise-contract integration test scenario crashed (StepFailed) after only 19 seconds without producing any policy evaluation results. The same snapshot was simultaneously evaluated by a different EC scenario (hypershift-operator-main-enterprise-contract) which succeeded with 264 policy successes and 18 warnings. All three Konflux build pipelines (on-pull-request) completed successfully, confirming the image was built and pushed correctly. The identical EC scenarios passed on PRs #8967, #8973, and #8983 (July 9–11), and the PR only modifies kas-bootstrap Go source code with no changes to Dockerfiles, .tekton pipelines, or EC policies.

Root Cause

The ec-cli tool within the Konflux verify Tekton step failed with a hard crash (StepFailed) before it could evaluate any Enterprise Contract policies. Key indicators:

  1. Abnormally short runtime: The failing verify step completed in only 19–20 seconds, compared to the normal 23–42 seconds required for successful policy evaluation (264–279 rules). This means the ec-cli binary exited with a non-zero code before reaching the policy evaluation phase.

  2. No policy output: The failing run produced no success/warning/failure counts, unlike the passing scenarios which reported "264 success(es), 18 warning(s)". A policy violation would still produce evaluation counts — the complete absence indicates an infrastructure-level failure (e.g., image fetch error, signature retrieval failure, OPA/Rego engine crash, or resource exhaustion).

  3. Same snapshot, different outcomes: The hypershift-operator-main-enterprise-contract scenario evaluated the exact same snapshot (hypershift-operator-20260713-111024-000) and succeeded. This proves the image, its signatures, and its attestations are valid — the failure is in the pipeline execution, not the artifact content.

  4. Pattern across components: The same StepFailed pattern hit two unrelated components (hypershift-operator-main and hypershift-cli-mce-50) in their respective EC scenarios, while other components in the same scenarios succeeded. This is consistent with a transient infrastructure issue (e.g., a pod scheduling on a degraded node, a brief network partition to an internal registry or transparency log, or container resource limits being hit).

  5. Not a Sigstore outage: The public Sigstore status page reported all services online throughout the July 13 window. However, Konflux uses its own internal Rekor/Fulcio instances which are not covered by the public status page.

  6. PR changes are irrelevant: The PR modifies only kas-bootstrap/kas_boostrap.go and its test file — a Go code refactoring that removes an applyClient wrapper. No Dockerfiles, .tekton pipeline definitions, or Enterprise Contract policies were changed. The build pipeline succeeded, proving the code compiles and the container image was built correctly.

Recommendations
  1. Re-trigger the failing check: Push an empty commit or re-run the Konflux integration test to get a fresh EC evaluation. Since this is a transient infrastructure failure, a retry is expected to succeed.

  2. Verify the re-run succeeds: After re-triggering, confirm the hypershift-operator-enterprise-contract scenario produces policy evaluation results (success/warning counts) rather than StepFailed.

  3. No code changes needed: The PR code changes are correct and unrelated to this failure. All unit tests, envtest suites, linting, and verify checks passed. The Konflux build itself also succeeded.

  4. If the failure persists after retry: File an issue with the Konflux team (namespace crt-redhat-acm-tenant) referencing PipelineRun hypershift-operator-enterprise-contract-sgbpw. Request access to the verify step logs to determine the specific ec-cli error (image fetch timeout, signature validation error, OOM kill, etc.).

  5. Consider the check's merge-blocking status: The hypershift-operator-enterprise-contract check reported as failure while the hypershift-operator-main-enterprise-contract check (evaluating the same snapshot) reported as neutral. If only the latter is merge-blocking, this failure may not actually block the PR.

Evidence
Evidence Detail
Failing check Red Hat Konflux / hypershift-operator-enterprise-contract / hypershift-operator-mainfailure
Failing PipelineRun hypershift-operator-enterprise-contract-sgbpw
Verify step duration 19 seconds (abnormally short; normal is 23–42s)
Verify step status StepFailed — no policy evaluation output
Passing EC check (same snapshot) hypershift-operator-main-enterprise-contractneutral (264 successes, 18 warnings)
Second failing EC check enterprise-contract-mce-50 / hypershift-cli-mce-50failure (StepFailed, 20s)
Second passing EC check enterprise-contract-mce-50 / hypershift-release-mce-50neutral (279 successes, 38 warnings)
Build pipeline hypershift-operator-main-on-pull-requestsuccess
Same scenario on PR #8983 (Jul 11) hypershift-operator-enterprise-contractneutral (264 successes, 22 warnings)
Same scenario on PR #8973 (Jul 9) hypershift-operator-enterprise-contractneutral
Same scenario on PR #8967 (Jul 9) hypershift-operator-enterprise-contractneutral
Sigstore status (Jul 13) All services online — no incidents reported
PR code changes kas-bootstrap/kas_boostrap.go and kas_boostrap_test.go only — Go refactoring, no Dockerfile/pipeline/policy changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants