Skip to content
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

[Feature]: Add support of custom authenticators for jaeger cassandra storage #5627

Closed
quantumlexa opened this issue Jun 14, 2024 · 12 comments
Closed
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@quantumlexa
Copy link

quantumlexa commented Jun 14, 2024

Requirement

as a Jaeger power user I want to use cassandra that uses custom authenticator as a storage for jaeger traces

Problem

Jarger uses BasicAuthenticator from config with no AllowedAuthenticators set. That makes gocql to use default set of Authenticators.

https://github.com/jaegertracing/jaeger/blob/main/pkg/cassandra/config/config.go#L144-L149
As the result all cassandra instances that uses custom Authenticators can't be used as a storage for jaeger traces
Here by custom authenticators I mean not included in the following list: https://github.com/gocql/gocql/blob/master/conn.go#L26-L39

Proposal

Add ability to set authenticator by an user.

Open questions

No response

@yurishkuro
Copy link
Member

What does it mean to allow those custom authenticators from the list you mention? If it's just an option we need to pass to gocql driver, then it's reasonable to support. If it requires integrating with another dependency, then it gives me a pause. E.g. we intentionally stayed away from integrating with AWS AIM because it requires taking dependency on their custom SDKs (and increases the binary size like crazy, although that ship may have sailed now since OTEL collector wasn't as diligent).

@quantumlexa
Copy link
Author

What does it mean to allow those custom authenticators from the list you mention? If it's just an option we need to pass to gocql driver, then it's reasonable to support.

Yes, that is exactly what I want in this issue: just to pass another option to gocql driver

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Jun 14, 2024
@ayushrakesh
Copy link

Hey @quantumlexa I want to work on this issue. Please assign to me.

@yurishkuro
Copy link
Member

As the result all cassandra instances that uses custom Authenticators can't be used as a storage for jaeger traces
Here by custom authenticators I mean the following list: https://github.com/gocql/gocql/blob/master/conn.go#L26-L39

@quantumlexa why do you refer to these as "custom"? I think it's creating confusion, we have two PRs going about this ticket in completely different directions: #5628 and #5637. When I hear "custom" I think more of the approach in #5637 where we implement an authenticator API. Whereas #5628 simply adds a CLI flag to pass on of those "approved" authenticators from the list. The documentation in gocql is lacking on this front.

@hellspawn679
Copy link
Contributor

hellspawn679 commented Jun 17, 2024

@quantumlexa why do you refer to these as "custom"? I think it's creating confusion, we have two PRs going about this ticket in completely different directions: #5628 and #5637. When I hear "custom" I think more of the approach in #5637 where we implement an authenticator API. Whereas #5628 simply adds a CLI flag to pass on of those "approved" authenticators from the list. The documentation in gocql is lacking on this front.

@yurishkuro I think the use of passing "approved" authenticators let's say you are using instacluster cassendra db so now they have there own custom auth for that so to connect to that you can either specify the auth you are allowing

cluster.Authenticator = gocql.PasswordAuthenticator{Username: "Username", Password: "Password", AllowedAuthenticators: []string {"com.instaclustr.cassandra.auth.InstaclustrPasswordAuthenticator"}}

it would still work if you don't pass any allowed auth

cluster.Authenticator = gocql.PasswordAuthenticator{Username: "Username", Password: "Password"}

as you can see the func approve
https://github.com/gocql/gocql/blob/master/conn.go#L26-L52
if no allowed auth is passed it will try all the defaultApprovedAuthenticators one by one in most case it will use the default auth
org.apache.cassandra.auth.PasswordAuthenticator
but when you are using cassendra in scylladb(discussed in this issue) or some cloud provider AWS(Helenus Authenticator), instacluster which have there own auth you can specify the exact auth you want to use for that it would be fine even if you don't pass the allowed auth

@quantumlexa
Copy link
Author

As the result all cassandra instances that uses custom Authenticators can't be used as a storage for jaeger traces
Here by custom authenticators I mean the following list: https://github.com/gocql/gocql/blob/master/conn.go#L26-L39

@quantumlexa why do you refer to these as "custom"? I think it's creating confusion, we have two PRs going about this ticket in completely different directions: #5628 and #5637. When I hear "custom" I think more of the approach in #5637 where we implement an authenticator API. Whereas #5628 simply adds a CLI flag to pass on of those "approved" authenticators from the list. The documentation in gocql is lacking on this front.

@yurishkuro I'm sorry for such a badly formatted issue description.
By custom authenticators I mean not included in the following list: https://github.com/gocql/gocql/blob/master/conn.go#L26-L39

Looking at #5628 it seems that it should solve this issue

@yurishkuro
Copy link
Member

By custom authenticators I mean not included in the following list: <>
Looking at #5628 it seems that it should solve this issue

are you sure? According to #5627 (comment), the default list is used regardless of whether we specify them or not. Even if that conclusion is wrong, the PR does nothing to enable authenticators that are NOT on the list.

@hellspawn679
Copy link
Contributor

hellspawn679 commented Jun 17, 2024

By custom authenticators I mean not included in the following list: <>
Looking at #5628 it seems that it should solve this issue

are you sure? According to #5627 (comment), the default list is used regardless of whether we specify them or not. Even if that conclusion is wrong, the PR does nothing to enable authenticators that are NOT on the list.

no if you specify the default list won't be used it will only try among the provided auth
https://github.com/gocql/gocql/blob/master/conn.go#L43-L43

@yurishkuro
Copy link
Member

@hellspawn679 our current code leaves this field empty, which causes the default list to be used https://github.com/gocql/gocql/blob/34fdeebefcbf183ed7f916f931aa0586fdaa1b40/conn.go#L44

So if someone wants to use one of these approved authenticators, no changes is needed in Jaeger. Only if someone wants to prohibit using the full list would we need a user-configurable option to allow user to provide the exact list, OR to provide some name that is not even on the list. I think your PR is still on point, but given the utter lack of documentation in gocql about that I want to (a) understand it myself and (b) write adequate CLI flag description that is unambiguous.

@hellspawn679
Copy link
Contributor

@yurishkuro yes

yurishkuro pushed a commit that referenced this issue Jun 18, 2024
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## Which problem is this PR solving?
- #5627

## Description of the changes
- added defaultApprovedAuthenticators

## How was this change tested?
- 

## Checklist
- [ ] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [ ] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: mehul gautam <mehulsharma4786@gmail.com>
@quantumlexa
Copy link
Author

quantumlexa commented Jun 18, 2024

are you sure? According to #5627 (comment), the default list is used regardless of whether we specify them or not. Even if that conclusion is wrong, the PR does nothing to enable authenticators that are NOT on the list.

@yurishkuro
yes, I just doublechecked the implementation of #5628 and all further comments in this PR, that looks good to me.
Thanks!

@ayushrakesh
Copy link

@yurishkuro @quantumlexa, I understood the problem it in a wrong way, I have read the whole description above why I am wrong. By the way thanks for it. Looking forward to contribute more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants