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

tests/e2e: Add auth registry libvirt tests #1932

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

tests/e2e: Add auth registry libvirt tests #1932

wants to merge 3 commits into from

Conversation

stevenhorsman
Copy link
Member

Run the authenticated regsitry tests for libvirt cloud provider

Depends on #1931

@stevenhorsman stevenhorsman force-pushed the libvirt-auth-e2e-tests branch 6 times, most recently from 093e700 to 72da0f7 Compare July 25, 2024 15:55
@stevenhorsman stevenhorsman added test_e2e_libvirt Run Libvirt e2e tests and removed hold labels Jul 25, 2024
@stevenhorsman
Copy link
Member Author

stevenhorsman commented Jul 25, 2024 •
edited
Loading

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.

@stevenhorsman
Copy link
Member Author

stevenhorsman commented Jul 25, 2024 •
edited
Loading

The tests are all failing for libvirt anyway:

=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithValidCredentials
=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test
    assessment_runner.go:261: timed out waiting for the condition
--- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithValidCredentials (600.12s)
    --- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test (600.12s)
=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithInvalidCredentials
=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithInvalidCredentials/InvalidAuthImagePeerPod_test
    assessment_runner.go:201: secrets "auth-json-secret-default" already exists
--- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithInvalidCredentials (0.04s)
    --- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithInvalidCredentials/InvalidAuthImagePeerPod_test (0.04s)
=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithoutCredentials
=== RUN   TestLibvirtCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test
    assessment_runner.go:201: secrets "auth-json-secret-default" already exists
--- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithoutCredentials (0.02s)
    --- FAIL: TestLibvirtCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test (0.02s)

@stevenhorsman stevenhorsman force-pushed the libvirt-auth-e2e-tests branch 4 times, most recently from 49fe28e to 7638f97 Compare August 9, 2024 11:10
@stevenhorsman stevenhorsman force-pushed the libvirt-auth-e2e-tests branch 5 times, most recently from 8d48800 to 8707615 Compare August 12, 2024 17:10
@stevenhorsman
Copy link
Member Author

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:

=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials
=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test
time="2024-08-12T10:15:00-07:00" level=info msg="ServiceAccount default updated successfully."
    assessment_runner.go:245: Deleting pre-existing auth-json-secret-default...
    assessment_runner.go:249: Creating empty auth-json-secret-default...
=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test/Peer_pod_with_Authenticated_Image_without_Credentials_has_been_created
    assessment_runner.go:379: podEvent: &{EventType:Failed EventDescription:Failed to pull image "quay.io/kata-containers/confidential-containers-auth:test": failed to pull and unpack image "quay.io/kata-containers/confidential-containers-auth:test": failed to resolve reference "quay.io/kata-containers/confidential-containers-auth:test": unexpected status from HEAD request to https://quay.io/v2/kata-containers/confidential-containers-auth/manifests/test: 401 UNAUTHORIZED EventReason:}
    assessment_runner.go:381: Output Log from Pod: &{Failed Failed to pull image "quay.io/kata-containers/confidential-containers-auth:test": failed to pull and unpack image "quay.io/kata-containers/confidential-containers-auth:test": failed to resolve reference "quay.io/kata-containers/confidential-containers-auth:test": unexpected status from HEAD request to https://quay.io/v2/kata-containers/confidential-containers-auth/manifests/test: 401 UNAUTHORIZED }
=== NAME  TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test
    assessment_runner.go:617: Deleting pod authenticated-image-without-creds-4104531429-pod...
    assessment_runner.go:624: Pod authenticated-image-without-creds-4104531429-pod has been successfully deleted within 60s
--- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials (10.12s)
    --- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test (10.12s)
        --- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithoutCredentials/InvalidAuthImagePeerPod_test/Peer_pod_with_Authenticated_Image_without_Credentials_has_been_created (0.01s)
=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials
=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test
    assessment_runner.go:264: Waiting for containers in pod: authenticated-image-with-creds-1450112107-pod are ready
=== RUN   TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test/Peer_pod_with_Authenticated_Image_with_Valid_Credentials(Default_service_account)_has_been_created
=== NAME  TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test
    assessment_runner.go:617: Deleting pod authenticated-image-with-creds-1450112107-pod...
    assessment_runner.go:624: Pod authenticated-image-with-creds-1450112107-pod has been successfully deleted within 60s
--- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials (20.12s)
    --- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test (20.12s)
        --- PASS: TestDockerCreatePeerPodWithAuthenticatedImageWithValidCredentials/ValidAuthImagePeerPod_test/Peer_pod_with_Authenticated_Image_with_Valid_Credentials(Default_service_account)_has_been_created (0.04s)
PASS

@stevenhorsman stevenhorsman marked this pull request as ready for review August 13, 2024 08:31
wainersm
wainersm reviewed Sep 16, 2024
View reviewed changes
src/cloud-api-adaptor/test/e2e/common_suite.go
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()
Copy link
Member

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).

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

ok, got it. thx!

wainersm
wainersm reviewed Sep 16, 2024
View reviewed changes
src/cloud-api-adaptor/test/e2e/common_suite.go
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()
Copy link
Member

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!

Copy link
Member Author

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.

@wainersm wainersm added test_e2e_libvirt Run Libvirt e2e tests and removed test_e2e_libvirt Run Libvirt e2e tests labels Sep 16, 2024
@stevenhorsman
tests/e2e: Various authentication related refactors
36a8c06 
- 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]>
@stevenhorsman
tests/e2e: Add auth registry libvirt tests
6d9a8eb 
Run the authenticated regsitry tests for libvirt cloud provider

Signed-off-by: stevenhorsman <[email protected]>
@stevenhorsman
tests/e2e: Add auth registry docker tests
b1b0b5f 
Run the authenticated regsitry tests for docker cloud provider

Signed-off-by: stevenhorsman <[email protected]>
wainersm
wainersm approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@wainersm wainersm left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wainersm wainersm wainersm approved these changes

@mkulke mkulke Awaiting requested review from mkulke

@bpradipt bpradipt Awaiting requested review from bpradipt

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stevenhorsman @wainersm