Add a config endpoint on a new api server #21025
Conversation
Bloop Bleep... Dogbot HereRegression Detector ResultsRun ID: cdc6290e-71b5-4624-87c4-314d290a4a33 Explanation A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI". For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
No interesting changes in experiment optimization goals with confidence ≥ 90.00% and |Δ mean %| ≥ 5.00%. Experiments that were not declared erratic but were detected as being so, coefficient of variation cutoff 0.10:
Experiments where analysis encountered missing or malformed data: dogstatsd_string_interner_128MiB_100 Usually, this warning means that there is no usable optimization goal data for that experiment, which could be a result of misconfiguration. Fine details of change detection per experiment.
|
cmd/agent/api/server.go
Outdated
| tlsListener := tls.NewListener(listener, srv.TLSConfig) | ||
| if apiConfigEnabled { | ||
| if err := startIPCServer(apiConfigHostPort, tlsConfig); err != nil { | ||
| StopServer() |
There was a problem hiding this comment.
Do you need to call StopServer when startIPCServer fail? Ideally when startIPCServer returns an error, startIPCServer should release any resources allocated if any.
There was a problem hiding this comment.
Also, the fact that we are using global variables cmdListener and ipcConfigListener makes it more difficult to grok what is going on. Have you consider moving away from global variables and having server struct that holds cmdListener and ipcConfigListener information? That way, we could have functions like StopCMDServer and StopIPCServer , the same for startCMDServer and startIPCServer defined as part of the server struct
There was a problem hiding this comment.
I know it was previously using var listener net.Listener but it might be a good opportunity to refactor such pattern 😄
There was a problem hiding this comment.
@ogaca-dd I need to call StopServer if startIPCServer fails because the other server was started beforehand, and needs to be stopped. I added some comments to make that clearer.
@GustavoCaso This would be implemented in the comp PR, in this one I try to avoid changing things I don't need to
There was a problem hiding this comment.
comp PR
You mean the API component PR?
There was a problem hiding this comment.
Yes, the componentization of the API will likely remove these global variables
| apiAddr, tlsConfig, tlsCertPool, | ||
| configService, flare, dogstatsdServer, | ||
| capture, serverDebug, wmeta, logsAgent, | ||
| senderManager, hostMetadata, invAgent, | ||
| demux, invHost, secretResolver, |
There was a problem hiding this comment.
It may not be a convention in Go, but I see having too many parameters/arguments in a function as a code smell.
Maybe a suggestion for the CMD server could be its struct that we initialize and later call startServer or something. I also understand it might not be the right suggestion for this particular use case. At least we can start a conversation 😄
There was a problem hiding this comment.
I definitely agree, IMO the API should at some point become a generic components others could register endpoints into, but this would be part of an eventual refactor of the API component
There was a problem hiding this comment.
FWIW, all these arguments will be removed in the near future as we'll embed them in the API Component directly (they will be injected to the API component through Fx)
See comment here:
datadog-agent/comp/api/api/component.go
Line 32 in b1ec829
| func initializeTLS() error { | ||
| cert, key := buildSelfSignedKeyPair() | ||
| func initializeTLS(additionalHostIdentities ...string) (*tls.Certificate, *x509.CertPool, error) { | ||
| log.Info("Initializing TLS certificates") |
There was a problem hiding this comment.
nit: Related to the original comment around logging, it would be good to identify what TLS certificates we're initializing, probably by the caller ID/name so that we know the requestor of the cert. I know we print out the hosts with the name in securiity.go but that log line could be far away from this one in the output.
There was a problem hiding this comment.
I'll add that in a separate PR where we can discuss what kind of information we want around TLS initialization
| ctx, gwmux, tlsAddr, dopts) | ||
| if err != nil { | ||
| return fmt.Errorf("error registering agent handler from endpoint %s: %v", tlsAddr, err) | ||
| if err := util.CreateAndSetAuthToken(); err != nil { |
There was a problem hiding this comment.
Do we have any tests around the token validation on either the old server code or the new?
There was a problem hiding this comment.
Not that I know of, I'll open a separate PR to fix that (I would like to merge this one as it's blocking other PRs)
Co-authored-by: Srdjan Grubor <[email protected]>
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. This build is going to start soon! (estimated merge in less than 45m) you can cancel this operation by commenting your pull request with |
dd-devflow
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
dd-devflow
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
What does this PR do?
Add a config endpoint to query config values on a new api server (on a new host and port).
Motivation
Eventually allow synchronizing config values (which can be dynamic with RC) across agent processes.
Additional Notes
The new server is on a different host and port than the existing cmd api server, which means we have to create an entirely new http server.
The security of the server is the same as the one of the usual API server.
I renamed a few functions to factor both servers and make the difference clearer.
I used the same cert for both servers, but we can easily refactor it to use separate ones, it might be better if we want both servers to be completely separate.
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
Enable the endpoint by setting a non zero value to
agent_ipc_port(eg. 5004).Check that the original API server still works, and that the new api server works properly to fetch
api_key.Only
api_keyis allowed to be queried as of now, so check that anything else returns an error.The following command queries the endpoint from CLI:
Check that expvars are properly filled, either with flare or with the endpoint directly:
Reviewer's Checklist
Triagemilestone is set.major_changelabel if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.changelog/no-changeloglabel has been applied.qa/skip-qalabel is not applied.team/..label has been applied, indicating the team(s) that should QA this change.need-change/operatorandneed-change/helmlabels have been applied.k8s/<min-version>label, indicating the lowest Kubernetes version compatible with this feature.