Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Support customized server config #313

Merged

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Feb 5, 2021

Issue #, if available:
#98
This is the most reported issue so far. Users are not able to generate any report if they have customized Kibana sever setting(host, sever base path, port), or enable TLS.

Server config is set in kibana.yml

Description of changes:

  • remove the hardcoded localhost:5601 when building the complete url
  • Dynamically retrieve serverInfo(which contains port, host, protocol), and server.basePath and pass them through route handler for validating the input url, compose complete url.
  • Add one more puppeteer launch option ignoreHttpsError: true to support TLS-enabled kibana server https://github.com/puppeteer/puppeteer/blob/main/docs/api.md#puppeteerlaunchoptions
  • Add one more param "path: server.basePath" to the cookie object, to support customized server.basePath when security is enabled.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #313 (3d518fb) into dev (1a5b871) will increase coverage by 0.07%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #313      +/-   ##
============================================
+ Coverage     64.16%   64.24%   +0.07%     
  Complexity      291      291              
============================================
  Files           100      100              
  Lines          4044     4047       +3     
  Branches        614      613       -1     
============================================
+ Hits           2595     2600       +5     
+ Misses         1288     1287       -1     
+ Partials        161      160       -1     
Flag Coverage Δ Complexity Δ
Kibana-reports 77.82% <88.88%> (+0.14%) 0.00 <0.00> (ø)
reports-scheduler 53.28% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
kibana-reports/server/routes/utils/constants.ts 100.00% <ø> (ø) 0.00 <0.00> (ø)
...r/routes/utils/visual_report/visualReportHelper.ts 83.92% <ø> (ø) 0.00 <0.00> (ø)
...orts/server/routes/utils/converters/backendToUi.ts 79.04% <83.33%> (+2.12%) 0.00 <0.00> (ø)
kibana-reports/server/utils/validationHelper.ts 90.00% <100.00%> (+0.81%) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a5b871...3d518fb. Read the comment docs.

@zhongnansu zhongnansu requested review from joshuali925, akbhatta and davidcui1225 and removed request for joshuali925 February 5, 2021 01:11
@zhongnansu zhongnansu marked this pull request as ready for review February 5, 2021 01:11
exposeToBrowser: {
access: true,
},
schema: configSchema,
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this file do besides defining KibanaReportsPluginConfigType? it seems it's adding port and basepath config to the kibana.yml file? shouldn't be necessary since we are not reading from yml

Copy link
Member Author

Choose a reason for hiding this comment

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

This is something I forget to remove when I tried the plugin config approach. Will remove

) => {
report.query_url = report.query_url.replace(basePath, '');
report.report_definition.report_params.core_params.base_url = report.report_definition.report_params.core_params.base_url.replace(
basePath,
Copy link
Contributor

Choose a reason for hiding this comment

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

is query_url/base_url the full url with hostname? e.g. localhost:5601/alh/app/... would it replace alh in localhost

Copy link
Member Author

Choose a reason for hiding this comment

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

query_url doesn't contain hostname

@zhongnansu zhongnansu merged commit 21aebcc into opendistro-for-elasticsearch:dev Feb 5, 2021
@Artain Artain mentioned this pull request Feb 19, 2021
zhongnansu added a commit to zhongnansu/kibana-reports that referenced this pull request Feb 26, 2021
zhongnansu added a commit that referenced this pull request Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access kibana.yml from plugin to read server.host, server.port and server.basePath
3 participants