Optionally read cluster resources with dryrun CLI #388

jan-law · 2025-09-15T22:16:17Z

Instead of providing input yaml files to the dryrun command args, set the --from-cluster flag or DRYRUN_FROM_CLUSTER to read cluster resources via default kubeconfig.

Input file args or API mapping file representing cluster resources will be ignored.

As before, the policies are patched with remediationAction: Inform, preventing modifications to the cluster.

ref: https://issues.redhat.com/browse/ACM-22932

test/dryrun/context_vars/object_cluster_scoped/output_from_cluster.txt

test/dryrun/context_vars/object_namespaced/policy.yaml

test/dryrun/context_vars/objectname_nsselector/policy.yaml

jan-law · 2025-09-26T18:32:35Z

test/e2e/case46_dryrun_cli_test.go

+		}
+	})
+
+	testScenarios := []struct {


I wanted to document which test folders I reused from test/dryrun/. Let me know if you have suggestions for a cleaner pattern to follow, especially when the test folders are nested e.g. no_name and no_name/with_object_selector

I think this pattern looks good. The alternative would be to leverage //go:embed like the unit tests do, but I'm not sure there's an advantage either way.

My only thought is a musing whether the https://github.com/open-cluster-management-io/config-policy-controller/blob/main/test/dryrun/utils.go file could be leveraged to have shared code, but I'm not sure that's necessary since you have something working here.

dhaiducek

I think this looks really good, @jan-law! Your comments both in the code and in the PR are really helpful! I have some comments/questions mostly around the test cases.

pkg/dryrun/cmd.go

test/e2e/case46_dryrun_cli_test.go

dhaiducek · 2025-10-02T02:27:40Z

test/e2e/case46_dryrun_cli_test.go

+	"kubernetes.io/metadata.name:",
+}
+
+var _ = Describe("Testing dryrun CLI", Ordered, func() {


Is the Ordered decorator necessary--are there collisions when run in parallel?

Added a question below about handling namespace deletions. Will continue on the other conversation thread #388 (comment)

test/e2e/case46_dryrun_cli_test.go

dhaiducek · 2025-10-02T02:55:55Z

test/e2e/case46_dryrun_cli_test.go

+		}
+	})
+
+	testScenarios := []struct {


I think this pattern looks good. The alternative would be to leverage //go:embed like the unit tests do, but I'm not sure there's an advantage either way.

My only thought is a musing whether the https://github.com/open-cluster-management-io/config-policy-controller/blob/main/test/dryrun/utils.go file could be leveraged to have shared code, but I'm not sure that's necessary since you have something working here.

test/e2e/case46_dryrun_cli_test.go

pkg/dryrun/cmd.go

ref: https://issues.redhat.com/browse/ACM-22932 No logic changed here. Refactor before adding a new dry run cli flag Signed-off-by: Janelle Law <[email protected]>

ref: https://issues.redhat.com/browse/ACM-22932 Instead of providing input yaml files to the dryrun command args, set the --from-cluster flag to read cluster resources via default kubeconfig. As before, the policies are patched with remediationAction: Inform, preventing modifications to the cluster. Signed-off-by: Janelle Law <[email protected]>

ref: https://issues.redhat.com/browse/ACM-22932 Tests the dryrun CLI can read resources from a cluster with --from-cluster flag. Reuses the dryrun unit test cases, applies the resources to the test kind cluster, then runs the e2e tests. Sometimes the policy diff from the cluster returns extra context lines surrounding the relevant +/- changes. When this diff does not match the expected output.txt from the unit tests, the e2e test will compare the output with the output_from_cluster.txt file. Assisted by Cursor. Signed-off-by: Janelle Law <[email protected]>

JustinKuli

I think this is looking good overall! I didn't take a super close look at the test setup (seems like Dale's been through it more carefully) but it looks right to me. Just letting Dale have final approval 😄

JustinKuli · 2025-10-03T18:54:34Z

pkg/dryrun/dryrun.go

