V2 Control Protocol #29

blakerouse · 2022-03-15T12:19:34Z

V2 Control Protocol

This is a proposal on the new design of the control protocol.

Features

A unique state/config per unit (multiple inputs and output per process).
A similar actions protocol (only added 1 extra field).
A key-value storage scoped per unit for persistent storage.
Access to the artifacts API through the control protocol.
Ability to report name, version and metadata about the program to Elastic Agent.
Ability to write log messages from processes that are not started as sub-processes.

Backward Compatibility

This adds a new CheckinV2 that is to be used when the Elastic Agent is operating in V2 mode. The original Checkin will be deprecated at some point in the future (once all tools have migrated off).

Closes elastic/elastic-agent#215

blakerouse · 2022-03-15T12:21:36Z

We need to decide on how we want to merge this into the repository. I was thinking about making it github.com/elastic/elastic-agent-client/v2 keeping the current client as github.com/elastic/elastic-agent-client.

elasticmachine · 2022-03-15T12:22:34Z

💚 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: 2022-05-19T13:38:20.114+0000
Duration: 3 min 37 sec

🤖 GitHub comments

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

/test : Re-trigger the build.

ph · 2022-03-15T12:33:19Z

@blakerouse I think we should work the same as our other project, something like:

create a 1.x branch.
Tag the current code for v1.0.0 ?
Update the project to use that version.
Use main for v2?

cmacknz · 2022-03-15T13:15:53Z

Use main for v2?

This will work but I've usually seen public proto packages versioned like Go packages, where v1 and v2 coexist together in the tree until one obsoletes the other. Like Go modules, this will make the import path change between versions. You could use package proto; and package proto.v2; for example and do the same on the generated Go modules.

Here is Google Cloud Storage using this approach: https://github.com/googleapis/googleapis/tree/master/google/storage

ph · 2022-03-15T13:19:42Z

@cmacknz good point, but at the same time we have a limited set of users, so I am not sure having a v2 worth it. I believe this would reduce our flexibility to make changes in repo/testing and so on.

blakerouse · 2022-03-15T14:27:32Z

I am +1 for using main for v2.

cmacknz · 2022-03-15T15:07:44Z

@cmacknz good point, but at the same time we have a limited set of users, so I am not sure having a v2 worth it. I believe this would reduce our flexibility to make changes in repo/testing and so on.

Fair point, go with whatever makes development easier.

elastic-agent-client.proto

blakerouse · 2022-03-15T15:24:33Z

@jlind23 Had a good idea in our 1-1 to instead of making a whole v2, that we instead try to make this work with the existing v1 instead of a hard breaking change.

He had some good points of allowing things like Endpoint Security and APM server to migrate slower to v2. I think this is a valid point and something that we could do in this protocol.

The change only adds 1 field to Actions stream so we can easily add that and pass it to current processes and they will ignore it (which is going to be okay, until they support the v2 protocol anyway).

The new additional functions are not streaming protocols that are even required for processes to even use, so if they don't use them that is fine and not breaking.

The largest change is the Checkin stream, because of how state and configuration is changed. The idea I have is to keep the Checkin stream as it is today and then add a CheckinV2 stream. A newer process will connect to V2 and an older one that has not migrated will continue to connect to old Checkin. We then deprecate the old Checkin and we give a EOL for that Checkin in some future 8.x version.

elastic-agent-client.proto

ph · 2022-03-15T16:28:28Z

@blakerouse The v1,v2 I am worried to run in a hybrid mode, wouldn't that bring more complexity while debugging?

jlind23 · 2022-03-15T16:48:52Z

@ph we will be aware when agent will be running in V2 mode, we just have to properly log it then.

blakerouse · 2022-03-15T16:54:45Z

@ph Really we have 2 different ways we could implement it.

Run in v2 mode explicitly, so even though the Checkin function is present, we do not allow connection to it over the GRPC. Elastic Agent will error to the client that its in v2 mode and must connect over CheckinV2.
Run in hybrid mode that allows a v1 client but it translates to a v2 operating mode. So Elastic Agent runs in v2 mode, it computes the multiple units per process and builds the runtime environment. Then that v2 runtime environment is then merged to send a single configure to a v1 client, where the status is duplicated across the v2 runtime. This gives a temporary translation layer for v1 clients.

