Update the shipper to work with the elastic-agent #185

fearful-symmetry · 2022-11-17T22:27:22Z

What does this PR do?

This closes #161 and updates the shipper's V2 client code to work with the elastic-agent. There's a number of missing pieces:

We have a number of open design issues that relate to the shipper control system: Meta Issue: Shipper configuration and I/O #226. As it stands, I'm not sure if we want to keep this PR open until we resolve those issues, or merge and change as we go.
This is a bit hard to test, as the beat shipper output has a blocking bug: Shipper output running into gRPC marshalling errors on filebeat beats#34319
TLS has a lot of issues, and is currently disabled:
- The beats shipper output has some config issues: Beats shipper output will not enable TLS beats#34321
- The PKI design itself is non-traditional and under consideration: Standardize the shipper's gRPC TLS setup process #224
- Even temporarily working around some of these issues, I'm still running into a lot of TLS-related connection errors. Currently hoping the above PKI redesign will help fix this.
- We're failing to send events to ES due to this (I assume unrelated) error: Limit of mapping depth [20] has been exceeded due to object field [fields.data.system.Kind.StructValue.data.cpu.Kind.StructValue.data.idle.Kind.StructValue.data.norm.Kind.StructValue.data.pct.Kind]

Because of the above issues, this code is somewhat untested. I would like to expand the available integration tests, but for now, the code is testable, with the note that some beat events will not be sent to the shipper due to elastic/beats#34319.

How to Test

In order to test, you can run elastic-agent in standalone mode with an output config like this:

outputs:
  default:
    log_level: debug
    type: elasticsearch
    hosts: [https://127.0.0.1:9200]
    #api-key: "example-key"
    username: elastic
    password: changeme
    shipper:
      enabled: true

You may also need to place the shipper binary and shipper.spec.yml file under the components/ subdirectory.

💚 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

Start Time: 2023-02-09T20:11:34.542+0000
Duration: 16 min 59 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

mergify · 2022-11-28T17:04:45Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b setup-agent-config upstream/setup-agent-config
git merge upstream/main
git push upstream setup-agent-config

cmacknz · 2022-11-29T01:02:42Z

Here's the draft PR for the same changes in Fleet: elastic/kibana#145755

cmacknz

First pass, still need to test this myself.

go.mod

config/config.go

cmacknz · 2023-01-25T02:35:17Z

controller/helpers.go

+	"github.com/elastic/elastic-agent-shipper/config"
+)
+
+// I am not net sure how this should work or what it should do, but we need to read from that error channel


You should log the error, it's a way for the agent to report failures making gRPC calls to somewhere that can log them.

Example where it is used https://github.com/elastic/elastic-agent-client/blob/4477e3ace394ef1abfec55afa5cc5e1f6f87980c/pkg/client/client_v2.go#L248-L252

Also reading this I realized that Beats doesn't do this elastic/beats#34381

controller/run.go

controller/server.go

fearful-symmetry · 2023-01-30T19:28:06Z

Okay, so, there's two ongoing issues related to this PR:

I'm currently unsure of how SetStrictMode will work. It should come from the input unit, but I have no way of actually verifying that, and this code currently has the assumption that we get it from the output unit, since that's where most of the config is coming from. I'll file an issue about this after the PR is merged, but for now I'm assuming that it will be need to be set later by a different subsystem.
There's tons code that will need to be changed once we address the issue with how we want split configs across different units. For now, there's definitely some awkwardness in how the GRPC endpoint is being started/stopped that I'm assuming will go away once we have a dedicated input unit.

cmacknz · 2023-01-30T20:00:55Z

I'm currently unsure of how SetStrictMode will work. It should come from the input unit, but I have no way of actually verifying that, and this code currently has the assumption that we get it from the output unit, since that's where most of the config is coming from.

SetStrictMode is a development only option. We can consider removing it if it is adding complexity, or perhaps only support it when the config is loaded from a file if that is possible.

cmacknz

Next round of comments, many of them about testing.

config/config.go

controller/units.go

cmacknz · 2023-02-01T20:19:38Z

controller/run.go

@@ -31,15 +43,27 @@ func LoadAndRun() error {
 }

 // RunUnmanaged runs the shipper out of a local config file without using the control protocol.
-func RunUnmanaged(cfg config.ShipperConfig) error {
+func RunUnmanaged(cfg config.ShipperRootConfig) error {


Do we have any tests for running in unmanaged mode? What is enforcing that it keeps working?

Yah, I've been deprioritizing it a bit, since it took me so long to just get the shipper talking to other components. Will see if I can add a test or two, at least...

controller/run.go

grpcserver/inputhandler.go

grpcserver/server.go

cmacknz · 2023-02-01T20:23:55Z

grpcserver/server_test.go

@@ -408,3 +405,7 @@ func (p *publisherMock) TryPublish(event *messages.Event) (queue.EntryID, error)
 func (p *publisherMock) PersistedIndex() (queue.EntryID, error) {
 	return p.persistedIndex, nil
 }
+
+func (p *publisherMock) IsInitialized() bool {
+	return true


There should probably be a test of the case where this is false.

fearful-symmetry · 2023-02-02T00:19:49Z

Okay, updating this as I go, in case more people want to look at it between now and tomorrow. Biggest thing that still needs to be done is expanding the tests.

fearful-symmetry · 2023-02-03T02:22:27Z

Okay, added a bunch of tests. @cmacknz going through and verifying agent behavior, then writing tests, is proving to be a bit of a laborious process, if we're worried about blocking the performance testing, we might want to decide what cases we want to be handled here, and what we want to add in a followup PR.

config/config.go

leehinman · 2023-02-06T18:15:09Z

controller/controller_client.go

+	// figure out if we need to initialize the gRPC endpoint
+	// TODO: this is another thing that will change with https://github.com/elastic/elastic-agent-shipper/issues/225,
+	// As we'll have a dedicated unit for the input, and we won't be using random input unit updates to see if we need to update the gRPC endpoint.
+	c.startgRPC(unit, conn)


They could change over the lifetime of the shipper. If for example a CA or server certificate expires and needs to be updated. But I think in general, both the server side and output side should expect to be restarted independently.

cmacknz · 2023-02-06T21:00:05Z

@cmacknz going through and verifying agent behavior, then writing tests, is proving to be a bit of a laborious process, if we're worried about blocking the performance testing, we might want to decide what cases we want to be handled here, and what we want to add in a followup PR.

I would rather only do the performance tests once we are convinced this implementation is correct.

fearful-symmetry · 2023-02-08T00:11:48Z

Alright, we should be covering all the scenarios we want in testing now. Going to do a few more tests against actual elastic-agent tomorrow, just to make sure I didn't miss anything.

fearful-symmetry · 2023-02-08T17:45:30Z

Alright, tested with elastic-agent and flipping various units on and off. With all the bugs in the shipper output/ES components, actual end-to-end testing is sort of a non-starter, but most of the fixes with that are in-progress.

cmacknz

A couple of mostly minor comments, this is looking good.

config/config.go

controller/controller_client.go

controller/helpers.go

controller/run.go

controller/units.go

fearful-symmetry added 3 commits November 16, 2022 16:29

handle output config, refactor

21c8fb2

clean up, tinkering with inputs

a02055b

Merge remote-tracking branch 'upstream/main' into setup-agent-config

2c68e8e

fearful-symmetry added enhancement New feature or request Team:Elastic-Agent Label for the Agent team v8.6.0 labels Nov 17, 2022

fearful-symmetry self-assigned this Nov 17, 2022

fearful-symmetry requested a review from a team as a code owner November 17, 2022 22:27

fearful-symmetry requested review from faec and leehinman and removed request for a team November 17, 2022 22:27

fearful-symmetry marked this pull request as draft November 17, 2022 22:27

fearful-symmetry added 4 commits November 18, 2022 11:29

update go deps

486b66a

linter

e3d16f6

add headers

ef4772c

update notice

9746559

leehinman mentioned this pull request Dec 9, 2022

[Meta] elastic-agent-shipper journey to GA #197

Open

54 tasks

fearful-symmetry added 4 commits January 19, 2023 13:47

continue to sync shipper with agent

fe7e4f8

Merge remote-tracking branch 'upstream/main' into setup-agent-config

97d37f8

try to make linter happy

4d17c78

fix tests

a81454c

fearful-symmetry marked this pull request as ready for review January 24, 2023 19:32

add more comments, logic for unit state

9d1a866

cmacknz reviewed Jan 25, 2023

View reviewed changes

fearful-symmetry added 2 commits January 25, 2023 11:13

update deps, clean up logging

2e269fe

still tinkering with logging

2a4f57d

fearful-symmetry mentioned this pull request Feb 1, 2023

Enable TLS by default in shipper output elastic/beats#34425

Closed

fearful-symmetry added 3 commits February 1, 2023 10:41

change config unpacking

f197d39

update deps

bcc639b

update notice

9e78d98

cmacknz reviewed Feb 1, 2023

View reviewed changes

change comments, adjust unit map behavior, add tests

7de6b9b

fearful-symmetry mentioned this pull request Feb 2, 2023

Shipper output producing events that get rejected with illegal_argument_exception elastic/beats#34449

Closed

fearful-symmetry added 2 commits February 1, 2023 16:25

lint

9aa0b34

add tests, change unit map behavior

35b9107

tinker with tests

bf5fd83

leehinman reviewed Feb 6, 2023

View reviewed changes

fearful-symmetry added 2 commits February 6, 2023 12:05

tinkering with tests, still

b76df30

clean up

4d75339

fearful-symmetry added 2 commits February 6, 2023 13:53

add tests

e69f074

add more tests

5a5f835

fearful-symmetry requested review from leehinman and cmacknz February 8, 2023 16:23

leehinman approved these changes Feb 8, 2023

View reviewed changes

cmacknz reviewed Feb 8, 2023

View reviewed changes

fearful-symmetry added 2 commits February 8, 2023 13:10

add comments, rename unit methods

511c999

change log change logic

5fc5662

cmacknz approved these changes Feb 9, 2023

View reviewed changes

fearful-symmetry merged commit aa99fdd into elastic:main Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the shipper to work with the elastic-agent #185

Update the shipper to work with the elastic-agent #185

fearful-symmetry commented Nov 17, 2022 •

edited

Loading

mergify bot commented Nov 17, 2022

elasticmachine commented Nov 17, 2022 •

edited

Loading

Build stats

mergify bot commented Nov 28, 2022

cmacknz commented Nov 29, 2022

cmacknz left a comment

cmacknz Jan 25, 2023

cmacknz Jan 25, 2023

cmacknz Jan 25, 2023

fearful-symmetry commented Jan 30, 2023

cmacknz commented Jan 30, 2023

cmacknz left a comment •

edited

Loading

cmacknz Feb 1, 2023

fearful-symmetry Feb 2, 2023

cmacknz Feb 1, 2023

fearful-symmetry commented Feb 2, 2023 •

edited

Loading

fearful-symmetry commented Feb 3, 2023

leehinman Feb 6, 2023

cmacknz commented Feb 6, 2023

fearful-symmetry commented Feb 8, 2023

fearful-symmetry commented Feb 8, 2023

cmacknz left a comment

Update the shipper to work with the elastic-agent #185

Update the shipper to work with the elastic-agent #185

Conversation

fearful-symmetry commented Nov 17, 2022 • edited Loading

What does this PR do?

How to Test

Other open Questions:

mergify bot commented Nov 17, 2022

elasticmachine commented Nov 17, 2022 • edited Loading

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

mergify bot commented Nov 28, 2022

cmacknz commented Nov 29, 2022

cmacknz left a comment

Choose a reason for hiding this comment

cmacknz Jan 25, 2023

Choose a reason for hiding this comment

cmacknz Jan 25, 2023

Choose a reason for hiding this comment

cmacknz Jan 25, 2023

Choose a reason for hiding this comment

fearful-symmetry commented Jan 30, 2023

cmacknz commented Jan 30, 2023

cmacknz left a comment • edited Loading

Choose a reason for hiding this comment

cmacknz Feb 1, 2023

Choose a reason for hiding this comment

fearful-symmetry Feb 2, 2023

Choose a reason for hiding this comment

cmacknz Feb 1, 2023

Choose a reason for hiding this comment

fearful-symmetry commented Feb 2, 2023 • edited Loading

fearful-symmetry commented Feb 3, 2023

leehinman Feb 6, 2023

Choose a reason for hiding this comment

cmacknz commented Feb 6, 2023

fearful-symmetry commented Feb 8, 2023

fearful-symmetry commented Feb 8, 2023

cmacknz left a comment

Choose a reason for hiding this comment

fearful-symmetry commented Nov 17, 2022 •

edited

Loading

elasticmachine commented Nov 17, 2022 •

edited

Loading

cmacknz left a comment •

edited

Loading

fearful-symmetry commented Feb 2, 2023 •

edited

Loading