+
+	runtimeClient, err := client.New(kubeConfig, client.Options{
+		Scheme: scheme.Scheme,
+		DryRun: &readOnlyMode,


I didn't know about this option, awesome!

JustinKuli · 2025-10-03T19:02:04Z

test/e2e/case46_dryrun_cli_test.go

+		}
+
+		By("Creating namespace from " + nsFilePath)
+		cmd := exec.Command("kubectl", "apply", "-f", nsFilePath, "--kubeconfig="+kubeconfig)


I think utils.Kubectl handles the kubeconfig and some of the output already, but I could be missing some other thing that needs to be specially handled in these tests?

(This isn't necessary to change if everything else in the PR is ready to go - it's just useful to know about)

Some dryrun tests reuse the same namespace names. When I deleted and recreated namespaces in each DescribeTable entry, the previous namespace is still terminating when the next test starts, causing failures due to leftover pods. e.g. kind test failure missing_name

I originally used Ordered with AfterAll to delay all namespaces deletion until the end, but switched to Serial (the Ginkgo docs say AfterAll only works with Ordered). I wrapped namespace creation in Eventually with the g.Expect(err).ToNot(HaveOccurred() from this kubectl apply cmd, hoping it would retry until the previous delete completed, but namespaces never fully terminate before the next test starts.

Any ideas for handling namespaces in a Serial + DescribeTable setup without AfterAll or suite-level hooks?

the previous namespace is still terminating when the next test starts

My only thought for this is to add --wait=true to the delete command, but it sounds like you had already tried that?

These tests aren't modifying the objects, so I think having the namespace/objects created in the BeforeSuite() and cleaned up after wouldn't be bad.

That said, I wouldn't want to hold up the PR too long on this topic (sorry if I already did! 😬), so I think moving back to using Ordered for now is also fine for the sake of moving it forward.

Oh, I tried adding --wait only on the namespace deletion and not the resource deletion! Adding the --wait to the resource deletion too

Local test runs are passing. Same with the last two CI runs

Tests dryrun scenarios in any order. Refactored namespace creation/deletion before and after each test Entry in DescribeTable. Eventually() will retry the dryrun command to wait for any duplicate resources pending deletion from previous test cases. ref: https://issues.redhat.com/browse/ACM-22932 Assisted by Cursor. Signed-off-by: Janelle Law <[email protected]>

JustinKuli

/hold

LGTM! Holding in case Dale had any requested changes I missed, but @jan-law, you should be able to un-hold it if you're happy with the way it looks!

openshift-ci · 2025-10-07T13:07:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan-law, JustinKuli

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JustinKuli,jan-law]

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

jan-law · 2025-10-08T14:18:35Z

/unhold

openshift-ci bot added dco-signoff: yes do-not-merge/work-in-progress approved labels Sep 15, 2025

jan-law force-pushed the dryrun-read branch 14 times, most recently from 41e7dea to 1d9a176 Compare September 19, 2025 21:28

jan-law changed the title ~~[DRAFT] Read cluster resources during dryrun CLI~~ Read cluster resources during dryrun CLI Sep 19, 2025

jan-law changed the title ~~Read cluster resources during dryrun CLI~~ Optionally read cluster resources with dryrun CLI Sep 19, 2025

jan-law marked this pull request as ready for review September 19, 2025 22:05

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 19, 2025

openshift-ci bot requested review from JustinKuli and gparvin September 19, 2025 22:05

jan-law requested a review from dhaiducek September 19, 2025 22:07

jan-law commented Sep 26, 2025

View reviewed changes

dhaiducek reviewed Oct 2, 2025

View reviewed changes

Move input resource creation to helper func

7797080

ref: https://issues.redhat.com/browse/ACM-22932 No logic changed here. Refactor before adding a new dry run cli flag Signed-off-by: Janelle Law <[email protected]>

jan-law force-pushed the dryrun-read branch from 1d9a176 to 04ea848 Compare October 3, 2025 18:45

jan-law added 2 commits October 3, 2025 11:50

jan-law force-pushed the dryrun-read branch from 04ea848 to 825be10 Compare October 3, 2025 18:50

JustinKuli reviewed Oct 3, 2025

View reviewed changes

jan-law force-pushed the dryrun-read branch from 825be10 to 6db1305 Compare October 6, 2025 20:37

JustinKuli approved these changes Oct 7, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Oct 7, 2025

openshift-ci bot assigned JustinKuli Oct 7, 2025

openshift-ci bot added the lgtm label Oct 7, 2025

openshift-ci bot removed the do-not-merge/hold label Oct 8, 2025

openshift-merge-bot bot merged commit d2c11fc into open-cluster-management-io:main Oct 8, 2025
20 checks passed

This was referenced Oct 8, 2025

🤖 Sync from open-cluster-management-io/config-policy-controller: #388 stolostron/config-policy-controller#1512

Merged

😿 Failed to sync the upstream PRs: #388 stolostron/config-policy-controller#1513

Closed

Optionally read cluster resources with dryrun CLI #388

Optionally read cluster resources with dryrun CLI #388

Uh oh!

Conversation

jan-law commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhaiducek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 7, 2025

Uh oh!

jan-law commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jan-law commented Sep 15, 2025 •

edited

Loading