[Backport 2.x] [Multiple Datasources] Add TLS configuration for multiple data sources (#6171) #6244
Conversation
opensearch-project#6171) * Add TLS configuration for multiple data sources Signed-off-by: Craig Perkins <[email protected]> * Add to CHANGELOG and add examples commented out in opensearch_dashboards.yml Signed-off-by: Craig Perkins <[email protected]> * Add tests and replace instance of any Signed-off-by: Craig Perkins <[email protected]> * Add tls config to legacy client Signed-off-by: Craig Perkins <[email protected]> * Add test for certificate mode Signed-off-by: Craig Perkins <[email protected]> * Respond to PR feedback Signed-off-by: Craig Perkins <[email protected]> * Extract readCertificateAuthorities to util file and add more tests Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit a9b400e)
cwperks
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
BSFishy,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT and
xinruiba
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #6244 +/- ##
=======================================
Coverage 67.26% 67.27%
=======================================
Files 3335 3336 +1
Lines 64580 64623 +43
Branches 10327 10337 +10
=======================================
+ Hits 43441 43475 +34
- Misses 18635 18638 +3
- Partials 2504 2510 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| # To disregard the validity of SSL certificates for connected data sources, change this setting's value to 'none'. | ||
| # Possible values include full, certificate and none | ||
| #data_source.ssl.verificationMode: full |
There was a problem hiding this comment.
Are these explained in a README doc?
There was a problem hiding this comment.
@AMoo-Miki Where do you think would be best to document this? These settings are already defined for the default opensearch via the settings opensearch.ssl.verificationMode and opensearch.ssl.certificationAuthorities and this PR extends those to datasources. These settings would be commonly used for all configured datasources, although I filed another issue on per datasource configuration.
Here are the analogous entries for default OpenSearch.
There was a problem hiding this comment.
Adding some more info to src/plugins/data_source/README would be great. As a user, I find OSD's documentation very inadequate and this small step would help users find answers sooner.
| certificateAuthorities: schema.maybe( | ||
| schema.oneOf([schema.string(), schema.arrayOf(schema.string(), { minSize: 1 })]) | ||
| ), |
There was a problem hiding this comment.
The minSize unnecessarily complicates this; why should we not consider a blank array as undefined?
There was a problem hiding this comment.
This is the same as the client for default opensearch. The ways its implemented in this PR handles empty as you describe, so minSize can be removed here, but I had this here to keep it consistent with the other place its defined for default opensearch.
There was a problem hiding this comment.
That is great. i think we can progressively enhance these. I am glad to hear that the code can handle that case.
|
@AMoo-Miki Is there anything you'd like me to change in this backport or on main first and then into this backport? |
@cwperks let's deal with it in a separate PR and backport. |
Manual backport of #6171 to 2.x.