RFC: etcdserver: swap priority of cert CN and username + password #8594
Conversation
|
we probably should add a test? |
|
@xiang90 sure, I'll add the test later. How do you think about the option? I'm feeling that just mentioning on release not would be enough, but not sure yet. |
|
@mmerrill3 imo a flag is an overkill. What's your take? Please read #8584 |
|
@raoofm, yeah, its more natural to use the presented username if it is there. |
|
If we have not documented the previous behavior explicitly, I think we should just fix this and start to document it. This behavior is more natural, and the previous behavior seems buggy. |
|
@mitake can you add a test and let us get this merged. |
|
@xiang90 added a commit for the test case, could you take a look? The second commit also fixes a silly bug in the existing |
|
lgtm. /cc @xiang90 |
| @@ -960,3 +964,55 @@ func authTestEndpointHealth(cx ctlCtx) { | |||
| cx.t.Fatalf("endpointStatusTest ctlV3EndpointHealth error (%v)", err) | |||
| } | |||
| } | |||
|
|
|||
| func authTestCertCNAndUsername(cx ctlCtx) { | |||
There was a problem hiding this comment.
we should not use t.fatal all over the place.
fatal should be used for the pre-condition check, errorf should be used for the actual correctness checking.
There was a problem hiding this comment.
I see, thanks for pointing out. I'll update soon.
|
LGTM after fixing the test nits. |
mitake
force-pushed
the
auth-priority
branch
from
fede8ee
to
f815d9a
Compare
|
@xiang90 updated for the test nits, could you take a look? |
|
lgtm |
/cc @raoofm
I'd like to hear comments about we should add an option for controlling the priority or not.
Fix #8584