Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
ameukam wants to merge 7 commits into
base: master
Choose a base branch
from
Open

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

ameukam wants to merge 7 commits into from
+93 −8

Conversation

@ameukam
Copy link
Member

@ameukam ameukam commented Dec 11, 2025 •
edited
Loading

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

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 11, 2025

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

@k8s-ci-robot 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
@ameukam
Copy link
Member Author

ameukam commented Dec 11, 2025

/test pull-kops-aws-distro-debian13

@k8s-ci-robot 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
Copy link
Member Author

ameukam commented Dec 11, 2025

/test pull-kops-aws-distro-debian13

@ameukam
Copy link
Member Author

ameukam commented Dec 11, 2025

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

@ameukam ameukam marked this pull request as ready for review December 11, 2025 15:11
@k8s-ci-robot 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
Copy link
Member

hakman commented Dec 11, 2025

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

@ameukam
Copy link
Member Author

ameukam commented Dec 11, 2025

@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
Copy link
Member Author

ameukam commented Dec 11, 2025

/test pull-kops-e2e-cni-cilium

@hakman
Copy link
Member

hakman commented Dec 11, 2025

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

@ameukam
Copy link
Member Author

ameukam commented Dec 11, 2025

cc @justinsb @rifelpet

hakman
hakman requested changes Dec 11, 2025
View reviewed changes
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tests/e2e/kubetest2-kops/aws/s3.go
Comment on lines +156 to +161
// Empty the bucket first
paginator := s3.NewListObjectsV2Paginator(c.s3Client, &s3.ListObjectsV2Input{
Bucket: aws.String(bucketName),
})
Copy link
Member

@hakman hakman Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ameukam ameukam Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ameukam ameukam force-pushed the test-aws-discovery-store branch from a332cc7 to aa03eb6 Compare December 12, 2025 17:16
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 12, 2025

[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.

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

xmudrii and others added 6 commits December 14, 2025 11:30
@xmudrii @ameukam
kubetest2-kops: use ephemeral discovery store for AWS
0d49465 
Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>
@ameukam
Use variable bucket name regex
0379bcf 
Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@ameukam
Empty the S3 bucket before it's deleted
94ca035 
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 <ameukam@gmail.com>
@ameukam
Ensure public access can be disabled
07e6368 
Ensure public access policies can be disabled when the bucket is public

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@ameukam
Pause for 10 seconds before applying the public read policy to the
ca5559e 
bucket.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@ameukam
Ensure an ephemeral discovery store S3 bucket is used for any test cl…
43cee3e 
…uster

on AWS.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@ameukam ameukam force-pushed the test-aws-discovery-store branch from aa03eb6 to 43cee3e Compare December 14, 2025 12:05
@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 14, 2025
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 14, 2025
@ameukam
Set discovery store with KOPS_DISCOVERY_STORE
6d7230c 
Ensure the discovery store S3 bucket is set with KOPS_DISCOVERY_STORE
value.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@k8s-ci-robot 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 14, 2025
@ameukam
Copy link
Member Author

ameukam commented Dec 14, 2025

@hakman @rifelpet PTAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hakman hakman hakman requested changes

@johngmyers johngmyers Awaiting requested review from johngmyers

@zetaab zetaab Awaiting requested review from zetaab

Assignees

@hakman hakman

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ameukam @k8s-ci-robot @hakman @xmudrii