Skip to content
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

Ta https server #2921

Merged
merged 7 commits into from
May 8, 2024
Merged

Conversation

ItielOlenick
Copy link
Contributor

@ItielOlenick ItielOlenick commented May 1, 2024

Description:
Added an additional HTTPS server with mTLS to serve scrape_config with secret values. This resolves one part of the issue. An additional PR will be opened for the generation and mounting of certs.

Link to tracking Issue(s):

Testing:
Tested in cluster with self-issued certificates. Successfully retrieved the redacted scrape_config using the existing HTTP server and obtained the scrape_config with actual secret values from the new HTTPS server.

Documentation:
Not yet added. Will be added once the entire feature is available.

@ItielOlenick ItielOlenick requested review from a team May 1, 2024 21:33
cmd/otel-allocator/server/server.go Outdated Show resolved Hide resolved
cmd/otel-allocator/main.go Outdated Show resolved Hide resolved
cmd/otel-allocator/server/server.go Outdated Show resolved Hide resolved
cmd/otel-allocator/server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Left some comments, but overall this looks good to me. @jaronoff97 can you have a look as well?

cmd/otel-allocator/config/config.go Outdated Show resolved Hide resolved
cmd/otel-allocator/config/config.go Outdated Show resolved Hide resolved
cmd/otel-allocator/main.go Outdated Show resolved Hide resolved
cmd/otel-allocator/main.go Outdated Show resolved Hide resolved
@swiatekm swiatekm requested review from swiatekm and jaronoff97 May 7, 2024 17:54
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution and patience with the review process.

@ItielOlenick
Copy link
Contributor Author

LGTM! Thank you for your contribution and patience with the review process.

Sure thing.
Perhaps for the rest of the changes we can have a short discussion about the implementation? Specifically cret management.

@swiatekm
Copy link
Contributor

swiatekm commented May 8, 2024

LGTM! Thank you for your contribution and patience with the review process.

Sure thing. Perhaps for the rest of the changes we can have a short discussion about the implementation? Specifically cret management.

Sure, let's do that in the issue. On that note, can you edit your PR description so Github doesn't auto-close the issue when this PR is merged?

@@ -189,6 +199,20 @@ func main() {
setupLog.Error(shutdownErr, "Error on server shutdown")
}
})
if cfg.HTTPS.Enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to put the above srv.Start() in an else block or are we okay to potentially run both a secure and insecure simultaneously? I guess doing it this way would make for a better migration from http to https.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on previous conversation, it was suggested to have both HTTP and HTTPS running at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the idea is to keep both and only serve secrets over HTTPS. The HTTP service is useful for debugging, for HTTPS you need to do some work to set up mTLS locally.

@jaronoff97
Copy link
Contributor

looks great! thank you for your contribution! 🙇

@jaronoff97 jaronoff97 merged commit 5d51393 into open-telemetry:main May 8, 2024
33 checks passed
@ItielOlenick ItielOlenick deleted the ta-https-server branch May 8, 2024 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants