Add NodePool STS Permissions Test for Day 2 Operations #3001

Amarthya-v · 2025-08-04T16:08:44Z

This test validates STS permissions required for ROSA HCP NodePool operations:

ec2:RunInstances (create EC2 instances for new nodes)
ec2:CreateTags (tag new instances)
ec2:DescribeInstances (validate nodes exist in AWS)

The test performs day 2 operations by creating a NodePool, waiting for nodes
to provision, and scheduling workloads. Test failures block releases until
STS permissions are fixed, ensuring proper AWS access for cluster operations.

Fixes: https://issues.redhat.com/browse/OSD-28896

openshift-ci · 2025-08-04T16:08:49Z

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

openshift-ci · 2025-08-04T16:09:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

OWNERS

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

btoll · 2025-08-04T21:55:45Z

@Amarthya-v Could you remove the trailing whitespace in the file? I use https://github.com/bitc/vim-bad-whitespace for vim, I'm sure there's a similar plugin for VS Code.

btoll

I'm wondering if should create nodepool and provision EC2 instances with proper STS permissions could be split up into smaller tests. For example, would it make sense to have a test just to create the nodepool and then a second to do the rest?

Could you add more tests? Also, can you test the bad paths as well as the happy paths?

pkg/e2e/verify/nodepool.go

christophermancini · 2025-08-13T13:45:01Z

/test all

christophermancini · 2025-08-13T13:47:12Z

@Amarthya-v looks like you have some failing checks at the moment.

openshift-ci · 2025-08-13T13:56:10Z

@Amarthya-v: 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/code-quality-checks	`5e1aef9`	link	true	`/test code-quality-checks`
ci/prow/hypershift-pr-check	`5e1aef9`	link	false	`/test hypershift-pr-check`

Full PR test history. Your PR dashboard.

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.

pkg/e2e/verify/nodepool.go

btoll · 2025-08-15T04:56:20Z

pkg/e2e/verify/nodepool.go

+				ginkgo.GinkgoLogr.Error(err, "Failed to cleanup test NodePool", "name", testNodePoolName)
+			}
+
+			ginkgo.By("Cleaning up test pods")


Wouldn't deleting the nodepool also delete its pods?

pkg/e2e/verify/nodepool.go

btoll · 2025-08-20T22:12:22Z

pkg/e2e/verify/nodepool.go

+			np, err := h.Dynamic().Resource(nodePoolGVR).Namespace(clusterNamespace).
+				Get(ctx, testNodePoolName, metav1.GetOptions{})
+			if err != nil { return false, err }
+			ready, _, _ := unstructured.NestedInt64(np.Object, "status", "readyReplicas")


We don't want to check for an error here?

pkg/e2e/verify/nodepool.go

btoll · 2025-09-05T02:26:25Z

pkg/e2e/verify/nodepool.go

