-
Notifications
You must be signed in to change notification settings - Fork 86
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.
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
tests/e2e: Add auth registry libvirt tests #1932
tests/e2e: Add auth registry libvirt tests #1932
Conversation
e93d9cb
to
7dc671a
Compare
093e700
to
72da0f7
Compare
There is some value in this PR in seeing if authenticated image pull is working at all, but I think there are issues with the invalid credentials test. In the course of trying to fix it up I think the case is that for this test, we pass through the correct pull secret into the peer pod (I think to make this incorrect we'd need to modify the kustomize/re-create the caa, so that might not even be worth testing?) and just give it an invalid imagePullSecret, which just checks that it errors on the host pull, not the guest and is the opposite way around to want we are trying to test, so I think there is still a bunch more to do along with a bunch of refactoring I've been working on. It might be that we just drop that test entirely and keep the valid creds works and no guest creds (but still correct image pull test) fails, if that even is sensible. |
The tests are all failing for libvirt anyway:
|
49fe28e
to
7638f97
Compare
8d48800
to
8707615
Compare
The libvirt provider should be tested in this PR, we don't have a docker provider e2e run yet, so the manual test results is:
|
8707615
to
e76838c
Compare
e76838c
to
e80b61f
Compare
imageName := os.Getenv("AUTHENTICATED_REGISTRY_IMAGE") | ||
pod := NewPod(E2eNamespace, podName, podName, imageName, WithRestartPolicy(v1.RestartPolicyNever), WithImagePullSecrets(secretName)) | ||
NewTestCase(t, e, "InvalidAuthImagePeerPod", assert, "Peer pod with Authenticated Image with Invalid Credentials has been created").WithSecret(secret).WithPod(pod).WithAuthenticatedImage().WithAuthImageStatus(expectedAuthStatus).WithCustomPodState(v1.PodPending).Run() | ||
NewTestCase(t, e, "ValidAuthImagePeerPod", assert, "Peer pod with Authenticated Image with Valid Credentials(Default service account) has been created").WithPod(pod).WithCustomPodState(v1.PodRunning).Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stevenhorsman !
I'm curious why originally it was checking for v1.PodPending
but now v1.PodRunning
(which seems the correct indeed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall the logic as I think I re-wrote it 2 months ago, but I think I ended up surprised that some of these tests ever passed. We did only run them for the ibm cloud provider, which has not had well supported tests for a long long time, so it might have been that if you got lucky with pull timing then pending was enough, but I was trying to make them more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, got it. thx!
imageName := os.Getenv("AUTHENTICATED_REGISTRY_IMAGE") | ||
pod := NewPod(E2eNamespace, podName, podName, imageName, WithRestartPolicy(v1.RestartPolicyNever)) | ||
NewTestCase(t, e, "InvalidAuthImagePeerPod", assert, "Peer pod with Authenticated Image without Credentials has been created").WithPod(pod).WithAuthenticatedImage().WithAuthImageStatus(expectedAuthStatus).WithCustomPodState(v1.PodPending).Run() | ||
expectedErrorString := "401 UNAUTHORIZED" | ||
NewTestCase(t, e, "InvalidAuthImagePeerPod", assert, "Peer pod with Authenticated Image without Credentials has been created").WithPod(pod).WithNoAuthJson().WithExpectedPodDescribe(expectedErrorString).WithCustomPodState(v1.PodPending).Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generalization to allow running tests WithExpectedPodDescribe(expectedErrorString)
is amazing! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the framework has lots of building blocks, which can be helpful, but many of them are quite specific, which just makes it a struggle to understand the flow and which path you are going down (for me at least), so I wanted to simplify and make some more basic ones that mimic more how I test things myself.
e80b61f
to
b1b0b5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Contain wanted refactor but CI doesn't like us recently
- Add new expectedPodDescribe check for general purpose pod describe message checking - Remove `GetAuthenticatedImageStatus` - Use the pod describe message as a better way to check for general errors, rather than bespoke auth image approach - Remove InvalidCredentials test as it's currently just using invalid credentials on the host side, not for the guest pull, so isn't useful and this feature is tested in kata-containers for bare-metal - Refactor auth.json file and auth-json-secret creation to be common and sharable by all cloud providers Signed-off-by: stevenhorsman <[email protected]>
Run the authenticated regsitry tests for libvirt cloud provider Signed-off-by: stevenhorsman <[email protected]>
Run the authenticated regsitry tests for docker cloud provider Signed-off-by: stevenhorsman <[email protected]>
b1b0b5f
to
58a9186
Compare
Hi @stevenhorsman ! My ack stands! I will find someone else to give the additional approval. |
Thanks - it's nice to have some green tests! |
Run the authenticated regsitry tests for libvirt cloud provider
Depends on #1931