Skip to content

Make "legacy" Kibana server aware of connection protocol.#20756

Merged
azasypkin merged 2 commits intoelastic:masterfrom
azasypkin:issue-xxx-legacy-server-info
Jul 13, 2018
Merged

Make "legacy" Kibana server aware of connection protocol.#20756
azasypkin merged 2 commits intoelastic:masterfrom
azasypkin:issue-xxx-legacy-server-info

Conversation

@azasypkin
Copy link
Contributor

@azasypkin azasypkin commented Jul 13, 2018

It somehow slipped under my radar, but we need to make sure that legacy Kibana is aware of TLS as several places in Kibana rely on server.info.protocol or req.connection.info.protocol (that's basically the same) to build correct external URLs (e.g. SAML in Security plugin and PDF generation in Reporting plugin).

To test, please try to generate PDF reports when ES and Kibana run as shown below:

$ yarn es snapshot --license trial
yarn start --no-base-path --server.ssl.enabled=true \
  --server.ssl.key=/projects/elastic/master/kibana/test/dev_certs/server.key \
  --server.ssl.certificate=/projects/elastic/master/kibana/test/dev_certs/server.crt

and then

$ yarn es snapshot --license trial
yarn start --no-base-path

And yeah, we desperately need TLS-enabled integration/functional tests :)

@azasypkin azasypkin added bug Fixes for quality problems that affect the customer experience Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform labels Jul 13, 2018
@azasypkin azasypkin requested review from rhoboat and spalger July 13, 2018 09:14
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

server.connection({
host: config.get('server.host'),
port: config.get('server.port'),
tls: config.get('server.ssl.enabled'),
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment above says that these values are the same as the new platform configuration, but that's not the case with this tls config. Here tls is a boolean, but in the new platform it's an object with properties. Can you either make them the same or update the comment so this doesn't look like a bug in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sure, I'll update comment

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

I tested that PDF reporting from visualize works with https enabled.

LGTM once you update the comment

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Contributor Author

retest

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Contributor Author

flaky selenium test

@azasypkin
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin azasypkin merged commit 52880f8 into elastic:master Jul 13, 2018
@azasypkin azasypkin deleted the issue-xxx-legacy-server-info branch July 13, 2018 15:55
azasypkin added a commit to azasypkin/kibana that referenced this pull request Jul 13, 2018
@azasypkin
Copy link
Contributor Author

6.x\6.4: 2d8ff67

yankouskia pushed a commit to yankouskia/kibana that referenced this pull request Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported bug Fixes for quality problems that affect the customer experience Feature:New Platform Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v6.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants