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

Fix cassandra schema scripts to be able to run from a remote node #4056

Conversation

aoterolorenzo
Copy link

@aoterolorenzo aoterolorenzo commented Nov 18, 2022

Signed-off-by: Alberto Otero Lorenzo [email protected]

Which problem is this PR solving?

The create.sh file from the cassandra-schema migration scripts for Jaeger, executes an 'a priori' inoffensive
command cqlsh -e "show version". This command shouldn't need of authentication, and it doesn't... But only when the target is the local cassandra node ($CQLSH_HOST points to localhost). Otherwise, the CLI tool will report an error asking for auth when running the scrips over a remote node:

Generating the schema for the keyspace jaeger_v1_dc1 and datacenter dc1
Connection error: ('Unable to connect to any servers', {'10.101.195.23:9042': AuthenticationFailed('Remote end requires authentication')})
Using template file /cassandra-schema/v004.cql.tmpl with parameters:
    mode = test
    datacenter = dc1
    keyspace = jaeger_v1_dc1
    replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}
    trace_ttl = 172800
    dependencies_ttl = 0

Short description of the changes

An if is added to the logic for create.sh's command cqlsh -e "show version" to be executed with authentication arguments in case a password is set. This doesn't check if the node where the CQL is executed is remote or not, but instead it provides auth in case it is provided, local or remote, covering all use cases.

| awk -F "." '{print $1}' \
)
else
cas_version=$(cqlsh -u ${USER} -p ${PASSWORD} -e "show version" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to pass credentials? Passing via cmd line arguments is not secure, they can be observed via ps.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid not. Seems only $CQLSH_HOST and $CQLSH_PORT can be provided as env vars, and the authentication can only be performed through CLI args or plain config file (see ref).

In fact, there's already calls of this kind on the parent file docker.sh:

if [ -z "$PASSWORD" ]; then
  ${CQLSH} ${CQLSH_SSL} ${CQLSH_HOST} ${CQLSH_PORT} -e "describe keyspaces"
else
  ${CQLSH} ${CQLSH_SSL} ${CQLSH_HOST} ${CQLSH_PORT} -u ${USER} -p ${PASSWORD} -e "describe keyspaces"
if [ -z "$PASSWORD" ]; then
  MODE="${MODE}" DATACENTER="${DATACENTER}" KEYSPACE="${KEYSPACE}" /cassandra-schema/create.sh "${TEMPLATE}" | ${CQLSH} ${CQLSH_SSL} ${CQLSH_HOST} ${CQLSH_PORT}
else
  MODE="${MODE}" DATACENTER="${DATACENTER}" KEYSPACE="${KEYSPACE}" /cassandra-schema/create.sh "${TEMPLATE}" | ${CQLSH} ${CQLSH_SSL} ${CQLSH_HOST} ${CQLSH_PORT} -u ${USER} -p ${PASSWORD}
fi

Is simply that cqlsh -e "show version" is so simple and straight forward that you don't think about a need of authentication to use it, so it's not until you run it from a remote machine when you realize of this (which indeed makes complete sense, to need auth for executing ANY command or query over a remote end).

Copy link
Member

@yurishkuro yurishkuro Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointers. Indeed, this shell script was supposed to be used only to generate the schema file, not to do any execution against the server.

What if we remove version check altogether and move it to docker.sh? This script can simply accept another parameter via env (default to 4).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the version check is only necessary if template is not provided.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the moment this would keep the current script behavior solving the problem and keeping consistency with the rest of the code, so I would accept it as a fix. Of course a proper refactor would be welcome if you guys find the time 🙂

@aoterolorenzo
Copy link
Author

Hi there! Any update on this?

@yurishkuro
Copy link
Member

yurishkuro commented Dec 7, 2022

I have alternative fix in #4087

yurishkuro added a commit that referenced this pull request Dec 7, 2022
Alternative solution, closes #4056

This keeps the create.sh script simpler and not aware of cqlsh.

Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Albert <[email protected]>
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this pull request Feb 22, 2023
Alternative solution, closes jaegertracing#4056

This keeps the create.sh script simpler and not aware of cqlsh.

Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Albert <[email protected]>
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this pull request Mar 5, 2023
Alternative solution, closes jaegertracing#4056

This keeps the create.sh script simpler and not aware of cqlsh.

Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Albert <[email protected]>
Signed-off-by: shubbham1215 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants