Skip to content

OCPBUGS-115110: Cover non-blocking required-SCC monitor results - #31660

Open
vsolanki12 wants to merge 1 commit into
openshift:mainfrom
vsolanki12:fix/OCPBUGS-115110
Open

vsolanki12 wants to merge 1 commit into
openshift:mainfrom
vsolanki12:fix/OCPBUGS-115110

Conversation

@vsolanki12

@vsolanki12 vsolanki12 commented Sep 18, 2026

Copy link
Copy Markdown

Summary

Add regression coverage for the existing required-SCC monitor behavior described
in OCPBUGS-115110.

This implements the issue's verification option: confirm that the existing
namespacesWithPendingSCCPinning and systemNamespaces handling makes the
reported namespaces non-blocking. It does not change SCC policy, add allowlist
entries, annotate workloads, or change the monitor's production behavior.

The new tests check:

  • The six reported namespaces emit a failed JUnit with pod diagnostics and a
    passing companion with exactly the same name, and the real monitor returns
    Succeeded.
  • Missing annotations in unlisted platform namespaces and a disallowed
    nonstandard SCC remain blocking.
  • Pinned pods, empty namespaces, and out-of-scope namespaces retain their
    existing behavior; skips are not accepted as passing companions.
  • A kube-system flake cannot hide a failure in another namespace.
  • Namespace/pod list errors propagate and make the monitor fail.

The tests use fake Kubernetes objects and execute the production checker and
monitor lifecycle. Only client construction at startup is replaced to retain
the fake client. They require no cluster access and do not test SCC admission.

Before and after

At base 147d29558f3fd344567894152cdf1ee6b9044063, the checker package reported
[no test files]. Its runtime behavior already made the reported namespaces
non-blocking. After this change, 21 focused scenarios pass: 16 classification
cases, two mixed-pod cases, one mixed-namespace aggregation case, and two
API-error cases. Validated-but-unpinned pods remain violations; diagnostics for
mixed pods name only the unpinned workload.

Five deliberately broken variants were tested using temporary Go source
overlays, without changing the submitted production source:

Injected regression New tests that detect it
Remove passing companions All six reported namespace cases fail
Treat every namespace as a flake Unlisted-namespace, disallowed-SCC, and mixed-namespace cases fail
Give the companion a different name All six reported namespace cases fail
Stop processing pods after a pinned pod Both mixed-pod cases fail
Accept a validated SCC without requiring pinning Both validated-but-unpinned cases fail

Each variant exited 1 with the expected test failures, not a compilation error.
These are injected regressions demonstrating test sensitivity, not a claim that
the original CI build contained those changes.

The final focused tests passed 10 repetitions with race detection and shuffled
order. All 21 scenarios also passed again without overlays after the mutation
checks. Repository unit tests, full make verify, the openshift-tests binary
build, formatting, and whitespace checks passed. Local CodeRabbit
completed with zero findings on the final diff. All seven deep-review specialists
completed with no unresolved findings; two initial QA suggestions were
incorporated and re-reviewed.

Validation commands

Run from the repository root with Go 1.26.4 and vendored dependencies:

export GOWORK=off GOFLAGS=-mod=vendor
go test -race ./pkg/monitortests/authentication/requiredsccmonitortests \
  -run '^TestRequiredSCC' -count=10 -shuffle=on -timeout=10m
go test ./pkg/test/ginkgo -run '^Test_countRealFailures$' -count=1 -v
make test-unit GO_TEST_PACKAGES=./pkg/... GO_TEST_FLAGS='-p=4 -count=1 -timeout=15m'
make verify
make openshift-tests
git diff --check

The aggregation test passed all seven existing cases, including skips not
counting as passes. The unit-suite sandbox initially prevented local HTTP test
servers from binding; the unrestricted rerun passed. The build output was
directed to a temporary directory. Verification's APM step also required access
to its tool cache outside the sandbox; its rerun passed with existing warnings
about vendored instruction files, which were left untouched.
No full live conformance or HyperShift
operator-upgrade suite is claimed.

Fresh live evidence gathered before this test-only change

On 2026-09-18, the actual Origin analyzer and monitor lifecycle were run
read-only from source c4a623c952596ab73581f217f170b94f8bce6d4a against two
clusters. The relevant checker, monitor aggregation, and suite failure-counting
files are unchanged between that source and this PR's upstream base.

Environment Evaluated namespaces Non-blocking flaky namespaces Blocking SCC failures Monitor result
AWS IPI, OCP 4.22.6 72 19 0 Succeeded
AWS HCP, 4.22.15 target during rollout 60 6 0 Succeeded

