far-operator: add negative validation tests (RHWA-964) by gamado · Pull Request #85 · medik8s/system-tests

gamado · 2026-08-12T15:03:48Z

Summary

Add 4 non-destructive misconfiguration tests migrated from Python (ocp-edge-auto test_far_cli.py TestMisconfigurations class)
OCP-65954 -- CR name not matching any node, controller logs error
OCP-66090 -- unsupported --action value, webhook rejects
OCP-71219 -- invalid fence agent in FAR CR, webhook/CRD rejects
OCP-71220 -- invalid fence agent in FARTemplate, webhook/CRD rejects
OCP-61334 (IPMI server misconfiguration) deferred pending platform verification on baremetal

Jira

RHWA-964

Test plan

All 4 tests pass on AWS cluster-bot (verified twice before review fixes)
go build, go vet, gofmt clean
Prow CI presubmit passes
All 4 Polarion IDs appear as PASSED in CI results (not SKIPPED)

Summary by CodeRabbit

Tests
- Added coverage for invalid FAR and FARTemplate configurations, including nonexistent nodes, unsupported actions, and invalid fence agents.
- Added validation for webhook rejection and controller error reporting.
- Expanded documentation with non-destructive negative-validation scenarios and updated test numbering.

gamado · 2026-08-12T15:05:52Z

/test 4.22-konflux-e2e-far-aws

coderabbitai · 2026-08-12T15:06:04Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e258f1ae-60ee-4d25-be42-b32d5d411a8e

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b6be627d-aa16-471b-bce4-953f09247c79

📥 Commits

Reviewing files that changed from the base of the PR and between 2d10e5f and 215e414.

📒 Files selected for processing (2)

tests/far-operator/README.md
tests/far-operator/tests/far_negative_validation.go

🚧 Files skipped from review as they are similar to previous changes (2)

tests/far-operator/README.md
tests/far-operator/tests/far_negative_validation.go

📝 Walkthrough

Walkthrough

Added FAR and FARTemplate negative-validation tests for missing nodes, unsupported actions, unsupported fence agents, and invalid agent prefixes. Added shared constants, resource builders, cleanup helpers, controller log polling, and test documentation updates.

Changes

FAR negative validation

Layer / File(s)	Summary
Shared test support `tests/far-operator/internal/farparams/const.go`, `tests/far-operator/tests/far_negative_validation.go`	Added expected messages, polling values, misconfiguration parameters, test setup, shared IPMI parameters, cleanup helpers, and FAR/FARTemplate resource builders.
Negative validation execution `tests/far-operator/tests/far_negative_validation.go`, `tests/far-operator/README.md`	Added controller log and webhook validation tests. Documented the new scenarios and renumbered the existing destructive tests.

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

Possibly related PRs

medik8s/system-tests#19: Updates the related FAR test-suite infrastructure and shared constants.
medik8s/system-tests#20: Extends the same farparams/const.go test infrastructure.
medik8s/system-tests#22: Extends the same FAR test infrastructure and destructive-test scaffold.

Suggested labels: ok-to-test

Suggested reviewers: weshayutin, rbartal, razo7

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the FAR operator negative validation tests added by this pull request.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ 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.}

coderabbitai

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/far-operator/README.md`:
- Around line 129-137: Align the “Pass criteria” in the “Verify Node-Not-Found
Error for Non-Existent CR Name” section with the assertions in the corresponding
negative-validation test: either remove the claim that fencing is not attempted
or add a reliable observable assertion proving fencing was not attempted. Keep
the existing CR-creation and “Could not find CR's target node” log checks
unchanged.

In `@tests/far-operator/tests/far_negative_validation.go`:
- Around line 89-120: The four affected It specifications in
tests/far-operator/tests/far_negative_validation.go (lines 89-120, 122-138,
142-163, and 165-186) must each invoke ReportIfFailed() so failures collect the
configured namespaces and CRDs. Add the call within every specification,
including OCP-65954, OCP-66090, OCP-71219, and OCP-71220.
- Around line 115-119: Update the test around APIClient.Create to capture a log
boundary immediately before creating the FAR resource, then pass that timestamp
through the findMessageInFARControllerLogs log query using
PodLogOptions.SinceTime. Ensure the Eventually assertion only matches
NodeNotFoundMsg entries emitted after the current test run began.

🪄 Autofix

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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3275ec56-1627-43bc-bbb2-4413aa4f63ed

📥 Commits

Reviewing files that changed from the base of the PR and between 3e38888 and 2d10e5f.

📒 Files selected for processing (3)

tests/far-operator/README.md
tests/far-operator/internal/farparams/const.go
tests/far-operator/tests/far_negative_validation.go

coderabbitai · 2026-08-12T15:11:18Z

+			It("should log node-not-found error for CR with non-existent node name",
+				reportxml.ID("65954"),
+				Label(labels.ComponentRemediation),
+				func() {
+					By("Building FAR CR with name that does not match any cluster node")
+
+					farCR := buildMisconfigFAR(farparams.MisconfigTestCRName,
+						farparams.FenceAgentIPMI, nil, nil)
+
+					By("Creating FAR CR")
+
+					Expect(APIClient.Create(ctx, farCR)).To(Succeed(),
+						"Failed to create FAR CR with non-existent node name")
+
+					By("Verifying FAR CR exists")
+
+					created := &unstructured.Unstructured{}
+					created.SetGroupVersionKind(farGVK)
+					Expect(APIClient.Get(ctx, client.ObjectKey{
+						Name:      farparams.MisconfigTestCRName,
+						Namespace: medik8sparams.OperatorNs,
+					}, created)).To(Succeed(),
+						"FAR CR %s should exist after creation", farparams.MisconfigTestCRName)
+
+					By("Waiting for node-not-found message in FAR controller logs")
+
+					Eventually(func() error {
+						return findMessageInFARControllerLogs(
+							farparams.NodeNotFoundMsg, farparams.LogSearchWindow)
+					}, farparams.LogSearchTimeout, farparams.DefaultPollInterval).Should(Succeed(),
+						"%q should appear in FAR controller logs", farparams.NodeNotFoundMsg)
+				})


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Call ReportIfFailed() in every specification.

None of these It specifications invokes the shared failure reporter. Add the required call to each specification so a failure collects the configured namespaces and CRDs.

tests/far-operator/tests/far_negative_validation.go#L89-L120: add ReportIfFailed() to OCP-65954.

tests/far-operator/tests/far_negative_validation.go#L122-L138: add ReportIfFailed() to OCP-66090.

tests/far-operator/tests/far_negative_validation.go#L142-L163: add ReportIfFailed() to OCP-71219.

tests/far-operator/tests/far_negative_validation.go#L165-L186: add ReportIfFailed() to OCP-71220.

As per coding guidelines, “Every It and DescribeTable specification must call ReportIfFailed() to dump configured namespaces and CRDs when tests fail.”

📍 Affects 1 file

tests/far-operator/tests/far_negative_validation.go#L89-L120 (this comment)

tests/far-operator/tests/far_negative_validation.go#L122-L138

tests/far-operator/tests/far_negative_validation.go#L142-L163

tests/far-operator/tests/far_negative_validation.go#L165-L186

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/far-operator/tests/far_negative_validation.go` around lines 89 - 120, The four affected It specifications in tests/far-operator/tests/far_negative_validation.go (lines 89-120, 122-138, 142-163, and 165-186) must each invoke ReportIfFailed() so failures collect the configured namespaces and CRDs. Add the call within every specification, including OCP-65954, OCP-66090, OCP-71219, and OCP-71220.

Source: Coding guidelines

- Fix README OCP-65954: remove unverified claim about fencing not being attempted (test only checks controller log message, matching the Polarion test plan) - Fix log search stale entry risk: capture time.Now() before CR creation, use time.Since(baseline) as log window so only logs from after the CR was created are searched Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gamado · 2026-08-12T18:32:26Z

/test 4.22-konflux-e2e-far-aws

- Fix NodeNotFoundMsg to match current FAR controller source, keep legacy message for backward compat with older Konflux builds - Move OCP-66090 (webhook test) from "controller log messages" to "webhook rejection" context - Split OCP-71219 and OCP-71220 into separate It blocks so each sub-case runs independently (ContinueOnFailure is inter-It only) - Replace AfterEach with DeferCleanup registered in BeforeAll - Rename all FART references to FARTemplate across test and const files - Use context.Background() instead of context.TODO() in cleanup funcs - Remove Python code references from comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gamado · 2026-08-16T20:28:13Z

/test 4.22-konflux-e2e-far-aws

razo7

Some small changes to address

