common tests framework: cluster client creation could fail with invalid auth #14331
Conversation
chaochn47
force-pushed
the
auth_test_framework_update
branch
from
f749c30
to
e2dffb0
Compare
chaochn47
changed the title
|
Does it look better or I can give another shot to split the PR? @ahrtr |
chaochn47
force-pushed
the
auth_test_framework_update
branch
from
e2dffb0
to
c32c049
Compare
|
mixin test failure is unrelated. There is no monitoring changes. All the comments are addressed. PTAL, thx! @ahrtr |
Codecov Report
@@ Coverage Diff @@
## main #14331 +/- ##
==========================================
- Coverage 75.39% 75.37% -0.02%
==========================================
Files 457 457
Lines 37207 37181 -26
==========================================
- Hits 28053 28026 -27
- Misses 7394 7397 +3
+ Partials 1760 1758 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
chaochn47
force-pushed
the
auth_test_framework_update
branch
from
c32c049
to
b886bbc
Compare
|
This PR is accidentally closed because I pushed to the same commit of main to the remote branch. How to re-open it? |
chaochn47
force-pushed
the
auth_test_framework_update
branch
2 times, most recently
from
c362f7d
to
a29ebe9
Compare
|
This PR is ready to review. Would you mind take a look again? @ahrtr @serathius Thanks!! |
chaochn47
force-pushed
the
auth_test_framework_update
branch
2 times, most recently
from
dc11322
to
2006d13
Compare
chaochn47
requested review from
serathius and
ahrtr
and removed request for
serathius
|
Kindly ping again, do you think upstream still needs this auth test migration? @serathius @ahrtr |
|
For faster review I would recommend removing unrelated changes from the PR. It's over 300 lines of code, but still includes changes that muddy the water. Best if PRs do one thing and only this one thing. |
Makes sense. 300+ lines of code is a burden for PR reviewers. I will try to split into smaller PRs for future bigger PRs going forward. This PR is already a split from original even bigger PR. sigh^ |
| // use integration test client to validate if permissions are authorized | ||
| _, err := integration.NewClient(c.t, clientv3.Config{ | ||
| Endpoints: c.EndpointsV3(), | ||
| DialTimeout: 5 * time.Second, | ||
| DialOptions: []grpc.DialOption{grpc.WithBlock()}, | ||
| Username: cfg.Username, | ||
| Password: cfg.Password, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Probably we do not need this. Reasons:
- It seems not good for e2e test to depends on integration test;
- The related test cases will get a denied response if the auth info isn't correct. So the pre-check isn't necessary.
There was a problem hiding this comment.
I agree it's not optimal that e2e test depending on integration test. This is an exception when auth config is not empty.
Copying from past conversation between @serathius and me.
From me
Some existing auth tests checks unauthenticated errors.
Integration test will throw an error at client creation stage with
clientv3.NewLine 439 in 7ec336f
However, e2e test won't fail at client creation stage and fail at "got response" stage instead.
Also every request from e2e test will establish a new grpc connection to the server but integration test reuses existing one. In order to use a different user name and password, integration test needs to create another client.
From Marek
I think we can create
clientv3.Clientjust for this.
ref. #14041 (comment)
Does this make sense to you?
There was a problem hiding this comment.
Thanks for the clarification.
I don't think e2e and integration test must follow the same pattern or behavior. Specifically, integration test will get an error when creating the client, while e2e test will not get an error until the submitting request stage when the auth config is invalid. It isn't necessary to change the behavior for e2e test. e2e test is just simulating the real users' use case, in which there is no explicit creating client stage from user perspective, and users just get an error response after submitting the request.
It isn't a big deal, so I won't insist on this so that we do not get blocked here.
There was a problem hiding this comment.
It seems not good for e2e test to depends on integration test;
It's not tests, it's e2e tests using integration client, which is just a wrapper for clientv3 library. There is nothing wrong with e2e tests using client library for some things that are not surfaced in etcdctl. To complete migration of tests to new framework this is even required as Some tests cannot be expressed using just e2e or integration tools
I don't think e2e and integration test must follow the same pattern or behavior
But it would make testing and reasoning about them much simpler.
| @@ -30,7 +30,7 @@ type testRunner interface { | |||
|
|
|||
| type Cluster interface { | |||
| Members() []Member | |||
| Client() Client | |||
| Client(cfg clientv3.AuthConfig) (Client, error) | |||
There was a problem hiding this comment.
I am thinking whether it's possible to get rid of the parameter cfg clientv3.AuthConfig. When the test cases setup the user & role, and enable the auth, then the client should record the authConfig automatically.
Does this make sense?
There was a problem hiding this comment.
I don't think client should be stateful. In general, it makes the client not concurrent safe. If multiple callers set up the user & role and enable the auth using the client, this would cause contention.
There was a problem hiding this comment.
ack.
It's just a little strange to provide a specific cfg clientv3.AuthConfig parameter, but automatically get all other parameters from the cluster. Probably it makes more sense to resolve this using a generic way, such as change the parameter to something like opts ...Option. It might be a little over engineering from another perspective.
Let's keep it as it's for now, and probably think about it separately.
|
Please also rebase this PR although there is no conflict. |
chaochn47
force-pushed
the
auth_test_framework_update
branch
from
2006d13
to
0cf4ad5
Compare
Signed-off-by: Chao Chen <chaochn@amazon.com>
chaochn47
force-pushed
the
auth_test_framework_update
branch
from
0cf4ad5
to
8d057ea
Compare
common tests framework: cluster client creation with auth could fail
related to #14041
This PR has 227 additions and 73 deletions.
Signed-off-by: Chao Chen chaochn@amazon.com
In order to fail the test cluster client creation with invalid auth, I have to change the interface function signature.