+	return false
+}
+
+func failWithSTSError(err error, operation, permissions string) {


Why not combine this with isSTSError and call it something like failIfSTSError?

btoll · 2025-09-05T02:29:03Z

pkg/e2e/verify/nodepool.go

+
+func getClusterNamespace(h *helper.H) (string, error) {
+	if name := viper.GetString(config.Cluster.Name); name != "" {
+		return "clusters-" + name, nil


You're using fmt.Sprintf in other functions but you're not using it here. Probably be good to be consistent.

Fixed now using fmt.Sprintf consistently

btoll · 2025-09-05T02:32:09Z

pkg/e2e/verify/nodepool.go

+
+	ginkgo.BeforeAll(func() {
+		cloudProvider := viper.GetString(config.CloudProvider.CloudProviderID)
+		if cloudProvider != "aws" {


How about using a constant here for the "aws" string and anywhere else you have "aws"? (like line 274)

btoll · 2025-09-05T02:39:29Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.It("rejects operations on non-existent NodePools", func(ctx context.Context) {
+		if clusterNamespace == "" || !canAccessNodePoolAPI(h, clusterNamespace) {


Once here, clusterNamespace should never by an empty string, should it? And, can the canAccessNodePoolAPI test be done before any tests are run?

moved API access check to BeforeAll and removed redundant namespace checks

btoll · 2025-09-05T02:39:52Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.It("rejects duplicate NodePool creation", func(ctx context.Context) {
+		if clusterNamespace == "" || !canAccessNodePoolAPI(h, clusterNamespace) {


Once here, clusterNamespace should never by an empty string, should it? And, can the canAccessNodePoolAPI test be done before any tests are run?

btoll · 2025-09-05T02:41:05Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.AfterAll(func() {
+		if createdNodePool && testNodePoolName != "" && clusterNamespace != "" {


Is this conditional necessary? Are any of the tests doing any node pool cleanup? And, if they are, should they be?

btoll · 2025-09-05T02:42:05Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.It("creates new NodePool successfully", func(ctx context.Context) {
+		if clusterNamespace == "" {


Once here, clusterNamespace should never by an empty string, should it? And, can the canAccessNodePoolAPI test be done before any tests are run?

Note, this isn't consistent with the others.

btoll · 2025-09-05T02:44:13Z

pkg/e2e/verify/nodepool.go

+		}
+
+		expect.NoError(err, "NodePool creation failed")
+		createdNodePool = true


How about putting any test that is dependent upon createdNodePool being true to be in the same block? Otherwise, it's kind of kludgy.

Grouped NodePool creation and provisioning tests in same Describe block

btoll · 2025-09-05T02:45:32Z

pkg/e2e/verify/nodepool.go

+		expect.NoError(err, "Failed to find ready NodePool nodes")
+		Expect(len(nodePoolNodes)).To(BeNumerically(">", 0), "No ready NodePool nodes available")
+
+		targetNode := &nodePoolNodes[0]


What if there are several nodepool nodes in this slice? Would it matter?

Added logic to prefer ready nodes instead of just taking the first

btoll · 2025-09-05T02:47:16Z

pkg/e2e/verify/nodepool.go

+		expect.NoError(err, "Should create pod with invalid selector")
+		defer client.Delete(ctx, pod)
+
+		time.Sleep(30 * time.Second)


Why this arbitrary sleep?

Replaced with proper wait condition using wait.For

btoll · 2025-09-05T02:47:55Z

pkg/e2e/verify/nodepool.go

+		expect.NoError(err, "Failed to find ready NodePool nodes")
+		Expect(len(nodePoolNodes)).To(BeNumerically(">", 0), "No ready NodePool nodes available")
+
+		targetNode := &nodePoolNodes[0]


Same comment as above about the possibility of several nodepool nodes.

btoll · 2025-09-05T02:51:39Z

pkg/e2e/verify/nodepool.go

+	}
+}
+
+func createPodWithSelector(namespace string, nodeSelector map[string]string) *corev1.Pod {


It looks like this function is virtually equivalent to createTestPod?

Created single generic createPod function to eliminate duplication

btoll · 2025-09-05T02:52:59Z

pkg/e2e/verify/nodepool.go

+	}
+}
+
+func createResourceIntensivePod(namespace, nodeName string) *corev1.Pod {


You could probably also roll this into the same generic createPod function.

btoll · 2025-09-05T02:53:53Z

pkg/e2e/verify/nodepool.go

+
+	ginkgo.AfterAll(func() {
+		if createdNodePool && testNodePoolName != "" && clusterNamespace != "" {
+			cleanupNodePool(context.Background(), h, clusterNamespace, testNodePoolName)


Since this is only done once, do you think you need a separate function to do it? What about inlining it here?

…etter namespace lookup

btoll · 2025-09-10T03:23:58Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.AfterAll(func() {
+		if createdNodePool {


Wouldn't you want to move this entire block into the one that starts on line 95?

btoll · 2025-09-10T03:25:27Z

pkg/e2e/verify/nodepool.go

+	})
+
+	ginkgo.Describe("NodePool creation and provisioning", func() {
+		ginkgo.It("creates new NodePool successfully", func(ctx context.Context) {


Wouldn't you want to create the nodepool in a BeforeAll and then you can get rid of the createdNodePool boolean?

btoll · 2025-09-10T03:25:59Z

pkg/e2e/verify/nodepool.go

+		Expect(apierrors.IsNotFound(err)).To(BeTrue(), "Should return NotFound error")
+	})
+
+	ginkgo.It("rejects duplicate NodePool creation", func(ctx context.Context) {


Why not move this into the Describe block above for the nodepool?

btoll · 2025-09-10T03:27:04Z

pkg/e2e/verify/nodepool.go

+	return "", fmt.Errorf("could not determine cluster namespace")
+}
+
+func canAccessNodePoolAPI(h *helper.H, namespace string) bool {


Would you want to fail the entire suite if this cannot be accessed and exit early? You could also then remove all of the canAccessAPI checks.

btoll · 2025-09-10T03:38:08Z

pkg/e2e/verify/nodepool.go

+	for _, pattern := range patterns {
+		if strings.Contains(msg, pattern) {
+			ginkgo.Fail(fmt.Sprintf(
+				"STS_PERMISSION_ERROR: %s failed due to missing AWS permissions (%s): %v. "+


This should be cleaned up. There's no need to concat strings within a fmt.Sprintf method call.

btoll · 2025-09-10T03:49:07Z

pkg/e2e/verify/nodepool.go

+		pod.Spec.NodeSelector = config.nodeSelector
+	}
+
+	if config.cpuLimit != "" && config.memLimit != "" {


Do you really need resource limits? Aren't these short-lived pods? Are you anticipating resource spikes that would need to be throttled?

btoll · 2025-09-10T03:50:35Z

pkg/e2e/verify/nodepool.go

+		},
+	}
+
+	if config.nodeName != "" {


Do you need this check and the one below it? Can't it just be defined in the above struct?

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 Aug 4, 2025

btoll reviewed Aug 4, 2025

View reviewed changes

btoll reviewed Aug 5, 2025

View reviewed changes

pkg/e2e/verify/nodepool.go Outdated Show resolved Hide resolved

Amarthya-v force-pushed the nodepool-sts-permissions-test branch from 2fc11fd to 3e9ef47 Compare August 8, 2025 20:09

btoll reviewed Aug 12, 2025

View reviewed changes

pkg/e2e/verify/nodepool.go Outdated Show resolved Hide resolved

Amarthya-v force-pushed the nodepool-sts-permissions-test branch from 3e9ef47 to 5e1aef9 Compare August 13, 2025 04:23

Amarthya-v force-pushed the nodepool-sts-permissions-test branch 2 times, most recently from ad2b5db to f1274bb Compare August 13, 2025 15:25

btoll reviewed Aug 15, 2025

View reviewed changes

Amarthya-v force-pushed the nodepool-sts-permissions-test branch 2 times, most recently from d9b4fe5 to 37a587c Compare August 19, 2025 03:38

btoll reviewed Aug 20, 2025

View reviewed changes

btoll reviewed Aug 21, 2025

View reviewed changes

pkg/e2e/verify/nodepool.go Outdated Show resolved Hide resolved

Amarthya-v force-pushed the nodepool-sts-permissions-test branch from 37a587c to d0df745 Compare August 28, 2025 03:15

Add NodePool STS permissions test

bc48a6d

Amarthya-v force-pushed the nodepool-sts-permissions-test branch from d0df745 to bc48a6d Compare September 4, 2025 13:59

btoll reviewed Sep 5, 2025

View reviewed changes

NodePool STS Permissions-tighter STS detection, wait-based cleanup, b…

26e1e43

…etter namespace lookup

btoll reviewed Sep 10, 2025

View reviewed changes

Add NodePool STS Permissions Test for Day 2 Operations #3001

Are you sure you want to change the base?

Add NodePool STS Permissions Test for Day 2 Operations #3001

Uh oh!

Conversation

Amarthya-v commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 4, 2025

Uh oh!

openshift-ci bot commented Aug 4, 2025

Uh oh!

btoll commented Aug 4, 2025

Uh oh!

btoll left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

christophermancini commented Aug 13, 2025

Uh oh!

christophermancini commented Aug 13, 2025

Uh oh!

openshift-ci bot commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btoll Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

Amarthya-v commented Aug 4, 2025 •

edited

Loading

btoll Sep 5, 2025 •

edited

Loading

btoll Sep 10, 2025 •

edited

Loading

btoll Sep 10, 2025 •

edited

Loading