Skip to content
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

Bumping envtest to 1.24 #5835

Merged
merged 7 commits into from
Jun 11, 2022
Merged

Bumping envtest to 1.24 #5835

merged 7 commits into from
Jun 11, 2022

Conversation

VenkatRamaraju
Copy link
Contributor

No description provided.

@laxmikantbpandhare
Copy link
Member

Changelog is missing. Please add it.

@VenkatRamaraju VenkatRamaraju force-pushed the envtest-bump branch 2 times, most recently from b27a74b to 87e633f Compare June 7, 2022 17:44
@camilamacedo86
Copy link
Contributor

this need wait we bump k8s 1.24 on SDK
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2022
@everettraven everettraven added release-blocker This issue blocks the parent release milestone blocked labels Jun 8, 2022
@everettraven everettraven added this to the v1.22.0 milestone Jun 8, 2022
varshaprasad96
varshaprasad96 reviewed Jun 9, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@VenkatRamaraju VenkatRamaraju force-pushed the envtest-bump branch 3 times, most recently from c4fe852 to 8f6048a Compare June 9, 2022 21:43
bumped envtest to 1.24
c90a728 
Signed-off-by: Venkat Ramaraju <vramaraj@redhat.com>
@camilamacedo86
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2022
camilamacedo86
camilamacedo86 reviewed Jun 9, 2022
View reviewed changes
Makefile Outdated
export K8S_VERSION = 1.23
# TODO: bump this to 1.21, after kubectl `--generator` flag is removed from e2e tests.
export ENVTEST_K8S_VERSION = 1.23.1
export K8S_VERSION = 1.24
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we check here if the kubebuilder tools 1.24.1 is download after the change?
Note that ENVTEST_K8S_VERSION probably is used here to get the kubebuilder-tolls, see: https://storage.googleapis.com/kubebuilder-tools

That is probably the reason for the CI be falling.
It is a block for the release so could we just revert the changes and do the bumps?

K8S_VERSION = 1.24
export ENVTEST_K8S_VERSION = 1.24.1

c/c @varshaprasad96 @VenkatRamaraju @everettraven

Copy link
Member

Choose a reason for hiding this comment

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

Yes, envtest needs a patch version.

Copy link
Member

Choose a reason for hiding this comment

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

Modifying K8S_VERSION to 1.24.1 should solve the issue.

Copy link
Member

Choose a reason for hiding this comment

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

In case it doesn't and if its taking time to debug we could just revert as @camilamacedo86 said, but I'm pretty sure changing it to 1.24.1 would make tests to pass.

Copy link
Contributor

@everettraven everettraven Jun 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@VenkatRamaraju What @varshaprasad96 is saying is correct. I think we could either set K8S_VERSION to 1.24.0 or 1.24.1

Copy link
Member

Choose a reason for hiding this comment

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

K8S_VERSION isn't used by the ENVTEST variable. They are separate AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind. I see we are getting rid of the specific ENV_TEST variable

@everettraven
Copy link
Contributor

@VenkatRamaraju this should be unblocked and likely needs a rebase

@everettraven everettraven mentioned this pull request Jun 10, 2022
Closed
13 tasks
1.24.1
9935111 
Signed-off-by: Venkat Ramaraju <vramaraj@redhat.com>
@varshaprasad96
Copy link
Member

So 1.24.1 does exist and it should work

➜  bin ./setup-envtest use 1.24.x
Version: 1.24.1
OS/Arch: darwin/amd64
md5: v0TKf8PL9Y4KC1Tb/YJJ5g==
Path: /Users/vnarsing/Library/Application Support/io.kubebuilder.envtest/k8s/1.24.1-darwin-amd64

@everettraven
Copy link
Contributor

@VenkatRamaraju doing some extra looking it seems like we will need to update Kind to be v0.14.0 instead of v0.11.0.

In the release notes for Kind v0.13.0:

NOTE: You must use kind v0.13.0+ with Kubernetes v1.24.0+ images, and if you built your own Kubernetes v1.24.0+ image
with a previous kind version you will need to re-built when switching to kind v0.13.0+.

everettraven
everettraven reviewed Jun 10, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
new kind version
4483bef 
Signed-off-by: Venkat Ramaraju <vramaraj@redhat.com>
@everettraven
Copy link
Contributor

@VenkatRamaraju According to the Kubernetes 1.24 Changelog it looks like ServiceAccounts no longer have a Token Secret auto generated.

Changelog: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#urgent-upgrade-notes

Bullet Point 3:

The LegacyServiceAccountTokenNoAutoGeneration feature gate is beta, and enabled by default. When enabled, Secret API objects containing service account tokens are no longer auto-generated for every ServiceAccount. Use the TokenRequest API to acquire service account tokens, or if a non-expiring token is required, create a Secret API object for the token controller to populate with a service account token by following this guide. (kubernetes/kubernetes#108309, @zshihang)

@everettraven
update e2e tests per k8s 1.24 changes
b248154 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
fix helm & ansible e2e
7f6236a 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
@everettraven
add license header to sa_secret.go
c5e75bf 
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
varshaprasad96
varshaprasad96 reviewed Jun 10, 2022
View reviewed changes
test/e2e/ansible/cluster_test.go
@@ -244,6 +245,15 @@ var _ = Describe("Running ansible projects", func() {
}
Eventually(verifyMemcachedPatch, time.Minute, time.Second).Should(Succeed())

// As of Kubernetes 1.24 a ServiceAccount no longer has a ServiceAccount token secret autogenerated. We have to create it manually here
By("Creating the ServiceAccount token")
secretFile, err := common.GetSASecret(tc.Kubectl.ServiceAccount, tc.Dir)
Copy link
Member

Choose a reason for hiding this comment

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

nice work @everettraven 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, thanks @everettraven!

varshaprasad96
varshaprasad96 approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2022
laxmikantbpandhare
laxmikantbpandhare approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@laxmikantbpandhare laxmikantbpandhare left a comment

Choose a reason for hiding this comment

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

/lgtm

Great work!

@jmrodri jmrodri merged commit 013d9c8 into operator-framework:master Jun 11, 2022
@everettraven everettraven mentioned this pull request Jun 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri left review comments

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

@everettraven everettraven everettraven left review comments

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

@laxmikantbpandhare laxmikantbpandhare laxmikantbpandhare approved these changes

@jberkhahn jberkhahn Awaiting requested review from jberkhahn

Assignees

@varshaprasad96 varshaprasad96

@laxmikantbpandhare laxmikantbpandhare

Labels
lgtm Indicates that a PR is ready to be merged. release-blocker This issue blocks the parent release milestone
Projects
None yet
Milestone
v1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@VenkatRamaraju @laxmikantbpandhare @camilamacedo86 @everettraven @varshaprasad96 @jmrodri