kubetest2-kops: use ephemeral discovery store for AWS #17809

ameukam · 2025-12-11T11:19:06Z

Retake of #17520

In addition:

ensure bucket is empty before deletion
use exponential back-off to apply public read policy
ensure bucket public access policy is disabled

Signed-off-by: Marko Mudrinić <[email protected]>

Signed-off-by: Arnaud Meukam <[email protected]>

Ensure all the objects of a bucket are deleted before it's deleted. AWS S3 requires the bucket is empty before deletion Signed-off-by: Arnaud Meukam <[email protected]>

5 attempts are done to ensure the public read policy is applied Signed-off-by: Arnaud Meukam <[email protected]>

k8s-ci-robot · 2025-12-11T11:19:10Z

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

ameukam · 2025-12-11T11:22:14Z

/test pull-kops-aws-distro-debian13

ameukam · 2025-12-11T12:58:45Z

/test pull-kops-aws-distro-debian13

ameukam · 2025-12-11T13:46:48Z

/test pull-kops-aws-distro-al2023
/test pull-kops-aws-distro-ubuntu2404
/test pull-kops-e2e-cni-cilium

hakman · 2025-12-11T15:12:51Z

@ameukam This needs to be tested with something that enables discovery store. Do we have such job?

ameukam · 2025-12-11T15:23:39Z

@ameukam This needs to be tested with something that enables discovery store. Do we have such job?

I don't think we have something like that as a presubmit.

ameukam · 2025-12-11T15:42:49Z

/test pull-kops-e2e-cni-cilium

hakman · 2025-12-11T15:46:32Z

Please ignore the failing tests, let's get the review going first.

ameukam · 2025-12-11T16:22:54Z

cc @justinsb @rifelpet

hakman

The main part looks great, thank you both @ameukam & @xmudrii for this.
There are still some Q, though nothing huge.

hakman · 2025-12-11T19:20:14Z

tests/e2e/kubetest2-kops/aws/s3.go

+	// Empty the bucket first
+	paginator := s3.NewListObjectsV2Paginator(c.s3Client, &s3.ListObjectsV2Input{
+		Bucket: aws.String(bucketName),
+	})


It is a good idea, but I would expect that kOps removes all the files from the bucket, otherwise it's a bug.
This approach may hide such issues.

kOps force delete the S3 buckets at shutdown whether the bucket is empty or not. This is just an extra step to guarantee the bucket is properly deleted.

Potential issues will probably not of the emptiness of the bucket.

Happy to drop this if you think it's unnecessary

hakman · 2025-12-11T19:23:26Z

tests/e2e/kubetest2-kops/aws/s3.go

+	var err error
+	backoff := time.Second
+	for i := 0; i < 5; i++ {
+		_, err = c.s3Client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: aws.String(bucketName),
+			Policy: aws.String(policy),
+		})
+		if err == nil {
+			return nil
+		}
+		klog.Infof("Failed to set public read policy on bucket %s, retrying in %v. Error: %v", bucketName, backoff, err)
+		time.Sleep(backoff)
+		backoff *= 2


I am not sure if exponential backoff is the right approach here:

it is not a recurring task

it does not happen concurrently

we don't expect rate limiting.
I would vote for keeping it simple with some fixed interval and number of retries, making the behaviour more predictable.

+1. I'll set a time sleep of 10 seconds

hakman · 2025-12-11T19:24:38Z

tests/e2e/kubetest2-kops/aws/s3.go

+			BlockPublicAcls:       aws.Bool(false),
+			IgnorePublicAcls:      aws.Bool(false),
+			BlockPublicPolicy:     aws.Bool(false),
+			RestrictPublicBuckets: aws.Bool(false),


Should we keep the ACLs to 'true`?

I was not sure of the ACL requirements of kOps for the service discovery so I went with a simple configuration. We can try what you are suggesting.

hakman · 2025-12-11T19:28:58Z

tests/e2e/kubetest2-kops/deployer/common.go

+				return ""
+			}
+			d.createBucket = true
+			discovery = "s3://" + bucketName


I was expecting this to be used somewhere, by being passed to kOps when creating the cluster.
Maybe it's me, but I don't see it used in any of the tests.

My understanding is that the test will need a discovery bucket by default* and we just want to make sure we don't depends on a static reference of an AWS resource we (sig-k8s-infra) currently don't have control on.

*based on: https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/kops/17809/pull-kops-aws-distro-al2023/1999113652837289984/build-log.txt

I1211 13:57:27.886490 13618 s3.go:129] Bucket k8s-infra-kops-state-ae62-20251211135010 created successfully I1211 13:57:28.292397 13618 s3.go:129] Bucket k8s-infra-kops-discovery-ae62-20251211135727 created successfully I1211 13:57:28.648562 13618 s3.go:146] Public read policy set on bucket k8s-infra-kops-discovery-ae62-20251211135727

I1211 14:34:05.635169 13618 s3.go:222] Bucket k8s-infra-kops-state-ae62-20251211135010 deleted I1211 14:34:05.920863 13618 s3.go:222] Bucket k8s-infra-kops-discovery-ae62-20251211135727 deleted

kOps requires --discovery-store to be passed to tests needing it (see kubernetes/test-infra#35648 as reference). In the past, due to the static nature of the bucket, it was passed as part of the test definition. As it is dynamically created now, kubetest2-kops needs to pass it to kOps somehow (either as flag or new env var, similar to the state store).

It's preferable we pass it as a env var in prowjob definition to avoid hard coding it. Setup a dedicated bucket is cheap.

Sorry, but not sure I understand what you mean. We cannot pass it from prow anymore, as it's dynamically generated.

Ensure public access policies can be disabled when the bucket is public Signed-off-by: Arnaud Meukam <[email protected]>

k8s-ci-robot · 2025-12-12T17:16:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from hakman. 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

xmudrii and others added 4 commits December 11, 2025 10:49

kubetest2-kops: use ephemeral discovery store for AWS

eb3936d

Signed-off-by: Marko Mudrinić <[email protected]>

Use variable bucket name regex

5147a42

Signed-off-by: Arnaud Meukam <[email protected]>

Empty the S3 bucket before it's deleted

1861446

Ensure all the objects of a bucket are deleted before it's deleted. AWS S3 requires the bucket is empty before deletion Signed-off-by: Arnaud Meukam <[email protected]>

Use exponential backoff to apply public read policy

719ced0

5 attempts are done to ensure the public read policy is applied Signed-off-by: Arnaud Meukam <[email protected]>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2025

k8s-ci-robot requested review from johngmyers and zetaab December 11, 2025 11:19

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2025

ameukam marked this pull request as ready for review December 11, 2025 15:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2025

hakman requested changes Dec 11, 2025

View reviewed changes

k8s-ci-robot assigned hakman Dec 11, 2025

Ensure public access can be disabled

aa03eb6

Ensure public access policies can be disabled when the bucket is public Signed-off-by: Arnaud Meukam <[email protected]>

ameukam force-pushed the test-aws-discovery-store branch from a332cc7 to aa03eb6 Compare December 12, 2025 17:16

kubetest2-kops: use ephemeral discovery store for AWS #17809

Are you sure you want to change the base?

kubetest2-kops: use ephemeral discovery store for AWS #17809

Conversation

ameukam commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

hakman commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

hakman commented Dec 11, 2025

Uh oh!

ameukam commented Dec 11, 2025

Uh oh!

hakman 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!

hakman Dec 11, 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!

k8s-ci-robot commented Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ameukam commented Dec 11, 2025 •

edited

Loading

hakman Dec 11, 2025 •

edited

Loading