Default user positive and negative test for deploying

[castelblanque]

Description of the change

This PR adds a positive test-case to use the more limited user kubeapps-user (deploy & delete).
It also adds a new negative test for that user trying to deploy from a repo with secrets to which user does not have access.

Benefits

We are better covering the "limited" user deploy/undeploy actions in CI.

Possible drawbacks

N/A

Applicable issues

• fixes CI does not test install with a default user #4081

[absoludity]

Love the new output with playwright. Especially marking flaky tests etc.

[absoludity]

FWIW, I wasn't thinking that we needed a new test for this, but rather just updating the existing default deployment test to use the kubeapps-user... we know if it passes with that that it will have no issues to pass with a higher-privileged account. Adding redundant tests will just lengthen the test run, won't it? Or is there a reason you think we should test both with the kubeapps-user as well as with kubeapps-operator?

[castelblanque]

In the new 09 test, repository is created by admin user and consumed by another user with less privileges. I don't think we are covering that in other tests? I mean, in 03 test, admin does everything (add repo, deploy, etc.) and in test 04 admin deploys to default namespace using an initial repo from Kubeapps installation.
I think each test covers a slightly different scenario, it's a bit of the "how far do we want to get with e2e tests?" question I would say.
However, happy to remove any test if you think that is not needed!

[absoludity]

OK - I hadn't realised it was adding a repo. I'm OK with having a separate test, but it's not clear to me why we'd add a repo as part of this test (given we already have a repo with charts we can use for this test, and have other tests that add repositories). Also, I'm not sure about adding a non-bitnami repo so that our CI is now dependent on the prometheus community helm charts?

[absoludity]

@absoludity Update: removing the rolebinding deletion from .circleci/config.yml works as expected.

Great, though it might be worth checking with @antgamdia - looks like he added the deletion. I can only guess that the intent was to use the default namespace for the kubeapps user and so extra RBAC for other namespaces was removed, at the time.

[antgamdia]

That's right, though I can't 100% remember, the idea was just using the default namespace (perhaps because of the problems you mentioned about the test selecting the wrong context?), so I just deleted this extra RBAC (which was granting edit ClusterRole in the kubeapps-user-namespace namespace

@antgamdia Do you know if putting back the kubeapps-user role binding for the view user could bring any issues?
Tests are passing successfully.

@castelblanque Now the tests are handling better how to switch contexts, +1 to remove any deletion that might veil any errors on the current rbac dev file we're using.

[absoludity]

From the errors, it looks like the kubeaps-user account can't read secrets in the kubeapps-user-namespace. I think what's happened is that previously these specific tests were done with token auth (not OIDC) and so were using the view and/or edit token which has view/edit access in the default namespace. To use the OIDC user (which is better, excellent), we'll need to add similar rolebindings (in the e2e-test.sh) for the oidc:kubeapps-user@example.com user.

[absoludity]

In the dev env, this is done with https://github.com/kubeapps/kubeapps/blob/main/docs/user/manifests/kubeapps-local-dev-users-rbac.yaml#L30-L52

[castelblanque]

To use the OIDC user (which is better, excellent), we'll need to add similar rolebindings (in the e2e-test.sh) for the oidc:kubeapps-user@example.com user.

I noticed that yesterday, wanted to give it a try first with the user "as-is" so that I'm sure I don't break up something else. For some reason the kubeapps-user rolebinding is applied first...and deleted in this line.

I think what's happened is that previously these specific tests were done with token auth (not OIDC)

That explains the deletion, thanks!
Now I'll try removing the deletion of the rolebinding.

[castelblanque]

@absoludity Update: removing the rolebinding deletion from .circleci/config.yml works as expected.

[absoludity]

Just check with Antonio about the deletion, in case there's a reason we don't know.

Also, I don't think we should make our CI test 09 dependent on the prometheus community helm chart repo? Can we consider either not including adding a repo in the 09 test (another test already tests that), or adding the bitnami repo with a different name and navigating to that as the other user, if it's necessary?

[castelblanque]

Also, I don't think we should make our CI test 09 dependent on the prometheus community helm chart repo? Can we consider either not including adding a repo in the 09 test (another test already tests that), or adding the bitnami repo with a different name and navigating to that as the other user, if it's necessary?

The repo added in test 09 is a public one, whereas the chartmuseum we are using in other tests has user/pwd. That allows to test both scenarios.
As I saw we are using a repo from Gitlab in another test, I thought it could be ok. But Gitlab's repo didn't allow to deploy correctly (it was failing due to needing to create statefulsets, secrets, etc.).
I wanted to avoid the Bitnami repo so that we don't overload the CI environment + the UI. Speed of the test clicking through the UI is a bit variable and sometimes it gets to the catalog before packages are there, or performing a search gets stuck.
We can look at alternatives like adding Bitnami repo with filtering, or any other known small VMWare repo (if any)? Another posibilty is to add an additional, public instance of chartmuseum.

[castelblanque]

Just check with Antonio about the deletion, in case there's a reason we don't know.

@antgamdia Do you know if putting back the kubeapps-user role binding for the view user could bring any issues?
Tests are passing successfully.

[absoludity]

Also, I don't think we should make our CI test 09 dependent on the prometheus community helm chart repo? Can we consider either not including adding a repo in the 09 test (another test already tests that), or adding the bitnami repo with a different name and navigating to that as the other user, if it's necessary?

The repo added in test 09 is a public one, whereas the chartmuseum we are using in other tests has user/pwd. That allows to test both scenarios.

The test is to ensure that a "Regular user can deploy and delete packages in its own namespace from global repo without secrets", which I think is a scenario we could have done with the default deployment (already using the public bitnami repo). Ah, or maybe you mean testing the adding of both a public and private repo (we currently test deploying from both, but didn't need to add a public one, since bitnami was there already). Not sure it's necessary, but yep, that's a valid reason, but different to the stated intent of the test. Fine either way. (EDIT: I'd missed that we currently don't actually add the normal bitnami repository at all, I'd thought you'd meant earlier that we shouldn't add it again, but see now you may have meant we shouldn't add it at all).

As I saw we are using a repo from Gitlab in another test, I thought it could be ok. But Gitlab's repo didn't allow to deploy correctly (it was failing due to needing to create statefulsets, secrets, etc.). I wanted to avoid the Bitnami repo so that we don't overload the CI environment + the UI. Speed of the test clicking through the UI is a bit variable and sometimes it gets to the catalog before packages are there, or performing a search gets stuck. We can look at alternatives like adding Bitnami repo with filtering, or any other known small VMWare repo (if any)? Another posibilty is to add an additional, public instance of chartmuseum.

I'd generally avoid as many external dependencies as possible to reduce CI issues unrelated to actual tests. So my preference would be for the latter if we really wanted to. Or simply generate and serve a trivial index.yaml of our own with one chart (using whatever the latest bitnami apache chart is or similar). As it is, I think our CI is now dependent on the external prometheus chart deploying successfully? Or just update the 04-default-deployment to use the kubeapps-user user (and yes, we wouldn't be testing adding a public repo in e2e tests, just adding a private one, which is OK imo). Up to you - I don't feel strongly, just don't understand why we would choose to add this dependency on both an external helm repo and chart.

[castelblanque]

we currently don't actually add the normal bitnami repository at all

Yeah, it was like that previously, so I preferred not to add it. Public Gitlab repo was being added instead. See my proposal below.

I'd generally avoid as many external dependencies as possible to reduce CI issues unrelated to actual tests.

Totally agree, but the priority was to unblock CI. There are many tasks to be done in order to tidy up CI, let's gather proposals in #4096.
In order to be independent from external sources (even Bitnami) we should have IMHO:

• Chartmuseum instance (or similar) with no credentials (we already have the one with creds)
• Images registry serving repos with and without credentials (in GKE we might be able to use Google's)

Not rocket science, just takes some time.