The open question is really do we want to support existing v1 clients when operating as v2, if so #2. Or we don't need too and #1 could be the answer. Or option #3, start with #1 and if we see an issue that requires us to do #2 then we implement the translation layer.

elastic-agent-client.proto

AndersonQ · 2022-03-16T15:11:16Z

elastic-agent-client.proto

+    //
+    // Transactional store is provided to allow multiple key operations to occur before a commit to ensure consistent
+    // state when multiple keys make up the state of an units persistent state.
+    rpc StoreBeginTxn(StoreBeginTxnRequest) returns (StoreBeginTxnResponse);


Question here: must all the store methods be called within a transaction, or could the Get|Set|Delete be called alone?

Must be in a transaction.

elastic-agent-client.proto

Co-authored-by: Anderson Queiroz <contato@andersonq.eti.br>

ph · 2022-03-17T14:44:36Z

@blakerouse I think what you are proposing in CheckV2, is the appropriate path. 1 to see if we need to support 2. I am considering that most processes would be fine with the new protocol because they are beats. The endpoint is a bit different, but and they do not need to adopt the Artifact service right away.

Or option 3, start with 1 and if we see an issue that requires us to do 2 then we implement the translation layer.

aleksmaus · 2022-05-17T19:52:10Z

elastic-agent-client.proto

+    //
+    // Transactional store is provided to allow multiple key operations to occur before a commit to ensure consistent
+    // state when multiple keys make up the state of an units persistent state.
+    rpc BeginTxn(StoreBeginTxnRequest) returns (StoreBeginTxnResponse);


nit: maybe rename BeginTxn -> BeginTx and other Txns to Txs?
similar to postgres go library for example BeginTx
https://github.com/lib/pq/blob/1ef134dc0e0dcdf8097ca347580c15df72ef786e/conn_go18.go#L58
or mysql
https://github.com/go-sql-driver/mysql/blob/bcc459a906419e2890a50fc2c99ea6dd927a88f2/connection.go#L471

"Tx" is more common abbreviation for transactions in other db libaries

Is there any handling for example for the broken connection after the transaction was started and left uncommitted? @blakerouse

@aleksmaus I would expect the method call to return an error when the method is called?

depending on the storage backend/library and transaction implementation details, not closing the transaction could leave the transaction locks on the database/tables/rows. should probably be coded to have transaction in the scope of the connection or something and the connection is broken the transaction should be auto rolled back. thinking if these kind of cases. don't completely know if this is relevant for our particular implementation.

@aleksmaus the state of the connection will be tracked on the server-side (aka. Elastic Agent), when the connection is lost then the transaction will be discarded server-side. At that point that transaction is broken and they client will need to handle the error and create a new transaction

I also think Elastic Agent should place a hard timeout on transactions, as it would not be good to keep them open for a long period of time. They really should be short in time.

I will go through and adjust it all from "Txn" abbr. to "Tx".

aleksmaus · 2022-05-17T19:57:23Z

pkg/client/client_v2.go

+	cfgLock sync.RWMutex
+	obsLock sync.RWMutex
+
+	kickChan  chan struct{}


nit: maybe rename kickChan ->kickCh etc for other chan vars. This naming convention seems to be more common in https://github.com/golang/go

aleksmaus · 2022-05-17T19:57:50Z

pkg/client/client_v2.go

+	kickChan  chan struct{}
+	errChan   chan error
+	unitsChan chan UnitChanged
+	unitsLock sync.RWMutex


nit: maybe rename unitsLock -> unitsMu. This naming convention seems to be more common in https://github.com/golang/go

aleksmaus · 2022-05-17T20:06:39Z

elastic-agent-client.proto