In the HCP observation, all four reported kube-system workload families had
running instances: kas-connection-checker, konnectivity-agent,
global-pull-secret-syncer, and kube-apiserver-proxy. Nine observed pods lacked
the required-SCC annotation; the namespace emitted one failed and one passing
JUnit with the same name. DNS, Multus, network-operator, and OVN also emitted
failure/pass pairs. Observed ingress pods were already annotated; the reported
unannotated ingress case is covered by the deterministic regression test.

The HCP run recorded 67 GET requests and zero rejected write attempts in the
guarded analyzer/inventory transport. Independent JUnit inspection found zero
failing-only names across 67 unique test names, including lifecycle checks.
The IPI run did not cover HyperShift kube-system workloads because that
namespace was empty there.

Limits and scope

  • Historical Prow artifacts are unavailable. These fresh observations do not
    reproduce the originally reported 5.0/5.1 build or prove its job outcome.
  • This is a source-built diagnostic, not a complete conformance run or the
    original CI test image. The HCP snapshot was taken during rollout.
  • Guest ClusterVersion was still Partial at the final health check because
    image-registry reported a failed image-pruner Job. That independent health
    failure was neither hidden nor fixed by SCC results.
  • Optional report metadata collection warned about the HCP's absent
    MachineConfigPool API; SCC collection and lifecycle checks succeeded.
  • No HyperShift product change is proposed. SCC pinning and any resulting
    worker rollout need a separate change and upgrade validation.

This PR supplies reproducible regression coverage and fresh evidence for the
existing-allowlist resolution. Acceptance of that evidence for the affected
release remains a maintainer decision.

Lock in the existing non-blocking classification for the six namespaces
reported in OCPBUGS-115110. Retain pod diagnostics and verify that missing
annotations outside the allowlists, disallowed SCCs, and collection errors
still fail through the real monitor lifecycle.

This is regression coverage, not a change to SCC policy or pod manifests.

Refs: OCPBUGS-115110
Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
@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: automatic mode

@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 18, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@vsolanki12: This pull request references Jira Issue OCPBUGS-115110, which is invalid:

  • expected the bug to target the "5.1.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

Add regression coverage for the existing required-SCC monitor behavior described
in OCPBUGS-115110.

This implements the issue's verification option: confirm that the existing
namespacesWithPendingSCCPinning and systemNamespaces handling makes the
reported namespaces non-blocking. It does not change SCC policy, add allowlist
entries, annotate workloads, or change the monitor's production behavior.

The new tests check:

  • The six reported namespaces emit a failed JUnit with pod diagnostics and a
    passing companion with exactly the same name, and the real monitor returns
    Succeeded.
  • Missing annotations in unlisted platform namespaces and a disallowed
    nonstandard SCC remain blocking.
  • Pinned pods, empty namespaces, and out-of-scope namespaces retain their
    existing behavior; skips are not accepted as passing companions.
  • A kube-system flake cannot hide a failure in another namespace.
  • Namespace/pod list errors propagate and make the monitor fail.

The tests use fake Kubernetes objects and execute the production checker and
monitor lifecycle. Only client construction at startup is replaced to retain
the fake client. They require no cluster access and do not test SCC admission.

Before and after

At base 147d29558f3fd344567894152cdf1ee6b9044063, the checker package reported
[no test files]. Its runtime behavior already made the reported namespaces
non-blocking. After this change, 21 focused scenarios pass: 16 classification
cases, two mixed-pod cases, one mixed-namespace aggregation case, and two
API-error cases. Validated-but-unpinned pods remain violations; diagnostics for
mixed pods name only the unpinned workload.

Five deliberately broken variants were tested using temporary Go source
overlays, without changing the submitted production source:

Injected regression New tests that detect it
Remove passing companions All six reported namespace cases fail
Treat every namespace as a flake Unlisted-namespace, disallowed-SCC, and mixed-namespace cases fail
Give the companion a different name All six reported namespace cases fail
Stop processing pods after a pinned pod Both mixed-pod cases fail
Accept a validated SCC without requiring pinning Both validated-but-unpinned cases fail

Each variant exited 1 with the expected test failures, not a compilation error.
These are injected regressions demonstrating test sensitivity, not a claim that
the original CI build contained those changes.

The final focused tests passed 10 repetitions with race detection and shuffled
order. All 21 scenarios also passed again without overlays after the mutation
checks. Repository unit tests, full make verify, the openshift-tests binary
build, formatting, and whitespace checks passed. Local CodeRabbit
completed with zero findings on the final diff. All seven deep-review specialists
completed with no unresolved findings; two initial QA suggestions were
incorporated and re-reviewed.

Validation commands

Run from the repository root with Go 1.26.4 and vendored dependencies:

