-
Notifications
You must be signed in to change notification settings - Fork 124
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
[ID] httpapi: add https and mTLS support to ingest server #913
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice refactor along the way! thanks ;-)
ddf1ebd
to
f77bed5
Compare
Some updates to this PR:
|
@@ -28,60 +31,73 @@ const ( | |||
statusAPIPathReady = "/v1/status/ready" | |||
ingestAPIPath = "/v1/data" | |||
ingestAPIPathReady = "/v1/data/ready" | |||
readinessProbeRetryBackoff = 10 * time.Millisecond | |||
readinessProbeRetryBackoff = 100 * time.Millisecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, why increase it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a bit of a sneaky change, sorry about that! The main motivation was that during tests, the probing mechanism was consuming quite a bunch of CPU. Since we're waiting for network requests to succeed, the 100ms backoff did not seem that much. I can always revert it back to 10ms if you prefer it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just a few typos and 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💪
Tests seem to be entering into some sort of deadlock and timing out :( |
fixup! make serverand status configs not pointers
If URL is https, mTLS might be enabled. For simplicity, readiness checker does not supply TLS certs, so we must ignore a possible 400 Bad Request being ignored by an HTTPs server.
fixup! readiness fixes
change Error to Fatal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 💪
This PR adds support for TLS on the Ingest server from the
httpapi
package. Server can be configured to listen in three modes:This feature, provided specific configuration, could allow to safely expose the infra-agent HTTP ingest endpoint in less trusted networks, such as a Kubernetes cluster.
Reviewing
As multiple files have been changed to make this feature available, I suggest to review commit by commit.