Wire kibana config from fleet #4670

jalvz · 2021-02-01T16:51:48Z

Motivation/summary

Adds support for central config and sourcemaps in managed mode.

Checklist

Update CHANGELOG.asciidoc
Documentation has been updated
Tests

How to test these changes

In the Kibana UI create an APM integration without (or with a wrong API key), and check that curl http://localhost:8200/config/v1/agents\?service.name=foo returns "Unauthorized". Then create a valid api key and update the apm policy with it. Repeat the same curl command, and this time should respond with service-does-not-exist error.

For sourcemaps: index a sourcemap, and check that APM Server can read it back when ingesting errors, provided the right API key has been configured in the policy editor.

Related issues

Requires elastic/beats#23856

Closes #4573

apmmachine · 2021-02-01T16:57:29Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #4670 updated
Start Time: 2021-02-11T18:48:07.695+0000
Duration: 43 min 58 sec
Commit: cc403ce

Test stats 🧪

Test	Results
Failed	0
Passed	4727
Skipped	124
Total	4851

Trends 🧪

Steps errors

Expand to view the steps failures

`Run Window tests`

Took 11 min 51 sec . View more details on here

`Compress`

Took 0 min 0 sec . View more details on here
Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage

`Compress`

Took 0 min 0 sec . View more details on here
Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests

`Test Sync`

Took 3 min 27 sec . View more details on here
Description: ./.ci/scripts/sync.sh

jalvz · 2021-02-04T16:41:56Z

Still missing tests, but opening this already for review

axw

I'm pretty confused about what this PR is meant to be doing.

We discussed that the API Key passed used by Fleet would not have sufficient privileges, i.e. would not have the APM space privileges required for querying central config. IIANM that's why you added apm-server.kibana section to the integration package.

So why is elastic/beats#23856 needed, and why is the server still looking at the Fleet config?

axw · 2021-02-05T01:28:33Z

beater/beater.go

