⚠️ Envtest refactor, support for 1.20+ API servers (secure serving) #1486

DirectXMan12 · 2021-04-20T07:16:54Z

This introduces (pluggable) authentication support to envtest, which is required for 1.20+ API servers. The default authentication provider is cert-based, since we already have TinyCA, and cert-based auth can provision users both before & after starting the API server. Theoretically, other options should be available as well. Correspondingly, RBAC is now turned on for authorization, but this should not impact existing users (see below).

We continue to provide support for the legacy helpers on controlplane to fetch a rest config & kubectl, but deprecate them in favor of methods to provision a new user. The RESTConfig field on envtest, however, is not deprecated.

We also deprecate insecure serving, since it's going away anyway, and add some new fields to support this. Theoretically, this should be largely transparent to end users, since I tried to backfill all legacy functionality with secure equivalents automatically (e.g. all "give me connection details" should now give an admin user, which should be transparent in terms of permissions). However if any users are manually constructing a RESTConfig without using the envtest fields or helper methods (i.e. just grabbing the API server URL from envtest), this will break them, so it's technically a breaking change.

Lmk if you spot anything that looks more seriously breaking, but AFAICT by-and-large this should just be a transparent switch with a nice new benefit of "also you can now test controller RBAC".

Closes #1357

Depends on #1541
Depends on #1540

DirectXMan12 · 2021-04-20T07:17:28Z

/assign @alvaroaleman

pkg/internal/testing/controlplane/plane.go

lobziik · 2021-04-20T12:11:28Z

One thing i've noticed - control plane wont start again after stop because temp directory with certificates was removed during stop procedure and didn't create again. Throws panic like this:

panic: unable to start control plane itself: failed to start the controlplane. retried 5 times: unable to save the client certificate CA to /tmp/k8s_test_framework_269505290/client-cert-auth-ca.crt: open /tmp/k8s_test_framework_269505290/client-cert-auth-ca.crt: no such file or directory

I was solve it with crutch like this: https://github.com/kubernetes-sigs/controller-runtime/pull/1477/files#diff-c6321f65a2defa4f1d2a0810e6262d27a4a7e55c75db83330bf396a3148bc72aR109. Probably, it might be worked around more elegant way.

Also, suggest to get back such test, which seemd to be removed here: https://github.com/kubernetes-sigs/controller-runtime/pull/1486/files#diff-c4ae583bca86061bdfa2f60d76bdf50eba0f945fd1f3b56bea8edcb4e268ee0cL147 ( pkg/internal/testing/integration/internal/integration_tests/integration_test.go Line 147) :)

DirectXMan12 · 2021-04-20T18:41:11Z

Good catch on the restart issue. I'll take a look. There's some weird interplay between "I want a manually specified certdir" & "I want to be able to restart and get a new temp dir".

DirectXMan12 · 2021-04-20T18:58:10Z

Actually, I think the extent of this should probably be discussed. It's not clear to me that this is a use case we can safely support while keeping our existing semantics -- if we clean up the work dir, we invalidate all existing users that have been provisioned under most auth strategies.

We can just recreate the work dir, but it's not clear to me that that's not gonna be surprising. I'll put the functionality to do that back in for now, but I think we probably want to deprecate that bit of functionality and just say "if you want a new control plane instance, ask for one".

I wonder what the use case here is?

DirectXMan12 · 2021-04-20T20:19:42Z

Ok, I've fixed the "can't re-start a ControlPlane" issue and added a basic test.

timebertt

Nice work, looking forward to using it :)
Left you a few comments inline, will give this a test spin later this week with the envtest suites in our project.

pkg/internal/testing/process/process.go

pkg/internal/testing/process/arguments.go

timebertt · 2021-05-04T12:03:44Z

pkg/internal/testing/controlplane/apiserver.go

+		}
+	} else if insec := s.InsecureServing; insec != nil {
+		if insec.Port == "" || insec.Address == "" {
+			port, host, err := addr.Suggest("")


Should we pass InsecureServing.Address down to addr.Suggest?

Suggested change

port, host, err := addr.Suggest("")

if insec.Port == "" {

port, host, err := addr.Suggest(insec.Address)

I was mostly intending on porting the suggestion logic as-is from what it is currently to avoid breaking things -- the old code never passed anything to addr.Suggest AFAICT

Yep, also checked how it was done before. This was just a suggestion on top, nothing critical.

timebertt

I gave this a test spin now in our envtest suites and found a few issues, you might wanna address (mainly to make it more backwards-compatible and easier to upgrade to for users).
Also noticed some nits along the way.

Apart from the mentioned issues, everything worked pretty well! :)

pkg/envtest/server.go

pkg/internal/testing/controlplane/apiserver.go

pkg/internal/testing/controlplane/kubectl.go

pkg/internal/testing/process/arguments.go

alvaroaleman

Initial, somewhat cursory pass. This change is simply too big for me to be able to properly review it 🙃

examples/scratch-env/main.go

alvaroaleman · 2021-05-05T14:05:55Z

pkg/manager/manager_suite_test.go

+		// so we grab the transport right after it gets created so that we can
+		// type-assert on it (hopefully)?
+		// hopefully this doesn't break 🤞
+		clientTransport = rt.(*http.Transport)


What about intead of asserting here just changing var clientTransport *http.Transport to http.RoundTripper and doing the type assertion when we close the keep-alives? That would make this easier to debug if something fails (right now we'd just get a NPD, right?)

meh, then we have more type assertions, though :-/

I can def put a better panic message in though, which should help there.

pkg/envtest/server.go

alvaroaleman · 2021-05-05T14:14:34Z

pkg/internal/testing/certs/tinyca_test.go

+		})
+	})
+
+	It("should produce unique serials among all generated certificates of all types", func() {


Out of curiosity, does this have any practical relevance? I know the RFC requires it, but is it used for anything other than CRLs?

I don't actually remember. I remember thinking when I wrote TinyCA, "hmm, I should do this semi-correctly and generate unique serials at least", but I don't remember exactly why I thought that

alvaroaleman · 2021-05-05T14:16:08Z

pkg/internal/testing/certs/tinyca_test.go

+				// might want to reconsider.
+
+				localhostAddrs, err := net.LookupHost("localhost")
+				Expect(err).NotTo(HaveOccurred(), "should be able to find IPs for localhost")


Are you sure you want to do this lookup? I have seen it returning very surprising results at times :P

we actually do this currently within the code (perhaps unfortunately, depending on how you're looking at it), so it's there for compat reasons. This just adds a test for that behavior.

alvaroaleman · 2021-05-05T14:21:51Z

pkg/internal/testing/controlplane/apiserver.go

+		// required on 1.20+, fine to leave on for <1.20
+		"service-account-issuer":           []string{s.SecureServing.URL("https", "/").String()},
+		"service-account-key-file":         []string{filepath.Join(s.CertDir, saCertFile)},
+		"service-account-signing-key-file": []string{filepath.Join(s.CertDir, saKeyFile)},


Why does the Apiserver needs this if the CM creates the SAs?

You know, I'm not sure, but it does 🤷‍♀️ I'll look it up.

pkg/envtest/server.go

DirectXMan12 · 2021-05-05T20:22:49Z

This change is simply too big for me to be able to properly review it

@alvaroaleman I think what I'm gonna do here is finish addressing the comments from @timebertt and you, and then break this up into 3-4 smallers PRs, which should be more digestable.

DirectXMan12 · 2021-05-25T20:24:09Z

I've split the refactor of testing and the introduction of structured arguments into two separate PRs.

DirectXMan12 · 2021-05-25T20:24:21Z

Gotta rebase juggle now.

DirectXMan12 · 2021-05-26T22:01:20Z

Ok, I split the license change out into a separate PR to hopefully reduce the noise on this one as well a bit.

examples/scratch-env/go.mod

This introduces secure serving by default, transparently switching configs to connect to secure endpoints. Similarly, the insecure endpoints are now disabled by default -- they must be explicitly disabled if you want to use them (note that this will only work on API servers <=1.19). This also turns on flags around the tokenrequest endpoint which are required to be on in 1.20, and have been available for all the versions that we currently support (back to 1.16). There's a whole new set of control plane APIs for provisioning users, and the old "just get a single account" APIs of the internal package are mostly deprecated. The default `Environment.Config` is *NOT* deprecated however -- this is intended to continue working as normal in order to avoid breaking everyone and to keep things in envtest working easily -- most tests will be fine with an admin user. For users that want RBAC, individual users may be provisioned with the ControlPlane.AddUser method.

This adds a full test suite for TinyCA, which was missing before, and is especially needed with the new envtest client cert generation.

Previously, envtest had separate logic that overrode testing/process.BinPathFinder, mainly because the latter used to be in a separate module. Since they're now in the same module and BinPathFinder is internal, we can just unify the logic and remove some of our hacks.

This adds fairly comprehensive tests for the control plane components, which either didn't exist before the refactor or existed very loosely in the form of general integration tests.

This exposes a few pieces of the new APIs in internal/testing as public in envtest. These types are effectively part of public APIs (they're referenced by public methods or fields of APIServer, ControlPlane, etc), so we should let users name them.

This makes sure that you can start a ControlPlane after stopping it. This preserves existing functionality. Note that the ability to re-use users or keep data around is *not* preserved -- use a fixed CertDir if you want this.

vincepri

/lgtm

k8s-ci-robot · 2021-06-01T21:11:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12,vincepri]

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

The only place I've seen lll complain much is flag help, error messsages, and such, which aren't really a problem when they're long. Other stuff like complexity is probably best left to review, IMO.

vincepri · 2021-06-01T21:20:00Z

/lgtm

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 20, 2021

k8s-ci-robot requested review from alenkacz and shawn-hurley April 20, 2021 07:17

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 20, 2021

k8s-ci-robot assigned alvaroaleman Apr 20, 2021

DirectXMan12 force-pushed the feature/envtest-secure-port branch from 94acd08 to 0abb95a Compare April 20, 2021 07:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2021

lobziik reviewed Apr 20, 2021

View reviewed changes

pkg/internal/testing/controlplane/plane.go Show resolved Hide resolved

lobziik mentioned this pull request Apr 20, 2021

🌱 Envtest exports secure config, helpers for simplify users adding #1477

Closed

DirectXMan12 force-pushed the feature/envtest-secure-port branch from cbc2eb8 to f2e2c75 Compare April 20, 2021 20:28

This was referenced Apr 21, 2021

✨ Envtest exports rest.Config for the secure endpoint of kube-apiserver #984

Closed

'envtest' should support test scenarios with Authn/Authz configurations (should expose secure endpoint info of kube-apiserver) #983

Closed

timebertt mentioned this pull request May 4, 2021

🌱 Prepare envtest certs for aggregation layer #1449

Closed

timebertt reviewed May 4, 2021

View reviewed changes

timebertt reviewed May 5, 2021

View reviewed changes

alvaroaleman reviewed May 5, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021

This was referenced May 25, 2021

✨ Clean up pkg/internal/testing #1540

Merged

✨ Structured args in Testing #1541

Merged

DirectXMan12 force-pushed the feature/envtest-secure-port branch from f2e2c75 to 15dcbe3 Compare May 26, 2021 21:22

DirectXMan12 mentioned this pull request May 26, 2021

🌱 Add missing license headers to files #1542

Merged

DirectXMan12 force-pushed the feature/envtest-secure-port branch from 15dcbe3 to d4c8079 Compare May 26, 2021 21:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021

vincepri reviewed Jun 1, 2021

View reviewed changes

examples/scratch-env/go.mod Outdated Show resolved Hide resolved

DirectXMan12 force-pushed the feature/envtest-secure-port branch from d4c8079 to 7ed222c Compare June 1, 2021 20:52

DirectXMan12 added 6 commits June 1, 2021 14:09

Tests for TinyCA

848bc88

This adds a full test suite for TinyCA, which was missing before, and is especially needed with the new envtest client cert generation.

Tests for testing/controlplane components

c0ca959

This adds fairly comprehensive tests for the control plane components, which either didn't exist before the refactor or existed very loosely in the form of general integration tests.

Ensure testing ControlPlane is restartable

6fd12aa

This makes sure that you can start a ControlPlane after stopping it. This preserves existing functionality. Note that the ability to re-use users or keep data around is *not* preserved -- use a fixed CertDir if you want this.

DirectXMan12 force-pushed the feature/envtest-secure-port branch from 7ed222c to 6fd12aa Compare June 1, 2021 21:09

vincepri approved these changes Jun 1, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Jun 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2021

Disable lll

e77a2fc

The only place I've seen lll complain much is flag help, error messsages, and such, which aren't really a problem when they're long. Other stuff like complexity is probably best left to review, IMO.

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2021

k8s-ci-robot merged commit ab7825e into kubernetes-sigs:master Jun 1, 2021

k8s-ci-robot added this to the v0.9.x milestone Jun 1, 2021

DirectXMan12 deleted the feature/envtest-secure-port branch June 1, 2021 21:23

timebertt mentioned this pull request Jun 16, 2021

Upgrade sigs.k8s.io/controller-runtime to v0.9, k8s.io/* to v0.21 gardener/gardener#4174

Closed

wonderflow mentioned this pull request Jul 30, 2021

upgrade K8s dependency lib to v0.21 kubevela/kubevela#1985

Merged

alvaroaleman mentioned this pull request Oct 8, 2021

Secure serving support #788

Closed

wallrj mentioned this pull request Jan 11, 2024

Stop using deprecated f.environment.ControlPlane.KubeCtl function cert-manager/cert-manager#6627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Envtest refactor, support for 1.20+ API servers (secure serving) #1486

⚠️ Envtest refactor, support for 1.20+ API servers (secure serving) #1486

DirectXMan12 commented Apr 20, 2021 •

edited

Loading

DirectXMan12 commented Apr 20, 2021

lobziik commented Apr 20, 2021 •

edited

Loading

DirectXMan12 commented Apr 20, 2021

DirectXMan12 commented Apr 20, 2021

DirectXMan12 commented Apr 20, 2021

timebertt left a comment

timebertt May 4, 2021

DirectXMan12 May 4, 2021

timebertt May 5, 2021

timebertt left a comment

alvaroaleman left a comment

alvaroaleman May 5, 2021

DirectXMan12 Jun 1, 2021

alvaroaleman May 5, 2021

DirectXMan12 May 5, 2021

alvaroaleman May 5, 2021

DirectXMan12 May 5, 2021

alvaroaleman May 5, 2021

DirectXMan12 May 25, 2021

DirectXMan12 commented May 5, 2021

DirectXMan12 commented May 25, 2021

DirectXMan12 commented May 25, 2021

DirectXMan12 commented May 26, 2021

vincepri left a comment

k8s-ci-robot commented Jun 1, 2021

vincepri commented Jun 1, 2021

	port, host, err := addr.Suggest("")
	if insec.Port == "" {
	port, host, err := addr.Suggest(insec.Address)

⚠️ Envtest refactor, support for 1.20+ API servers (secure serving) #1486

⚠️ Envtest refactor, support for 1.20+ API servers (secure serving) #1486

Conversation

DirectXMan12 commented Apr 20, 2021 • edited Loading

DirectXMan12 commented Apr 20, 2021

lobziik commented Apr 20, 2021 • edited Loading

DirectXMan12 commented Apr 20, 2021

DirectXMan12 commented Apr 20, 2021

DirectXMan12 commented Apr 20, 2021

timebertt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timebertt left a comment

Choose a reason for hiding this comment

alvaroaleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented May 5, 2021

DirectXMan12 commented May 25, 2021

DirectXMan12 commented May 25, 2021

DirectXMan12 commented May 26, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 1, 2021

vincepri commented Jun 1, 2021

DirectXMan12 commented Apr 20, 2021 •

edited

Loading

lobziik commented Apr 20, 2021 •

edited

Loading