export GOWORK=off GOFLAGS=-mod=vendor
go test -race ./pkg/monitortests/authentication/requiredsccmonitortests \
 -run '^TestRequiredSCC' -count=10 -shuffle=on -timeout=10m
go test ./pkg/test/ginkgo -run '^Test_countRealFailures$' -count=1 -v
make test-unit GO_TEST_PACKAGES=./pkg/... GO_TEST_FLAGS='-p=4 -count=1 -timeout=15m'
make verify
make openshift-tests
git diff --check

The aggregation test passed all seven existing cases, including skips not
counting as passes. The unit-suite sandbox initially prevented local HTTP test
servers from binding; the unrestricted rerun passed. The build output was
directed to a temporary directory. Verification's APM step also required access
to its tool cache outside the sandbox; its rerun passed with existing warnings
about vendored instruction files, which were left untouched.
No full live conformance or HyperShift
operator-upgrade suite is claimed.

Fresh live evidence gathered before this test-only change

On 2026-09-18, the actual Origin analyzer and monitor lifecycle were run
read-only from source c4a623c952596ab73581f217f170b94f8bce6d4a against two
clusters. The relevant checker, monitor aggregation, and suite failure-counting
files are unchanged between that source and this PR's upstream base.

Environment Evaluated namespaces Non-blocking flaky namespaces Blocking SCC failures Monitor result
AWS IPI, OCP 4.22.6 72 19 0 Succeeded
AWS HCP, 4.22.15 target during rollout 60 6 0 Succeeded

In the HCP observation, all four reported kube-system workload families had
running instances: kas-connection-checker, konnectivity-agent,
global-pull-secret-syncer, and kube-apiserver-proxy. Nine observed pods lacked
the required-SCC annotation; the namespace emitted one failed and one passing
JUnit with the same name. DNS, Multus, network-operator, and OVN also emitted
failure/pass pairs. Observed ingress pods were already annotated; the reported
unannotated ingress case is covered by the deterministic regression test.

The HCP run recorded 67 GET requests and zero rejected write attempts in the
guarded analyzer/inventory transport. Independent JUnit inspection found zero
failing-only names across 67 unique test names, including lifecycle checks.
The IPI run did not cover HyperShift kube-system workloads because that
namespace was empty there.

Limits and scope

  • Historical Prow artifacts are unavailable. These fresh observations do not
    reproduce the originally reported 5.0/5.1 build or prove its job outcome.
  • This is a source-built diagnostic, not a complete conformance run or the
    original CI test image. The HCP snapshot was taken during rollout.
  • Guest ClusterVersion was still Partial at the final health check because
    image-registry reported a failed image-pruner Job. That independent health
    failure was neither hidden nor fixed by SCC results.
  • Optional report metadata collection warned about the HCP's absent
    MachineConfigPool API; SCC collection and lifecycle checks succeeded.
  • No HyperShift product change is proposed. SCC pinning and any resulting
    worker rollout need a separate change and upgrade validation.

This PR supplies reproducible regression coverage and fresh evidence for the
existing-allowlist resolution. Acceptance of that evidence for the affected
release remains a maintainer decision.

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.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

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: Advanced

Run ID: 74366521-1cb3-4607-9177-9c76d41ceffa

📥 Commits

Reviewing files that changed from the base of the PR and between 147d295 and 94fd433.

📒 Files selected for processing (1)
  • pkg/monitortests/authentication/requiredsccmonitortests/monitortest_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


Walkthrough

Adds a new test file for the required-SCC annotation checker. The tests use fake API clients and monitor lifecycles to verify annotation results, namespace aggregation, failure and flake handling, and collection errors.

Changes

Required SCC monitor tests

Layer / File(s) Summary
Monitor test harness
pkg/monitortests/authentication/requiredsccmonitortests/monitortest_test.go
Adds imports and helpers that run the checker with a preserved fake client through monitor start and stop operations.
Annotation result cases
pkg/monitortests/authentication/requiredsccmonitortests/monitortest_test.go
Tests pinned, unpinned, excluded, invalid, mixed, and flaky workload cases. The tests verify diagnostics, JUnit counts, and monitor states.
Collection error cases
pkg/monitortests/authentication/requiredsccmonitortests/monitortest_test.go
Tests namespace and pod listing errors. The tests verify error propagation, empty results, and failed monitor states.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to 94fd4