@@ -220,7 +223,7 @@ func (s *serverCreator) CheckConfig(cfg *common.Config) error {

 func (s *serverCreator) Create(p beat.PipelineConnector, rawConfig *common.Config) (cfgfile.Runner, error) {
 	integrationConfig, err := config.NewIntegrationConfig(rawConfig)
-	if err != nil {
+	if integrationConfig == nil || err != nil {


It doesn't make sense to return a nil cfgfile.Runner unless there's an error being returned, and integrationConfig will/should only be nil if config.NewIntegrationConfig returns an error -- so I don't think this is right.

beater/config/config.go

axw · 2021-02-05T02:09:36Z

beater/beater.go

+	cfg, err := config.NewConfig(args.RawConfig, args.KibanaConfig, elasticsearchOutputConfig(args.Beat))
 	if err != nil {
 		return nil, err
 	}


Suggested change

cfg, err := config.NewConfig(args.RawConfig, args.KibanaConfig, elasticsearchOutputConfig(args.Beat))

if err != nil {

return nil, err

}

cfg, err := config.NewConfig(args.RawConfig, elasticsearchOutputConfig(args.Beat))

if err != nil {

return nil, err

}

if args.KibanaConfig != nil {

args.Kibana = *args.KibanaConfig

}

Seeing as this is the only place we pass in the arg, can we just replace the Kibana config afterwards and avoid forcing all other callers to pass in nil? Alternatively we could not unpack Fleet.Kibana in the integration config, and merge it into the config in serverCreator.Create.

axw · 2021-02-05T02:15:53Z

apmpackage/apm/0.1.0/agent/input/template.yml.hbs

@@ -2,3 +2,6 @@ apm-server:
    host: {{host}}
    secret_token: {{secret_token}}
    rum.enabled: {{enable_rum}}
+    kibana:
+        enabled: true


Suggested change

enabled: true

enabled: true is implied, it's not necessary to explicitly specify it.

Sure, I just liked to be explicit.

jalvz · 2021-02-05T09:04:04Z

We discussed that the API Key passed used by Fleet would not have sufficient privileges

And this doesn't take the access_api_key from Fleet indeed, just the Kibana connection information (host, etc). We need that, right?

The API Key comes from user input in the package, as we talked about.

simitt · 2021-02-05T09:13:21Z

apmpackage/apm/0.1.0/manifest.yml

@@ -41,6 +41,11 @@ policy_templates:
        required: true
        show_user: true
        default: false
+      - name: kibana_api_key
+        type: string


should this be type: password?

it appears there is no such a thing...

Am I looking at the wrong place here https://github.com/elastic/package-spec/blob/a8fe73d82b25806f6b96582b99993a63b67ef130/versions/1/data_stream/manifest.spec.yml#L53?

axw · 2021-02-05T09:16:13Z

And this doesn't take the access_api_key from Fleet indeed, just the Kibana connection information (host, etc). We need that, right?

Ahhhh sorry, I see. Thanks for the explanation.

If elastic/beats#23856 does go ahead: is it not possible to have the spec extract "fleet.kibana" and merge it into "apm-server.kibana"? Is that the bit you said you couldn't get to work? I suppose this approach is fine though.

Anyway, I see there's some contention on the Beats issue so let's wait until that's resolved?

jalvz · 2021-02-05T15:04:29Z

is it not possible to have the spec extract "fleet.kibana" and merge it into "apm-server.kibana"? Is that the bit you said you couldn't get to work?

I tried a few things, I believe the problem with that one is that it is not possible to merge dicts on conflict; so we would lose either the api_key or the other fields.

axw

LGTM. A couple of things I'd like to see:

A test for the new APIKey behaviour in kibana.ConnectingClient. I think this should be done before merging.
A system test for agent central config when running under Fleet, using the injected fleet.kibana config, as a followup.

…to fleet-central-config

* Adds kibana and sourcemap api keys to package # Conflicts: # apmpackage/apm/0.1.0/agent/input/template.yml.hbs # apmpackage/apm/0.1.0/manifest.yml # changelogs/head.asciidoc # kibana/connecting_client.go

* Adds kibana and sourcemap api keys to package # Conflicts: # apmpackage/apm/0.1.0/agent/input/template.yml.hbs # apmpackage/apm/0.1.0/manifest.yml # changelogs/head.asciidoc # kibana/connecting_client.go Co-authored-by: Juan Álvarez <[email protected]>

axw · 2021-02-24T12:56:50Z

I tested this with 7.12 BC2, and it doesn't seem to be working. I started the stack with apm-integration-testing (with elastic/apm-integration-testing#1069 applied):

./scripts/compose.py start 7.12 --bc=37f40745 --apm-server-managed --with-elastic-agent --package-registry-snapshot

Then looking at the APM Server logs in the Fleet UI, I see logs like:

[elastic_agent.apm_server][info] Kibana url: http://localhost:5601
[elastic_agent.apm_server][error] failed to obtain connection to Kibana: fail to get the Kibana version: HTTP GET request to http://localhost:5601/api/status fails: fail to execute the HTTP GET request: Get "http://localhost:5601/api/status": dial tcp 127.0.0.1:5601: connect: connection refused. Response: .

jalvz · 2021-02-24T14:54:48Z

sorry @axw , Beats backports not merged yet

axw · 2021-03-03T03:06:36Z

Tested again with BC3, works well!

jalvz force-pushed the fleet-central-config branch from b9a8bba to 6c00307 Compare February 4, 2021 16:22

wire kibana config from fleet

392ea19

jalvz force-pushed the fleet-central-config branch from 6c00307 to 392ea19 Compare February 4, 2021 16:27

jalvz mentioned this pull request Feb 4, 2021

Update apm-server spec elastic/beats#23856

Merged

jalvz marked this pull request as ready for review February 4, 2021 16:41

jalvz requested a review from a team February 4, 2021 16:42

axw requested changes Feb 5, 2021

View reviewed changes

simitt reviewed Feb 5, 2021

View reviewed changes

jalvz added 2 commits February 9, 2021 13:16

Merge remote-tracking branch 'elastic/master' into fleet-central-config

73311c4

Code review + sourcemaps api key and fixes

07589d5

jalvz requested a review from axw February 9, 2021 13:46

axw approved these changes Feb 10, 2021

View reviewed changes

jalvz added 12 commits February 10, 2021 13:35

fix template

ff2cb74

add unit tests

c0c3891

update changelog

7b00869

update docs

8494c1c

Merge branch 'master' into fleet-central-config

4662003

fix struct tags

722f349

Merge branch 'fleet-central-config' of github.com:jalvz/apm-server in…

5fa35cb

…to fleet-central-config

fix test

5e59f0d

fix test again

8b27b92

Merge branch 'master' into fleet-central-config

6ebf5e1

goimports

db40635

Merge branch 'fleet-central-config' of github.com:jalvz/apm-server in…

cc403ce

…to fleet-central-config

jalvz merged commit 8507904 into elastic:master Feb 15, 2021

jalvz mentioned this pull request Feb 15, 2021

[Fleet] APM Server managed by Elastic Agent with Fleet - 7.12 #4558

Closed

15 tasks

axw mentioned this pull request Feb 19, 2021

[7.x] Wire kibana config from fleet (#4670) #4839

Merged

axw mentioned this pull request Feb 19, 2021

[7.12] Wire kibana config from fleet (#4670) #4840

Merged

axw added test-plan v7.12.0 labels Feb 20, 2021

axw self-assigned this Feb 24, 2021

axw added the test-plan-ok label Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wire kibana config from fleet #4670

Wire kibana config from fleet #4670

jalvz commented Feb 1, 2021 •

edited

Loading

apmmachine commented Feb 1, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

`Run Window tests`

`Compress`

`Compress`

`Test Sync`

jalvz commented Feb 4, 2021

axw left a comment

axw Feb 5, 2021

axw Feb 5, 2021

axw Feb 5, 2021

jalvz Feb 5, 2021

jalvz commented Feb 5, 2021

simitt Feb 5, 2021

jalvz Feb 9, 2021

simitt Feb 11, 2021

axw commented Feb 5, 2021

jalvz commented Feb 5, 2021 •

edited

Loading

axw left a comment

axw commented Feb 24, 2021

jalvz commented Feb 24, 2021

axw commented Mar 3, 2021

Wire kibana config from fleet #4670

Wire kibana config from fleet #4670

Conversation

jalvz commented Feb 1, 2021 • edited Loading

Motivation/summary

Checklist

How to test these changes

Related issues

apmmachine commented Feb 1, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

Steps errors

Run Window tests

Compress

Compress

Test Sync

jalvz commented Feb 4, 2021

axw left a comment

Choose a reason for hiding this comment

axw Feb 5, 2021

Choose a reason for hiding this comment

axw Feb 5, 2021

Choose a reason for hiding this comment

axw Feb 5, 2021

Choose a reason for hiding this comment

jalvz Feb 5, 2021

Choose a reason for hiding this comment

jalvz commented Feb 5, 2021

simitt Feb 5, 2021

Choose a reason for hiding this comment

jalvz Feb 9, 2021

Choose a reason for hiding this comment

simitt Feb 11, 2021

Choose a reason for hiding this comment

axw commented Feb 5, 2021

jalvz commented Feb 5, 2021 • edited Loading

axw left a comment

Choose a reason for hiding this comment

axw commented Feb 24, 2021

jalvz commented Feb 24, 2021

axw commented Mar 3, 2021

jalvz commented Feb 1, 2021 •

edited

Loading

apmmachine commented Feb 1, 2021 •

edited

Loading

`Run Window tests`

`Compress`

`Compress`

`Test Sync`

jalvz commented Feb 5, 2021 •

edited

Loading