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

[Fleet] add support for fleet server urls #94364

Merged
merged 16 commits into from
Mar 26, 2021

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Mar 10, 2021

Summary

Resolve #89442

Allow to configure Fleet server urls, (still support Kibana url) when Fleet server url is configured this settings will be used instead of the Kibana url.

The removal of the Kibana url will be done in a following PRs once all of our CI, e2e tests run Fleet server.

UI Changes

The UI implement the new design from here #94624

  • there is some help text around the field and the fields are full width
  • the save button is only active if there is some change
  • We now have a confirmation modal step

Screen Shot 2021-03-25 at 1 44 41 PM

Screen Shot 2021-03-25 at 9 09 13 AM

How to tests

You can test the UI, and that the policy generated contains fleet.hosts the end to end tests with fleet server is a little complicated now and I am trying to have easier way to run fleet server locally

@nchaulet nchaulet added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 10, 2021
@nchaulet nchaulet self-assigned this Mar 10, 2021
@nchaulet nchaulet added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 10, 2021
@nchaulet nchaulet marked this pull request as ready for review March 11, 2021 20:00
@nchaulet nchaulet requested a review from a team as a code owner March 11, 2021 20:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet requested a review from blakerouse March 11, 2021 20:00
@@ -10,9 +10,11 @@ import type { SavedObjectAttributes } from 'src/core/public';
export interface BaseSettings {
agent_auto_upgrade: boolean;
package_auto_upgrade: boolean;
has_seen_add_data_notice?: boolean;
fleet_server_urls: string[];
// TODO remove as part of https://github.com/elastic/kibana/issues/94303
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, we should add it in more place to make the cleanup easier and not forget things.

Copy link

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

I have tested this change locally with my changes to Elastic Agent (elastic/beats#24713).

I was able to switch between 2 different Fleet Servers by adjusting the URL in the Settings flyout.

@nchaulet nchaulet force-pushed the feature-fleet-server-urls branch from 86e6b4e to 5ca883e Compare March 24, 2021 13:38
@nchaulet nchaulet requested a review from jfsiii March 25, 2021 13:09
Copy link
Contributor

@hbharding hbharding left a comment

Choose a reason for hiding this comment

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

Left 2 comments

In addition, I want to confirm that the flyout's button says

image

and after the user confirm changes through the modal, the flyout's button should use a loading indicator and say

image

Copy link
Contributor

@hbharding hbharding left a comment

Choose a reason for hiding this comment

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

Found 1 more small thing

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

This is an initial code read review, will come back and do local testing.

I was wondering your thoughts on urls vs hosts, we have used both interchangeably for Kibana url/hosts support. @hbharding maybe you can weigh in on the UX side on rationale of surfacing one vs the other term in the UI. I was thinking hosts might be more consistent with other parts of the stack, such as Kibana's elasticsearch.hosts setting, but I'm not aware of many other examples.

@nchaulet nchaulet force-pushed the feature-fleet-server-urls branch from 193f128 to 2e52470 Compare March 25, 2021 19:37
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Tested locally and the main behavior works as expected, I see fleet.hosts populated in the agent yaml when I change the setting. I am approving but I would like to get consensus on urls vs hosts before this gets merged. It's a little strange that the setting goes from fleet_server.hosts to -> fleet_server_urls and back to -> fleet.hosts in the end.

Small UI suggestion, I think we should add the same non-empty validation for Fleet server & Elasticsearch URL inputs as we currently do for Kibana URLs, currently they don't any show any errors when trying to save with empty values:

image

return (
<EuiText size="s" color={item.direction === 'added' ? 'secondary' : 'danger'}>
{item.urls.map((url) => (
<div key={url}>{url}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

unlikely that the user will enter the same URL twice, but it might be safer to key on index

throw new Error('kibana_urls is missing');

fullAgentPolicy.fleet = {
hosts: settings.kibana_urls,
Copy link
Contributor

Choose a reason for hiding this comment

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

is it expected to have fleet.hosts be kibana URLs when there there aren't any fleet server urls configured yet? i guess this is temporary until we remove kibana URLs support?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that correct agent 7.13 will only support fleet.hosts even for Kibana during a short period to not break all of our CI

@hbharding
Copy link
Contributor

Thanks for raising the inconsistency on host vs url @jen-huang #94364 (review). It wasn't a conscious decision on my part. I think Fleet Server Host as the input label makes sense (as long we provide a description saying this is the URL of the host, which we do) and it is more consistent with the policy's YAML and other examples you've pointed out in our stack. Using the same logic, we would also rename the ES input to say Elasticsearch Host.

👍 I'm pro host and pro consistency. My only hesitation is that technically speaking the input's value is a URL and not just the host/hostname (number 2 in the diagram). But we address that in the input's help text.

image

cc @mostlyjason for awareness.

@nchaulet
Copy link
Member Author

Small UI suggestion, I think we should add the same non-empty validation for Fleet server & Elasticsearch URL inputs as we currently do for Kibana URLs, currently they don't any show any errors when trying to save with empty values:

I plan to add this as soon as we remove the Kibana URL I added a comment in the code for that.

@nchaulet
Copy link
Member Author

nchaulet commented Mar 26, 2021

Updated to use hosts instead of URL
Screen Shot 2021-03-26 at 12 47 00 PM

@hbharding
Copy link
Contributor

It looks like there is a stray character that should be removed at the end of the URL:

image

@nchaulet nchaulet added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 26, 2021
@nchaulet nchaulet enabled auto-merge (squash) March 26, 2021 18:47
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 461 462 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 705.7KB 716.3KB +10.7KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
ingest_manager_settings 4 5 +1

History

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

cc @nchaulet

@nchaulet nchaulet merged commit db7da22 into elastic:master Mar 26, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 26, 2021
@nchaulet nchaulet deleted the feature-fleet-server-urls branch March 26, 2021 18:56
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #95574

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 26, 2021
@amolnater-qasource
Copy link

Hi @EricDavisX

We have created 02 testcases for above merged PR. Test-cases link are as follows:

Further we have updated 13 testcases under Ingest Manager Test suite Section: Fleet Settings. Please let us know if we have missed anything to put down in expected result.

Thanks
QAS

@hbharding
Copy link
Contributor

@nchaulet We got some input from tech writers for some small text changes here #94624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Add fleet server URL
8 participants