✨ envtest: search the assets index for latest of a release series

[cbandy]

This lets one configure the DownloadBinaryAssetsVersion field of envtest.Environment with only the Major.Minor portions of a Kubernetes API version. When DownloadBinaryAssets is set, the framework will search the asset index for the latest stable version in that release series.

This matches the behavior of setup-envtest use {major}.{minor} without the full complexity of version selectors. The leading v is now optional, too.

For example:

• "v1.29" resolves to v1.29.4
• "1.27" resolves to v1.27.1
• "1.24" resolves to v1.24.2

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

• OWNERS

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

[k8s-ci-robot]

Hi @cbandy. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[sbueringer]

/ok-to-test

[sbueringer]

When are we hitting this case and what happens then?

[cbandy]

This would theoretically happen when the input is a tolerated version, and/but the string manipulations produce text that is not a range. As I was writing tests approaching this area, I ran into other edge cases of the blang/semver range parsing and behavior. In the current changeset, this moment is the final return nil, nil of interpretVersion.

When this happens, there is no "search" of the index; binaryAssetsVersion is used directly as the key/lookup in downloadBinaryAssetsArchive. That function errors with:

controller-runtime/pkg/envtest/binaries.go

Lines 222 to 225 in 6824653

	archives, ok := index.Releases[version]
	if !ok {
	return fmt.Errorf("failed to find envtest binaries for version %s", version)
	}

[cbandy]

I've rebased and made the requested changes. I switched from blang/semver to Masterminds/semver (MIT) after the tests hit edge cases in its parsing.¹ I found the new package easier to use, and the function turned out the way you imagined:

Can we limit this somehow? For example what if we only support whatever is directly parsable by semver.ParseTolerant and semvar.ParseRange?

I left the latest changes as a separate commit in case that helps during review. I'd like to squash this down before merge, tho.

Footnotes

1. https://www.github.com/blang/semver/issues/80#issuecomment-2316760091 ↩

[sbueringer]

The nice thing about using blang/semver was that we already had this as a dependency and we shared this dependency with k/k.

The nice thing about mostly sticking to k/k's dependencies is that they take care of all the maintenance for us (including sometimes forking abandoned dependencies if necessary).

Now we have an additional dependency that everyone that uses controller-runtime inherits

To be honest, I'm not sure if we can justify this for version parsing in envtest

[sbueringer]

@alvaroaleman What do you think?

[alvaroaleman]

I don't really mind to be honest - Its a commonly-used lib and it has zero dependencies itself: https://github.com/Masterminds/semver/blob/bf01c618459762fa583c24476ec46957a330c737/go.mod

[sbueringer]

Yeah. I would just try to keep the number of transitive dependencies of CR low (especially the ones we add on top of k8s.io/* transitive dependencies)

[cbandy]

I forgot about these packages:

• https://pkg.go.dev/k8s.io/apimachinery/pkg/version
• https://pkg.go.dev/k8s.io/apimachinery/pkg/util/version

🤔 The fuzzy matching I'm after may just be ParseMajorMinor... lol, no rush on this PR. I'll poke it again before EOW.

I've used the latter package when testing edge cases in SSA over the years:

		switch {
		case serverVersion.LessThan(version.MustParseGeneric("1.25.15")):
		case serverVersion.AtLeast(version.MustParseGeneric("1.26")) && serverVersion.LessThan(version.MustParseGeneric("1.26.10")):
		case serverVersion.AtLeast(version.MustParseGeneric("1.27")) && serverVersion.LessThan(version.MustParseGeneric("1.27.7")):

			assert.Assert(t, after.GetResourceVersion() != before.GetResourceVersion(),
				"expected https://issue.k8s.io/116861")

		default:
			assert.Assert(t, after.GetResourceVersion() == before.GetResourceVersion())
		}

[cbandy]

I forgot about these packages:

• https://pkg.go.dev/k8s.io/apimachinery/pkg/version
• https://pkg.go.dev/k8s.io/apimachinery/pkg/util/version

The latter apimachinery package worked well! Rather than add a rewrite here, I'm closing this to be replaced by #3280. Both PRs are about the same size, but the new one seems more straightforward.