Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
CASSANDRA-19366 trunk - Expose auth mode in system_views.clients, clientstats, metrics #3085
CASSANDRA-19366 trunk - Expose auth mode in system_views.clients, clientstats, metrics #3085
Changes from 5 commits
cf89185
605d9cb
f359c39
0d2b7b3
3ef7191
b6b7886
3a869c3
0d1853b
25bfea3
248ba2e
465109e
e17c35f
8f082a1
cb9439e
ea6e2fd
e1e3b2d
c48f792
f75d7b3
0ef40b3
dddae45
54da742
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we spell out the label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would break JMX as I am not sure how a metric with a space in its name would behave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably check that the display name does not contain space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I'm not sure what's the behavior for jmx. But if spaces break JMX, we should add some guards around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll test whether spaces work w/ JMX, I suspect it does, but isn't desirable as might require clients to escape; If it doesn't work, I suggest we throw a RuntimeException in the
AuthenticationMode
constructor.As far as the name, regardless of whether spaces are allowed, how about
MutualTls
(for consistency with how the authenticator is named)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it does work:
But it isn't easy to use, and I imagine will give some tooling trouble. For it to work with jmxterm for example I had to double
\
escape the space:I think we should document that spaces in the mode name aren't preferred as it may not work well with some tooling. I will do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 8f082a1 and renamed
Mtls
->MutualTls
. I also realized was still usingString
inClientMetrics
instead ofAuthenticationMode
so I updated that as well, hopefully that looks good!