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

RHCLOUD-30039 Use TLS CA from Clowder with recipients-resolver #2422

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gwenneg
Copy link
Member

@gwenneg gwenneg commented Jan 11, 2024

No description provided.

@gwenneg gwenneg force-pushed the RHCLOUD-30039-clowder-truststore branch from aa6fdb5 to 7a7d8bb Compare January 11, 2024 09:34
@gwenneg gwenneg marked this pull request as ready for review January 11, 2024 09:40
@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2024

Codecov Report

Attention: Patch coverage is 14.28571% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 69.89%. Comparing base (2850180) to head (8027e7a).
Report is 69 commits behind head on master.

Files Patch % Lines
...tifications/connector/email/EmailRouteBuilder.java 6.66% 14 Missing ⚠️
...s/connector/email/config/EmailConnectorConfig.java 40.00% 3 Missing ⚠️
...t/cloud/notifications/config/AggregatorConfig.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2422      +/-   ##
============================================
- Coverage     69.98%   69.89%   -0.10%     
+ Complexity     1794     1793       -1     
============================================
  Files           379      379              
  Lines          7903     7919      +16     
  Branches        685      686       +1     
============================================
+ Hits           5531     5535       +4     
- Misses         2080     2093      +13     
+ Partials        292      291       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.sslContextParameters(getSslContextParameters())
.x509HostnameVerifier(NoopHostnameVerifier.INSTANCE);
HttpEndpointBuilder endpointBuilder = https(fullURL.replace("https://", ""));
if (emailConnectorConfig.getRecipientsResolverTrustStorePath().isPresent() && emailConnectorConfig.getRecipientsResolverTrustStorePassword().isPresent() && emailConnectorConfig.getRecipientsResolverTrustStoreType().isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a keystore without password ? is password field is always mandatory, even it it's empty ?
I ask because on Kafka client setup, we must not add the keystore password configuration key if its empty.


endpointBuilder.sslContextParameters(sslContextParameters);
} else {
Log.warn("TLS is enabled for recipients-resolver but the trust store could not be used to build the Camel endpoint because the trust store path, password or type are missing in the Clowder configuration");
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we invalidate the health check it this error happends ?
My consern is if something goes wrong, all pods could be deployed with an inconsistent ssl config and all requests to the recipients-resolver will probably fail, involving a production outage ?

@g-duval
Copy link
Contributor

g-duval commented Jan 17, 2024

Shouldn't we apply the same keystore management on engine ?

@gwenneg gwenneg marked this pull request as draft January 25, 2024 18:27
@g-duval g-duval marked this pull request as ready for review February 15, 2024 13:37
@g-duval
Copy link
Contributor

g-duval commented Feb 15, 2024

/retest

2 similar comments
@lindgrenj6
Copy link

/retest

@lindgrenj6
Copy link

/retest

@g-duval g-duval force-pushed the RHCLOUD-30039-clowder-truststore branch from b4ad6df to 8027e7a Compare April 16, 2024 12:29
@g-duval
Copy link
Contributor

g-duval commented May 2, 2024

/retest

@g-duval g-duval force-pushed the RHCLOUD-30039-clowder-truststore branch from 8027e7a to 808e395 Compare May 3, 2024 09:28
@aleccohan
Copy link

/retest

1 similar comment
@syncrou
Copy link

syncrou commented May 15, 2024

/retest

@g-duval g-duval force-pushed the RHCLOUD-30039-clowder-truststore branch from 808e395 to 4580446 Compare May 16, 2024 08:07
@g-duval g-duval force-pushed the RHCLOUD-30039-clowder-truststore branch from 4580446 to 5e7f519 Compare May 29, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants