add e2e tests for banneduser #1045
Conversation
filariow
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
Signed-off-by: Francesco Ilario <[email protected]>
filariow
force-pushed
the
banneduser-middleware
branch
from
d3b02ab
to
625ce24
Compare
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
|
You can only pair this with one of regsvc or host-operator, I see it is failing since all 3 PRs now have the same branch name. |
|
since it's related to proxy test enhancements I think it should go with the regsvc PR right ? |
yes, it should. Would e2e tests run fine even if codeready-toolchain/host-operator#1083 is not merged? I was not able to have them running in my local without this change to host-operator. |
Correct! You need to merge first the host-operator PR since the actual registration-service deployment is the one that comes with host-operator as briefly documented here https://github.com/codeready-toolchain/registration-service/blob/master/deploy/README.md. So yes feel free to merge the host-operator PR once you're ready. |
test/e2e/parallel/proxy_test.go
Outdated
| }) | ||
|
|
||
| t.Run("cannot get workspace", func(t *testing.T) { | ||
| _, err := bannedUser.getWorkspace(t, hostAwait, bannedUser.compliantUsername) |
There was a problem hiding this comment.
Do we really need to use getWorkspace() here? It uses a poll (it's slow because it will keep trying until it's timed out) and hides the actual error. Shouldn't we make sure that everything is ready (spaces, space bindings, etc) and then just use the client directly proxyCl.Get(...) and then check that the error is Forbidden with the expected error message?
There was a problem hiding this comment.
great point, let me fix that
test/e2e/parallel/proxy_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| t.Run("lazy banned user", func(t *testing.T) { |
There was a problem hiding this comment.
What is the difference comparing to banneduser tests?
There was a problem hiding this comment.
the difference is that banneduser's client does not initialize correctly -unsuccessful call to /apis, so we expect it to return NoMatch errors. In this case the client initialized correctly and we have Forbidden errors.
There was a problem hiding this comment.
should we rename it to something like "user creates client and after is banned" ? But more important does it matter when the user is banned , if before or after the initialization of a kube client ?
I'm suggesting that probably we don't need this test. WDYT?
There was a problem hiding this comment.
AFAIK, for how Controller-Runtime's Client v0.13.0 is implemented, the Controller-Runtime's client performs a call to the /apis endpoint during initialization. If we ban the user before creating the client, this call will fail and subsequent requests will fail with NoMatch without really performing the request against the proxy, as the client was not able to initialize the list of resources the cluster is supporting.
Indeed, the client.New() function instantiates a new Dynamic RESTMapper (if not provided via client's Config).
The mapper invokes the /apis endpoint to fetch the list of resources that the cluster is supporting. That's done invoking the setStaticMapper function here.
Next usages of the client will lookup the cached list of supported resources to understand if the cluster supports the provided resource and return a NoMatch error without really executing the request.
Conversely, is we ban the user lately, the client will effectively perform the call and the proxy will reject it with Forbidden.
As the former scenario is mainly testing a Controller-Runtime's Client specific behavior, I'd say we could remove the banneduser suite, and rename the lazy banned user as banneduser. WDYT?
There was a problem hiding this comment.
Thanks for the detailed explanation.
I don't think we need to test the client (initialization). We want to test our proxy here. So I agree with @mfrancisc and would drop the tests which fails with the client initialization.
Create the client. Then ban the user. Then call the proxy and check the expected 403 error.
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
test/e2e/parallel/proxy_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| t.Run("lazy banned user", func(t *testing.T) { |
There was a problem hiding this comment.
should we rename it to something like "user creates client and after is banned" ? But more important does it matter when the user is banned , if before or after the initialization of a kube client ?
I'm suggesting that probably we don't need this test. WDYT?
Signed-off-by: Francesco Ilario <[email protected]>
|
/retest |
test/e2e/parallel/proxy_test.go
Outdated
| }) | ||
|
|
||
| t.Run("cannot list resources", func(t *testing.T) { | ||
| tt := map[string]client.Client{ |
There was a problem hiding this comment.
| tt := map[string]client.Client{ | |
| workspaces := map[string]client.Client{ |
There was a problem hiding this comment.
good idea, let me rename them
test/e2e/parallel/proxy_test.go
Outdated
| } | ||
| for k, wc := range tt { | ||
| t.Run(k, func(t *testing.T) { | ||
| tt := map[string]client.ObjectList{ |
There was a problem hiding this comment.
| tt := map[string]client.ObjectList{ | |
| resourceLists := map[string]client.ObjectList{ |
Signed-off-by: Francesco Ilario <[email protected]>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, filariow, mfrancisc 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 |
Linked to codeready-toolchain/registration-service#455 and codeready-toolchain/host-operator#1079
Signed-off-by: Francesco Ilario [email protected]