The added regression tests exercise the intended monitor lifecycle without bypassing SCC collection or result aggregation, so no merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning The added tests have several bare error assertions without operation context. In monitortest_test.go, t.Fatal(err) is used after CollectData (lines 132 and 194), monitor registration (276), moni… Add contextual messages to every bare error assertion. For example: t.Fatalf("CollectData failed for namespace %q: %v", tt.namespace, err), t.Fatalf("failed to register required-SCC monitor: %v", err), `t.Fatalf("failed to start monitor…
✅ Passed checks (13 passed)
Check name Status Explanation
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 The pull request adds one Go test file and does not add Ginkgo tests. All test titles are stable: the main table uses fixed literal name values, mixed-pod subtests use fixed namespace literals, and …
Microshift Test Compatibility ✅ Passed PASS: The pull request adds one standard Go unit-test file with testing.T tests (TestRequiredSCC...), not Ginkgo e2e tests. It has no Ginkgo DSL, MicroShift markers, live cluster setup, unavailabl…
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS. The pull request adds one Go unit-test file with standard testing functions (TestRequiredSCC...), not Ginkgo e2e tests. The tests use fake Kubernetes clients and in-memory Namespace/Pod obje…
Topology-Aware Scheduling Compatibility ✅ Passed PASS. The reviewed range adds only pkg/monitortests/authentication/requiredsccmonitortests/monitortest_test.go. It does not add or modify deployment manifests, operator code, controllers, or schedul…
Ote Binary Stdout Contract ✅ Passed PASS: The pull request adds only monitortest_test.go. Its fmt use is fmt.Sprintf inside test functions, and all other output-related calls are testing.T diagnostics inside test bodies. The dif…
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PASS: The pull request adds only standard Go testing unit tests. The AST outline shows Test... functions and subtests, with no Ginkgo It, Describe, Context, or When tests. The changed file…
No-Weak-Crypto ✅ Passed The pull request adds only one Go test file. Its imports and code use Kubernetes fake clients, monitor APIs, errors, formatting, strings, and time. The reviewed diff contains no MD5, SHA1, DES, 3DES, …
Container-Privileges ✅ Passed PASS. The pull request changes only one Go test file. It adds fake Kubernetes Namespace and Pod objects with metadata and SCC annotations; it does not add a container specification or manifest. The di…
No-Sensitive-Data-In-Logs ✅ Passed PASS. The PR adds one test file and does not change production logging. Test failure messages may print JUnit diagnostics, but all inputs are fixed fake namespaces, pod names, SCC names, and the liter…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the regression coverage for non-blocking required-SCC monitor results and matches the main purpose of the test-only changes.
Full details: Test Structure And Quality

Explanation

The added tests have several bare error assertions without operation context. In monitortest_test.go, t.Fatal(err) is used after CollectData (lines 132 and 194), monitor registration (276), monitor start (282), and monitor stop (286). These failures do not identify which operation failed, contrary to the required meaningful assertion messages. The tests use fake clients and t.TempDir, create no cluster resources, and contain no indefinite wait calls, so setup/cleanup and timeout concerns do not apply.

Resolution

Add contextual messages to every bare error assertion. For example: t.Fatalf("CollectData failed for namespace %q: %v", tt.namespace, err), t.Fatalf("failed to register required-SCC monitor: %v", err), t.Fatalf("failed to start monitor: %v", err), and t.Fatalf("failed to stop monitor: %v", err). Apply equivalent context to both CollectData call sites.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@openshift-ci
openshift-ci Bot requested review from deads2k and sjenning September 18, 2026 12:30
@openshift-ci

openshift-ci Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vsolanki12
Once this PR has been reviewed and has the lgtm label, please assign smg247 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

@openshift-ci openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Sep 18, 2026
@vsolanki12

Copy link
Copy Markdown
Author

/jira refresh

@openshift-ci-robot openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 18, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@vsolanki12: This pull request references Jira Issue OCPBUGS-115110, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.1.0) matches configured target version for branch (5.1.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

/jira refresh

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.

@vsolanki12

Copy link
Copy Markdown
Author

/pipeline required

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6

@vsolanki12

Copy link
Copy Markdown
Author

/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade

@openshift-ci

openshift-ci Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

@vsolanki12: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-ovn 94fd433 link true /test e2e-gcp-ovn
ci/prow/e2e-gcp-ovn-upgrade 94fd433 link true /test e2e-gcp-ovn-upgrade

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-trt

openshift-trt Bot commented Sep 21, 2026

Copy link
Copy Markdown

Job Failure Risk Analysis for sha: 94fd433

Job Name Failure Risk
pull-ci-openshift-origin-main-e2e-gcp-ovn IncompleteTests
Tests for this run (101) are below the historical average (3056): IncompleteTests (not enough tests ran to make a reasonable risk analysis; this could be due to infra, installation, or upgrade problems)

@vsolanki12
vsolanki12 marked this pull request as draft September 22, 2026 05:23
@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 Sep 22, 2026
@vsolanki12
vsolanki12 marked this pull request as ready for review September 22, 2026 11:12
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2026
@openshift-ci
openshift-ci Bot requested a review from p0lyn0mial September 22, 2026 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants