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.

Sign up for GitHub

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

Jump to bottom

[Ingest Manager] Use cloudId for ES & Kibana URLs if available. #65366

Merged
merged 3 commits into from
May 5, 2020
Merged

[Ingest Manager] Use cloudId for ES & Kibana URLs if available. #65366

merged 3 commits into from
May 5, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented May 5, 2020 •
edited
Loading

Summary

closes #64513

  • Adds decodeCloudId function based on code & tests in libbeat. Tests added in cf7b45a
  • If there's a cloudId, use those hosts. Otherwise, use anything passed in from the command line. Use localhost if all else fails.
  • Ideally we'd set this config earlier & centrally, but we currently only use each in one place (and we're staring down Feature Freeze)

Test

This only takes effect when creating the default host, so start ES fresh to ensure a default output is created.

Cloud

Start kibana with cloud plugin & ID

yarn start --no-base-path --xpack.ingestManager.enabled=true \
  --xpack.ingestManager.fleet.elasticsearch.host='http://from-ingest-manager-cli-flags' \
  --xpack.cloud.enabled=true \
  --xpack.cloud.id='custom-port:dXMtY2VudHJhbDEuZ2NwLmNsb3VkLmVzLmlvOjkyNDMkYWMzMWViYjkwMjQxNzczMTU3MDQzYzM0ZmQyNmZkNDYkYTRjMDYyMzBlNDhjOGZjZTdiZTg4YTA3NGEzYmIzZTA='

See the values that corresponds to here or
Screen Shot 2020-05-05 at 3 53 24 PM

Default output has the cloud value, even though a CLI flag was passed:
Screen Shot 2020-05-05 at 3 53 47 PM

Kibana URL is Cloud URL not local
Screen Shot 2020-05-05 at 3 55 07 PM

Local

Start Kibana without cloud & with one host via CLI

yarn start --no-base-path --xpack.ingestManager.enabled=true \
  --xpack.ingestManager.fleet.elasticsearch.host='http://from-ingest-manager-cli-flags' \

Default output has CLI value
Screen Shot 2020-05-05 at 4 11 28 PM

Kibana URL is local (port 5603 + basepath in this example)
Screen Shot 2020-05-05 at 4 11 46 PM

@jfsiii jfsiii requested review from a team and nchaulet May 5, 2020 20:31
@jfsiii jfsiii added the Team:Fleet Team label for Observability Data Collection Fleet team label May 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii changed the title Use cloudId for ES & Kibana URLs if available. [Ingest Manager] Use cloudId for ES & Kibana URLs if available. May 5, 2020
@jfsiii jfsiii added the release_note:skip Skip the PR/issue when compiling release notes label May 5, 2020
@jfsiii jfsiii self-assigned this May 5, 2020
nchaulet
nchaulet approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Nothing blocking it would be nice if we can have a logger un appContext instead of using console.log

nchaulet
nchaulet reviewed May 5, 2020
View reviewed changes
x-pack/plugins/ingest_manager/common/services/decode_cloud_id.ts Outdated
if (words.length < 3) {
// throw new Error(`Expected at least 3 parts in ${decoded}`);
// eslint-disable-next-line no-console
console.log(`Expected at least 3 parts in ${decoded}`);
Copy link
Member

Choose a reason for hiding this comment

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

missing a return here no?

jen-huang
jen-huang reviewed May 5, 2020
View reviewed changes
x-pack/plugins/ingest_manager/server/services/output.ts
const cloudUrl = cloudId && decodeCloudId(cloudId)?.elasticsearchUrl;
const flagsUrl = appContextService.getConfig()!.fleet.elasticsearch.host;
const defaultUrl = 'http://localhost:9200';
const defaultOutputUrl = cloudUrl || flagsUrl || defaultUrl;
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 it be flagsUrl || cloudUrl || defaultUrl? (prefer flagsUrl over everything else including cloudUrl)

Copy link
Member

Choose a reason for hiding this comment

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

I think if we are in the context of cloud cloudUrl is probably the better bet also you should not be able to edit the flags as these flags are not whitelisted

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I wasn't clear on where we were with having our settings whitelisted in cloud. if they are not going to end up whitelisted then my comment isn't relevant 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jen-huang The logic was initially discussed in Slack & #64513 (comment)

@nchaulet is correct that we cannot use these flags in Cloud. We should only have a cloudId if we're in cloud and, at least for now, if we're in Cloud we do not want users pointing to other instances.

x-pack/plugins/ingest_manager/server/services/setup.ts
@@ -53,7 +58,7 @@ export async function setupIngestManager(
return settingsService.saveSettings(soClient, {
agent_auto_upgrade: true,
package_auto_upgrade: true,
kibana_url: appContextService.getConfig()?.fleet?.kibana?.host ?? defaultKibanaUrl,
kibana_url: cloudUrl || flagsUrl || defaultUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here wrt ordering

@jfsiii jfsiii added the Ingest Management:alpha1 Group issues for ingest management alpha1 label May 5, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jfsiii jfsiii merged commit ee9c1a0 into elastic:master May 5, 2020
@jfsiii jfsiii mentioned this pull request May 5, 2020
Merged
jfsiii pushed a commit to jfsiii/kibana that referenced this pull request May 5, 2020
[Ingest Manager] Use cloudId for ES & Kibana URLs if available. (elas…
67d872d 
…tic#65366)

* Use cloudId for ES & Kibana URLs if available.

* Add tests and a missing return

* Use console.debug for consistency
jfsiii pushed a commit that referenced this pull request May 6, 2020
[Ingest Manager] Use cloudId for ES & Kibana URLs if available. (#65366…
1c0e76b 
…) (#65393)

* Use cloudId for ES & Kibana URLs if available.

* Add tests and a missing return

* Use console.debug for consistency
@jfsiii jfsiii mentioned this pull request May 11, 2020
Closed
@jfsiii jfsiii mentioned this pull request Nov 26, 2020
Closed
@jfsiii jfsiii mentioned this pull request Apr 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nchaulet nchaulet nchaulet approved these changes

@jen-huang jen-huang jen-huang left review comments

Assignees

@jfsiii jfsiii

Labels
Ingest Management:alpha1 Group issues for ingest management alpha1 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Fleet] Better default value for config kibana host and elasticsearch host
6 participants
@jfsiii @elasticmachine @kibanamachine @nchaulet @jen-huang @sorenlouv