- Remove unused LogSearchWindow constant (test uses time.Since(logBaseline)) - Fix NodeNotFoundMsgLegacy version comment: pre-v0.8.1, not pre-v0.9 - Add TODO to remove legacy constant once Konflux builds catch up Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gamado · 2026-08-17T08:37:19Z

/test 4.22-konflux-e2e-far-aws

- Fix gofmt alignment in far_destructive.go var block after rename - Add By() annotations in DeferCleanup for Ginkgo report visibility - Hoist Label(ComponentWebhook) from 5 It blocks to Context level - Replace workerNode selection with WebhookTestCRName placeholder (webhook validates agent/action, not node -- fixes SNO compat) - Reuse buildFARUnstructured/buildFARTemplateUnstructured from far_destructive.go, remove duplicate buildMisconfigFAR/FARTemplate - Migrate far_destructive.go from package-local deleteRemediationCR to shared helpers.DeleteRemediationCR (single deletion helper) - Remove //nolint:unused from buildFARTemplateUnstructured (now used) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gamado · 2026-08-17T20:14:35Z

/test 4.22-konflux-e2e-far-aws

gamado · 2026-08-17T20:28:30Z

/test 4.22-konflux-e2e-far-aws

gamado · 2026-08-18T17:56:50Z

@razo7 , can you please re-approve ?

razo7

I left few NITs but I am also ok with approving

Add 4 non-destructive misconfiguration tests migrated from Python (ocp-edge-auto test_far_cli.py TestMisconfigurations class): - OCP-65954: CR name not matching any node -- controller logs error - OCP-66090: unsupported --action value -- webhook rejects - OCP-71219: invalid fence agent in FAR CR -- webhook/CRD rejects - OCP-71220: invalid fence agent in FARTemplate -- webhook/CRD rejects OCP-61334 (IPMI server misconfiguration) deferred pending platform verification on baremetal. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Fix README OCP-65954: remove unverified claim about fencing not being attempted (test only checks controller log message, matching the Polarion test plan) - Fix log search stale entry risk: capture time.Now() before CR creation, use time.Since(baseline) as log window so only logs from after the CR was created are searched Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Fix NodeNotFoundMsg to match current FAR controller source, keep legacy message for backward compat with older Konflux builds - Move OCP-66090 (webhook test) from "controller log messages" to "webhook rejection" context - Split OCP-71219 and OCP-71220 into separate It blocks so each sub-case runs independently (ContinueOnFailure is inter-It only) - Replace AfterEach with DeferCleanup registered in BeforeAll - Rename all FART references to FARTemplate across test and const files - Use context.Background() instead of context.TODO() in cleanup funcs - Remove Python code references from comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Remove unused LogSearchWindow constant (test uses time.Since(logBaseline)) - Fix NodeNotFoundMsgLegacy version comment: pre-v0.8.1, not pre-v0.9 - Add TODO to remove legacy constant once Konflux builds catch up Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Fix gofmt alignment in far_destructive.go var block after rename - Add By() annotations in DeferCleanup for Ginkgo report visibility - Hoist Label(ComponentWebhook) from 5 It blocks to Context level - Replace workerNode selection with WebhookTestCRName placeholder (webhook validates agent/action, not node -- fixes SNO compat) - Reuse buildFARUnstructured/buildFARTemplateUnstructured from far_destructive.go, remove duplicate buildMisconfigFAR/FARTemplate - Migrate far_destructive.go from package-local deleteRemediationCR to shared helpers.DeleteRemediationCR (single deletion helper) - Remove //nolint:unused from buildFARTemplateUnstructured (now used) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…tring match Root cause: clock skew between the test runner and pod nodes caused SinceSeconds-based log fetches to return empty results. GetLog(5s) returned 0 lines even though the message was logged 5 seconds ago. Fix: use GetFullLog (fetches all logs since pod start) instead of GetLog(sinceSeconds). Check all known node-not-found message variants in a single pass over each pod's log output to avoid pod-restart races. The FAR controller message varies across versions: - v0.8.0-v0.8.1 (release-0.8): "Could not find CR's target node" - main (unreleased): "couldn't find node matching remediation" Both are checked in NodeNotFoundMsgs slice. Also: builds FAR CR without sharedSecretName (the validating webhook rejects CRs referencing non-existent secrets, which blocks OCP-65954 from creating the CR needed for the log test). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

OCP-71219 and OCP-71220 are each a single Polarion test case with two verifications (per the test plan), not two independent tests. The prior split into two It blocks left both blocks tagged with the same reportxml.ID, which makes Polarion overwrite one result with the other. Merge each pair back into one It block per Polarion ID, matching the merged NHC negative-test convention. A new verifyAdmissionRejections helper collects failures so both sub-cases run even if the first fails (addressing the original fail-fast concern) and emits a By() step per sub-case. Extract "placeholder-node" and "6233" into named constants. Addresses razo7 review comments on PR medik8s#85. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

gamado · 2026-08-23T09:06:30Z

/test 4.22-konflux-e2e-far-aws

weshayutin

/LGTM

openshift-ci · 2026-08-23T20:50:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gamado, razo7, weshayutin

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [razo7,weshayutin]

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

openshift-ci · 2026-08-26T10:19:21Z

PR needs rebase.

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.

openshift-ci Bot requested review from rbartal and weshayutin August 12, 2026 15:03

coderabbitai Bot reviewed Aug 12, 2026

View reviewed changes

razo7 mentioned this pull request Aug 16, 2026

Add FAR observability tests, fix flaky 2-worker leader-fencing test, and dump remediation diagnostics on failure #81

Open

razo7 requested changes Aug 16, 2026

View reviewed changes

openshift-ci Bot assigned razo7 Aug 16, 2026

gamado added highlight-in-reviewer-pr-list https://docs.google.com/document/d/1ZB5U3Aqf2G8QPXKdwXzTQZHqCwrZHfw4fhez8CmppjA/edit?tab=t.0 and removed highlight-in-reviewer-pr-list https://docs.google.com/document/d/1ZB5U3Aqf2G8QPXKdwXzTQZHqCwrZHfw4fhez8CmppjA/edit?tab=t.0 labels Aug 16, 2026

gamado requested a review from razo7 August 16, 2026 20:32

razo7 reviewed Aug 17, 2026

View reviewed changes

Comment thread tests/far-operator/internal/farparams/const.go

Comment thread tests/far-operator/internal/farparams/const.go Outdated

razo7 previously approved these changes Aug 17, 2026

View reviewed changes

openshift-ci Bot added lgtm approved labels Aug 17, 2026

ugreener reviewed Aug 17, 2026

View reviewed changes

gamado dismissed razo7’s stale review via 660f96b August 17, 2026 12:58

openshift-ci Bot removed lgtm approved labels Aug 17, 2026

gamado force-pushed the feat/far-negative-validation branch from 17e8672 to 9165b86 Compare August 17, 2026 20:28

gamado requested a review from ugreener August 17, 2026 20:30

gamado requested a review from razo7 August 17, 2026 20:30

razo7 previously approved these changes Aug 21, 2026

View reviewed changes

Comment thread tests/far-operator/tests/far_negative_validation.go Outdated

Comment thread tests/far-operator/tests/far_negative_validation.go

Comment thread tests/far-operator/tests/far_negative_validation.go Outdated

openshift-ci Bot added lgtm approved labels Aug 21, 2026

gamado and others added 7 commits August 23, 2026 12:02

gamado dismissed razo7’s stale review via 5d30d39 August 23, 2026 09:06

gamado force-pushed the feat/far-negative-validation branch from 9165b86 to 5d30d39 Compare August 23, 2026 09:06

openshift-ci Bot removed the lgtm label Aug 23, 2026

openshift-ci Bot removed the approved label Aug 23, 2026

gamado requested a review from razo7 August 23, 2026 09:08

razo7 approved these changes Aug 23, 2026

View reviewed changes

openshift-ci Bot added lgtm approved labels Aug 23, 2026

weshayutin approved these changes Aug 23, 2026

View reviewed changes

openshift-ci Bot assigned weshayutin Aug 23, 2026

openshift-ci Bot added the needs-rebase label Aug 26, 2026

Uh oh!

Conversation

gamado commented Aug 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Jira

Test plan

Summary by CodeRabbit

Uh oh!

gamado commented Aug 12, 2026

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gamado commented Aug 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gamado commented Aug 16, 2026

Uh oh!

razo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gamado commented Aug 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gamado commented Aug 17, 2026

Uh oh!

gamado commented Aug 17, 2026

Uh oh!

gamado commented Aug 18, 2026

Uh oh!

razo7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gamado commented Aug 23, 2026

Uh oh!

weshayutin left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 23, 2026

Uh oh!

openshift-ci Bot commented Aug 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

gamado commented Aug 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading