Adding an e2e test on GMSA support #82109
Conversation
|
/priority important-soon |
|
@wk8: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility. In response to this:
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/test-infra repository. |
Concerns from alpha-phase reviews have been addressed in kubernetes#80320 and kubernetes#82109 and early adopters have given positive feedback; so it seems there are no blockers to graduate GMSA support to beta. This patch also enables GMSA support by default, mainly for the sake of making it easier for cluster admins to use, as well as for e2e tests in nightly builds. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
e5f4339 to
894c80c
Compare
|
/assign @PatrickLang |
test/e2e/windows/gmsa_full.go
Outdated
There was a problem hiding this comment.
Please do a framework.skipf() here instead of Failf so that this test will not require explicit skipping in clusters where the dependencies are not fulfilled. Check out
or
kubernetes/test/e2e/storage/volume_metrics.go
Line 103 in 07e0cce
for an example.
There was a problem hiding this comment.
Changing, but wouldn't that also have the negative effect of skipping this test in nightly builds (where it should run, and fail, in that case) if the dependencies stop being fulfilled there?
test/e2e/windows/gmsa_full.go
Outdated
There was a problem hiding this comment.
golint requires this to be gmsaWebhookDeployScriptURL instead of gmsaWebhookDeployScriptUrl
|
/assign @adelina-t |
|
/test pull-kubernetes-bazel-build |
|
/test pull-kubernetes-e2e-gce-device-plugin-gpu |
|
/cc @BCLAU |
test/e2e/windows/gmsa_full.go
Outdated
There was a problem hiding this comment.
To be honest, not a fan of this URL being here in the tests. Then again, I don't really have any alternative ideea :/
There was a problem hiding this comment.
Same, didn't love it... Sorry. :(
test/e2e/windows/gmsa_full.go
Outdated
There was a problem hiding this comment.
could go with powershell get-content here. Type is aliased to get-content, but much less known.
test/e2e/windows/gmsa_full.go
Outdated
There was a problem hiding this comment.
more of a nit, but I feel this might go in a utils.go to be reused.
There was a problem hiding this comment.
Indeed. Do you mean a utils.go in the same package? Or something more general?
There was a problem hiding this comment.
in the same package, so it may be used by other tests if needed. It would be nice to have, but not a necessity
|
/lgtm |
The previously existing e2e GMSA test really only tests a small part of the whole GMSA set up process, namely that once the API has inlined the GMSA contents in the pod's spec, and sent that to a worker's kubelet, then the kubelet passes that down to the runtime. This new test, in contrast, really tests the whole thing, i.e. deploying the admission webhook, then deploying a GMSA custom resource, and using that resource within a pod. The downside of this test though, is that it does need to make a lot of assumptions about the cluster it runs against, notably that it runs on a worker node that's already been joined to a working Active Directory domain (there are other assumptions, all documented at the beginning of the test file); for that reason, it is only intended to ever be run against an AKS cluster with the custom AKS extension from kubernetes-sigs/windows-testing#98. Note that this test doesn't aim at testing every edge-case, such as a pod trying to use a GMSA it doesn't have access to; the webhook has its own tests for these. This test's goal is to ensure the happy path doesn't break. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
|
@adelina-t : thanks for the quick review! Amended as requested. |
|
/lgtm |
|
/milestone v1.16 |
| // downloadFile saves a remote URL to a local temp file, and returns its path. | ||
| // It's the caller's responsibility to clean up the temp file when done. | ||
| func downloadFile(url string) (string, error) { | ||
| response, err := http.Get(url) |
There was a problem hiding this comment.
today I learned that this automatically follows 30x redirects https://golang.org/pkg/net/http/#Client.Get
| node := nodes[0] | ||
|
|
||
| ginkgo.By("retrieving the contents of the GMSACredentialSpec custom resource manifest from the node") | ||
| crdManifestContents := retrieveCRDManifestFileContents(f, node) |
There was a problem hiding this comment.
just a nit - if this isn't referenced, will the linter fail?
I think the test sufficiently validates that the CRD exists as-is, so maybe this needs to be _ = ... . If the CRD contents are wrong, then createPodWithGmsa will fail later
There was a problem hiding this comment.
This is referenced in customResourceCleanup, err := createGmsaCustomResource(crdManifestContents) below :)
There was a problem hiding this comment.
oh yeah, missed that, needed to hit ctrl-f again
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PatrickLang, wk8 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
The previously existing e2e GMSA test really only tests a small part of the
whole GMSA set up process, namely that once the API has inlined the GMSA
contents in the pod's spec, and sent that to a worker's kubelet, then the
kubelet passes that down to the runtime.
This new test, in contrast, really tests the whole thing, i.e. deploying the
admission webhook, then deploying a GMSA custom resource, and using that
resource within a pod.
The downside of this test though, is that it does need to make a lot of
assumptions about the cluster it runs against, notably that it runs on a worker
node that's already been joined to a working Active Directory domain (there are
other assumptions, all documented at the beginning of the test file); for that
reason, it is only intended to ever be run against an AKS cluster with the
custom AKS extension from
kubernetes-sigs/windows-testing#98.
Note that this test doesn't aim at testing every edge-case, such as
a pod trying to use a GMSA it doesn't have access to; the webhook has
its own tests for these. This test's goal is to ensure the happy path
doesn't break.
Special notes for your reviewer:
Before this test can actually be run as part of nightly builds, we'll have to make
sure that:
WindowsGMSA=truefeature flag is turned on on both the API and the workers' kubeletsUntil then, this test should be skipped; note that I did it run successfully
on a cluster brought up with AKS-engine with all the modifications outlined
above.
Also, this test assumes that the test itself runs on a Linux box, is that okay?
This is because the webhook deploy script has been written in bash.
Does this PR introduce a user-facing change?: