Skip to content

Conversation

@yyin-talend
Copy link
Collaborator

Requirements

  • Any code change adding any logic MUST be tested through a unit test executed with the default build
  • Any API addition MUST be done with a documentation update if relevant

Why this PR is needed?

What does this PR adds (design/code thoughts)?

if (acceptAnyCertificate) {
builder.hostnameVerifier((host, session) -> true);
builder.sslContext(createUnsafeSSLContext());
} else if (keystoreLocation.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if keystore is present or not, or, simply raise an exception when it is not present ? (maybe a question to @coheigea )

Copy link
Member

Choose a reason for hiding this comment

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

I assume the code falls back to the JDK defaults if the keystore isn't present? If so that would be OK, and we could just log this.

builder.hostnameVerifier(hostnameVerifier);
builder.sslContext(createSSLContext(keystoreLocation, keystoreType, keystorePassword, truststoreType));
} else {
log.warn("Key store location is NOT present. ");
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 it is an issue to not have the keystore location defined. If not the default java certificate management will be used.
So maybe just :
log.info("TCK vault-client doesn't explicitly define the keystore location. You can use 'talend.vault.cache.client.vault.certificate.keystore.location' and 'talend.vault.cache.client.vault.certificate.keystore.type' to explicit it.")

or something like this...

@sonar-eks
Copy link

sonar-eks bot commented Jan 9, 2026

builder.hostnameVerifier(hostnameVerifier);
builder.sslContext(createSSLContext(keystoreLocation, keystoreType, keystorePassword, truststoreType));
} else {
log.info("TCK vault-client doesn't explicitly define the keystore location. Please configure " +
Copy link
Member

@undx undx Jan 9, 2026

Choose a reason for hiding this comment

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

Isn't a warning more relevant?

Copy link
Member

@undx undx left a comment

Choose a reason for hiding this comment

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

minor comment

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.

5 participants