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

OpenZipkin/Cassandra StorageType to turn On/off Hostname verification #3427 #3457

Closed
wants to merge 0 commits into from

Conversation

priyavivek2307
Copy link

No description provided.

@@ -144,6 +146,8 @@ zipkin:
index-fetch-multiplier: ${CASSANDRA_INDEX_FETCH_MULTIPLIER:3}
# Using ssl for connection, rely on Keystore
use-ssl: ${CASSANDRA_USE_SSL:false}
# Override hostname verification in Cassandra setting
override-hostname-verification: ${CASSANDRA_OVERRIDE_HOSTNAME:false}
Copy link
Contributor

@jcchavezs jcchavezs Jun 20, 2022

Choose a reason for hiding this comment

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

I'd rather call the var CASSANDRA_OVERRIDE_HOSTNAME_VERIFICATION to be consistent.

Copy link
Author

@priyavivek2307 priyavivek2307 Jun 20, 2022

Choose a reason for hiding this comment

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

updated the variable name as suggested. @jcchavezs

Copy link
Member

@codefromthecrypt codefromthecrypt Jan 23, 2024

Choose a reason for hiding this comment

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

for future, any time we are going to add a new variable, follow existing conventions first. The easiest way and regardless of cassandra or not, is to do a web or github search for the property and use that. If there are multiple styles used, prefer the most used, for example other tools that configure cassandra.

Regardless, we don't want any incoherence in our codebase, so we needn't invent anything and just reuse the norm. My follow-up PR does that, just explaining here as I noticed sometimes folks accidentally don't look and then ask others to use non-conventional names. https://github.com/apache/cassandra-java-driver/blob/8d5849cb38995b312f29314d18256c0c3e94cf07/core/src/main/java/com/datastax/oss/driver/api/core/config/TypedDriverOption.java#L230

@@ -77,7 +79,12 @@ public static CqlSession buildSession(
config = config.withBoolean(REQUEST_DEFAULT_IDEMPOTENCE, true);

if (useSsl) config = config.withClass(SSL_ENGINE_FACTORY_CLASS, DefaultSslEngineFactory.class);


if (overrideHostnameVerification)

Choose a reason for hiding this comment

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

I think we can write this in one line, excluding the need of if-else.

Replacing this

if (overrideHostnameVerification)
config = config.withBoolean(SSL_HOSTNAME_VALIDATION, true);
else
config = config.withBoolean(SSL_HOSTNAME_VALIDATION, false);

With this

config = config.withBoolean(SSL_HOSTNAME_VALIDATION, overrideHostnameVerification);

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

if you can use the square formatter, the change will be simpler as it won't change so many lines. I think you are using tabs where the project uses spaces

@codefromthecrypt
Copy link
Member

I think to get this across the finish line, it might be faster to redo it. Reason is that the cassandra property is SSL_HOSTNAME_VALIDATION and so that means our property should be CASSANDRA_SSL_HOSTNAME_VALIDATION for env and sslHostnameValidation for the field. using different names can cause confusion. Most of the work not done here is adding the spring yaml plumbing to propagate this setting, as well update the zipkin-server README section for cassandra.

@codefromthecrypt
Copy link
Member

reopened in #3701 as I was unable to alter the branch under this PR.

@codefromthecrypt
Copy link
Member

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.

4 participants