+    //
+    // Transactional store is provided to allow multiple key operations to occur before a commit to ensure consistent
+    // state when multiple keys make up the state of an units persistent state.
+    rpc BeginTxn(StoreBeginTxnRequest) returns (StoreBeginTxnResponse);


Is there any handling for example for the broken connection after the transaction was started and left uncommitted? @blakerouse

ph · 2022-05-18T13:15:14Z

elastic-agent-client.proto

+// Response from `SetKey`.
+message StoreSetKeyResponse {
+    // Empty at the moment, defined for possibility of adding future return values.
+}


Good catch, I see a TTL maybe or to indicate single-use key.

ph · 2022-05-18T13:16:20Z

elastic-agent-client.proto

+//
+// `txn_id` must be an ID of a transaction that was started with `READ_WRITE`.
+//
+// Does not error in the case that a key does not exist.


+1, align with Go's expected behavior.

pzl · 2022-05-18T13:27:38Z

@blakerouse @ph

What's the procedure look like for additions to this protocol in the future? If the changes are purely additive to the control protocol, can things be added to this spec in, say, 8.4 timeline?

We are still looking to add a new feature/API to Fleet Server for our next use case, but we don't have a full picture of what's being sent over the wire (aside from, its a big file. Not sure if streamed or pre-chunked. Metadata format, etc)

I assume you want this merged without waiting for us? Or is it much preferred to hammer out something on our side and send it in here

ph · 2022-05-18T13:28:28Z

elastic-agent-client.proto

+
+message LogMessageResponse {
+    // Empty at the moment, defined for possibility of adding future return values.
+}


+1 for forward change.

ph · 2022-05-18T16:56:58Z

pkg/client/client_v2.go

+		for {
+			expected, err := checkinClient.Recv()
+			if err != nil {
+				if err != io.EOF {


This would fail the linter.

Suggested change

if err != io.EOF {

if !errors.Is(err, io.EOF) {

ph · 2022-05-18T16:57:47Z

pkg/client/client_v2.go

+			return
+		}
+
+		t := time.NewTicker(c.minCheckTimeout)


Based on our morning discussion this would behavior weirdlky with sleep or hibernate?

After looking at the golang I do not believe this to be an issue. The ticker will keep ticking, on hibernate it will just pickup where it left off because it uses the monoclock. The ticker also handles a slow reader so it never adds more than one to the channel so until the next is removed from the channel it will not add another one.

Thanks for the verification!

ph · 2022-05-18T16:58:18Z

pkg/client/client_v2.go

+		defer checkinWrite.Done()
+
+		if err := c.sendObserved(checkinClient); err != nil {
+			if err != io.EOF {


Suggested change

if err != io.EOF {

if !errors.Is(err, io.EOF) {

ph · 2022-05-18T17:02:22Z

pkg/client/client_v2.go

+			}
+		}
+	}
+	c.units = c.units[:i]


I've almost missed the resize.

ph · 2022-05-18T17:06:10Z

pkg/client/client_v2.go

+					Token:  c.token,
+					Id:     action.Id,
+					Status: proto.ActionResponse_FAILED,
+					Result: ActionTypeUnknown,


ph · 2022-05-18T17:08:31Z

pkg/client/client_v2.go

+			Result: ActionErrUnitNotFound,
+		}
+		return
+	}


So this mean that If an agent receive an osquery and it receive an osquery action and the unit isn't runnning we would silently ignore the event?

Actually, not we would report back upstream, ok I think its fine.

ph

I've added a few questions in the PR, and so far it looks good to me.
I've been having a few difficulties concerning the Keystore API and implementation, but I find it hard without playing in the context of the agent.

Looking at other comments concerning renaming txn to tx I believe is a good thing to do.

ph · 2022-05-18T17:10:10Z

pkg/client/client_v2.go

+			Result: ActionErrUnmarshableParams,
+		}
+		return
+	}


thanks for adding this, +1 don't trust anything.

ph · 2022-05-18T17:14:26Z

pkg/client/client_v2_test.go

+						go func() {
+							time.Sleep(100 * time.Millisecond)
+							change.Unit.UpdateState(UnitStateStopped, "Stopped", nil)
+						}()


Should we make the "Healthy", "Stopping", "Stopped" and other a public constant?

For testing make it constant? For the library, no I would prefer they provide more detail than that. Like "Connected to elasticsearch(https://localhost:8200)" or something.

OK good point.

ph · 2022-05-18T17:14:48Z

pkg/client/client_v2_test.go

+		defer m.Unlock()
+
+		if !connected {
+			return fmt.Errorf("server never recieved valid token")


Suggested change

return fmt.Errorf("server never recieved valid token")

return fmt.Errorf("server never received a valid token")

ph · 2022-05-18T17:17:36Z

pkg/client/client_v2_test.go

+					units = append(units, change.Unit)
+					unitsLock.Unlock()
+				default:
+					panic("not implemented")


let's use testing's t.Fatal

Its in a seperate goroutine, go vet will not allow this.

call to (*T).Fatal from a non-test goroutine

ph · 2022-05-18T17:17:55Z

pkg/client/client_v2_test.go

+		defer m.Unlock()
+
+		if !gotInit {
+			return fmt.Errorf("server never recieved valid token")


Suggested change

return fmt.Errorf("server never recieved valid token")

return fmt.Errorf("server never received valid token")

ph · 2022-05-18T17:19:12Z

pkg/client/common.go

+	"github.com/elastic/elastic-agent-client/v7/pkg/utils"
+)
+
+// CheckinMinimumTimeout is the amount of time the client must send a new checkin even if the status has not changed.


See above discussion concerning hibernation and sleep.

Not an issue with the usage of ticker.

ph · 2022-05-18T17:24:07Z

pkg/client/common.go

+// where the action type is unknown.
+var ActionTypeUnknown = utils.JSONMustMarshal(map[string]string{
+	"error": "action type unknown",
+})


We should probably have a concrete type implementing MarshalJSON instead of creating JSON fragment, something like:

ActionTypeErr{ error: "action-type unknown" }

ph · 2022-05-18T17:30:30Z

pkg/client/store_test.go

+					unitsLock.Unlock()
+				default:
+					panic("not implemented")
+				}


ph · 2022-05-18T17:36:22Z

pkg/client/store_test.go

+		s.store = append(s.store, unitStore)
+	}
+	return unitStore
+}


Well, we have a full in-memory store :D

ph · 2022-05-18T17:37:49Z

pkg/client/unit.go

+
+	expLock   sync.RWMutex
+	exp       UnitState
+	config    string


This would be the string representation of the yaml, this seems asking for having his own type? We we can validate the presence of some fields depending on the unit or that we have valid yaml.

Sadly we can have a yaml.RawMessage because of the way indent works and could break the configuration.

Is it YAML, JSON, or something else. The point of the client is that it doesn't know and doesn't care. So its best to keep it just as a string representation that could then be decoded.

ph · 2022-05-19T13:23:59Z

@cmacknz Can you review this too, this directly impact the data plane?

blakerouse · 2022-05-19T13:38:22Z

@aleksmaus @ph Made the requested changes.

ph

LGTM, Let's get another approval from @elastic/elastic-agent-data-plane

cmacknz · 2022-05-19T16:13:01Z

elastic-agent-client.proto

@@ -25,6 +41,8 @@ message ConnInfo {
    bytes peer_cert = 5;
    // Peer private key.
    bytes peer_key = 6;
+    // Allowed services that spawned process can use. (only used in V2)
+    repeated ConnInfoServices services = 7;


What is it expected that processes do with the services list? Is the intent that each process checks to make sure the services it needs are actually available?

Correct. A process can only use the services that it is given, if it tries to Elastic Agent will return an error back to the caller.

cmacknz · 2022-05-19T17:34:07Z

pkg/client/client_v2.go

+}
+
+// Errors returns channel of errors that occurred during communication.
+func (c *clientV2) Errors() <-chan error {


Can you document that failing to read Errors() or UnitChanges() will cause the client to block internally?

It is documented on the interface.

cmacknz

LGTM, I expect we will have more feedback once we actually try to use the new V2 client :)

