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.

Sign up for GitHub

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

Jump to bottom

fix(cluster helper func): double quoting in keyspace names #10027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(cluster helper func): double quoting in keyspace names #10027

wants to merge 1 commit into from

Conversation

timtimb0t
Copy link
Contributor

@timtimb0t timtimb0t commented Feb 10, 2025 •
edited
Loading

When keyspace names start with a digit, they are returned with surrounding double quotes. This can lead to double quoting in CQL statements that can lead to SCT failure. This commit updates the get_test_keyspaces func to strip these extra quotes. If keyspace isnt quoted the change is harmless

refer: https://argus.scylladb.com/tests/scylla-cluster-tests/553b0e31-6b23-4f9b-9953-05fde9c09791

Testing

https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/eugene_test_folder/job/qutes/

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

@timtimb0t timtimb0t requested a review from fruch February 10, 2025 10:54
@timtimb0t timtimb0t marked this pull request as ready for review February 10, 2025 11:30
@temichus temichus added the backport/none Backport is not required label Feb 16, 2025
temichus
temichus previously approved these changes Feb 16, 2025
View reviewed changes
fruch
fruch reviewed Feb 16, 2025
View reviewed changes
sdcm/cluster.py Outdated
@@ -4491,7 +4491,7 @@ def get_test_keyspaces(self, db_node=None):
"""Function returning a list of non-system keyspaces (created by test)"""
db_node = db_node or self.nodes[0]
keyspaces = db_node.run_cqlsh("describe keyspaces").stdout.split()
return [ks for ks in keyspaces if not is_system_keyspace(ks)]
return [ks.strip('"') for ks in keyspaces if not is_system_keyspace(ks)]
Copy link
Contributor

Choose a reason for hiding this comment

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

you still need to make sure all the usages are using cql_quote_if_needed as needed when using it in CQL statements

maybe it's time to have cql_unquote_if_needed ? so it would be a bit more clear ?

@soyacz soyacz mentioned this pull request Feb 20, 2025
Open
2 tasks
soyacz
soyacz reviewed Feb 28, 2025
View reviewed changes
Copy link
Contributor

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

I verified places where it is used - and I think some places need to be fixed - see comments.

sdcm/cluster.py Outdated
@@ -4491,7 +4491,7 @@ def get_test_keyspaces(self, db_node=None):
"""Function returning a list of non-system keyspaces (created by test)"""
db_node = db_node or self.nodes[0]
keyspaces = db_node.run_cqlsh("describe keyspaces").stdout.split()
return [ks for ks in keyspaces if not is_system_keyspace(ks)]
return [ks.strip('"') for ks in keyspaces if not is_system_keyspace(ks)]
Copy link
Contributor

Choose a reason for hiding this comment

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

we do it also in nemesis.py in line 2922, could be removed from there

Copy link
Contributor

Choose a reason for hiding this comment

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

probably will cause failure for case in line nemesis.py:4153: need to add cql_quote_if_needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we shouldn't add cql_quote_if_needed for sdcm.cluster.BaseNode.get_cfstats - requires verification which form fits it

@timtimb0t timtimb0t force-pushed the quotes_in_ks_name branch from 94a328d to 6ecb2cf Compare March 3, 2025 15:57
@timtimb0t timtimb0t requested review from fruch and soyacz March 3, 2025 15:58
@timtimb0t timtimb0t force-pushed the quotes_in_ks_name branch from 6ecb2cf to 3fa2c75 Compare March 3, 2025 21:05
@soyacz
Copy link
Contributor

soyacz commented Mar 4, 2025

If always dropping quotes, please go again through all usages of get_test_keyspaces method and add 'quote_if_needed` (if required) or remove code that was dropping quotes (which now will be redundant).

@timtimb0t timtimb0t force-pushed the quotes_in_ks_name branch 2 times, most recently from 2b8ba10 to 149e557 Compare March 4, 2025 11:35
@timtimb0t
fix(cluster helper func): double quoting in keyspace names
58b7e7d 
When keyspace names start with a digit, they are returned with
surrounding double quotes. This can lead to double quoting in CQL
statements that can lead to SCT failure. This commit updates the
get_test_keyspaces func to strip these extra quotes. If keyspace
isnt quoted the change is harmless
@timtimb0t timtimb0t force-pushed the quotes_in_ks_name branch from 149e557 to 58b7e7d Compare March 4, 2025 11:36
@timtimb0t
Copy link
Contributor Author

@soyacz , could you please check again? there are few places in nemesis.py without usage cql_unquote_if_needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@temichus temichus temichus approved these changes

@aleksbykov aleksbykov Awaiting requested review from aleksbykov

@fruch fruch Awaiting requested review from fruch

@soyacz soyacz Awaiting requested review from soyacz

At least 2 approving reviews are required to merge this pull request.

Assignees

@timtimb0t timtimb0t

Labels
backport/none Backport is not required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timtimb0t @soyacz @fruch @temichus