ph · 2022-05-19T19:28:19Z

@cmacknz I also change when we try the real world.

fearful-symmetry · 2022-05-23T19:47:56Z

Agreed on real-world testing, I'm honestly kinda planning with the assumption that a lot of the APIs are gonna change over time as we actually use it.

* Move httpcommon from libbeat * updates to satisfy linter

Add proposed v2 protocol.

0b67b37

blakerouse added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Mar 15, 2022

blakerouse self-assigned this Mar 15, 2022

jlind23 requested review from AndersonQ, lykkin, narph, ph and michel-laterman March 15, 2022 14:39

cmacknz reviewed Mar 15, 2022

View reviewed changes

elastic-agent-client.proto Show resolved Hide resolved

cmacknz reviewed Mar 15, 2022

View reviewed changes

elastic-agent-client.proto Outdated Show resolved Hide resolved

cmacknz reviewed Mar 15, 2022

View reviewed changes

elastic-agent-client.proto Outdated Show resolved Hide resolved

ruflin reviewed Mar 16, 2022

View reviewed changes

elastic-agent-client.proto Show resolved Hide resolved

elastic-agent-client.proto Show resolved Hide resolved

AndersonQ reviewed Mar 16, 2022

View reviewed changes

cmacknz reviewed Mar 16, 2022

View reviewed changes

elastic-agent-client.proto Outdated Show resolved Hide resolved

blakerouse and others added 3 commits March 17, 2022 09:08

Update elastic-agent-client.proto

c4fd96f

Co-authored-by: Anderson Queiroz <contato@andersonq.eti.br>

Update elastic-agent-client.proto

57b5835

Co-authored-by: Anderson Queiroz <contato@andersonq.eti.br>

Update elastic-agent-client.proto

15380cf

Co-authored-by: Anderson Queiroz <contato@andersonq.eti.br>

ph mentioned this pull request Mar 17, 2022

[Design] Elastic Agent Control Protocol v2 gRPC. elastic/elastic-agent#215

Closed

15 tasks

aleksmaus reviewed May 17, 2022

View reviewed changes

ph requested review from ph and removed request for lykkin May 18, 2022 13:00

ph reviewed May 18, 2022

View reviewed changes

kvch mentioned this pull request May 18, 2022

[Meta] Elastic Agent Inputs elastic/elastic-agent-inputs#1

Closed

33 tasks

ph reviewed May 18, 2022

View reviewed changes

pkg/client/client_v2.go

}

}

}

c.units = c.units[:i]

Copy link

Contributor

ph May 18, 2022

Choose a reason for hiding this comment

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

I've almost missed the resize.

ph reviewed May 18, 2022

View reviewed changes

pkg/client/client_v2.go

Token: c.token,

Id: action.Id,

Status: proto.ActionResponse_FAILED,

Result: ActionTypeUnknown,

Copy link

Contributor

ph May 18, 2022

Choose a reason for hiding this comment

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

+1

ph reviewed May 18, 2022

View reviewed changes

ph suggested changes May 18, 2022

View reviewed changes

cmacknz requested a review from andrewkroh May 18, 2022 19:58

ph requested a review from cmacknz May 19, 2022 13:23

Changes from code review.

0323411

ph approved these changes May 19, 2022

View reviewed changes

cmacknz reviewed May 19, 2022

View reviewed changes

cmacknz approved these changes May 19, 2022

View reviewed changes

blakerouse merged commit 43bacbe into elastic:main May 24, 2022

blakerouse deleted the v2-protocol branch May 24, 2022 13:19

v1v pushed a commit to v1v/elastic-agent-client that referenced this pull request Sep 5, 2022

Move libbeat/common/transport/httpcommon (elastic#29)

6a4dec0

* Move httpcommon from libbeat * updates to satisfy linter

	return fmt.Errorf("server never recieved valid token")
	return fmt.Errorf("server never received a valid token")

V2 Control Protocol #29

V2 Control Protocol #29

Conversation

blakerouse commented Mar 15, 2022 • edited Loading