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

Add the httpsprovider component #6683

Merged
merged 12 commits into from
Jan 25, 2023

Conversation

rapphil
Copy link
Contributor

@rapphil rapphil commented Dec 5, 2022

Description: Add the httpsprovider component that allows the collector to load configuration from HTTP servers using the HTTPS protocol.

This PR is based on this work junhaoyu-aws#4

A bit of refactoring was made and an internal configurablehttpprovider was created. This component is used by both the httpsprovider and the httpprovider. The only difference is the configuration of the transport layer.

Testing: Unit tests using httptest and self signed certificates were added.

Documentation: A readme was added for the httpsprovider component.

@rapphil rapphil changed the title Add support to https in the httpprovider confmap provider Add support to https in the httpprovider Dec 5, 2022
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Base: 90.38% // Head: 90.38% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e31b142) compared to base (2b96397).
Patch coverage: 90.62% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6683   +/-   ##
=======================================
  Coverage   90.38%   90.38%           
=======================================
  Files         243      245    +2     
  Lines       14575    14638   +63     
=======================================
+ Hits        13173    13230   +57     
- Misses       1133     1139    +6     
  Partials      269      269           
Impacted Files Coverage Δ
...ider/internal/configurablehttpprovider/provider.go 90.00% <90.00%> (ø)
confmap/provider/httpprovider/provider.go 100.00% <100.00%> (+12.00%) ⬆️
confmap/provider/httpsprovider/provider.go 100.00% <100.00%> (ø)
otelcol/configprovider.go 79.06% <100.00%> (ø)
featuregate/registry.go 84.21% <0.00%> (-15.79%) ⬇️
extension/extension.go 97.61% <0.00%> (-2.39%) ⬇️
exporter/exporter.go 99.00% <0.00%> (-1.00%) ⬇️
receiver/receiver.go 99.00% <0.00%> (-1.00%) ⬇️
processor/processor.go 99.00% <0.00%> (-1.00%) ⬇️
internal/obsreportconfig/obsreportconfig.go 93.33% <0.00%> (-0.61%) ⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rapphil rapphil marked this pull request as ready for review December 5, 2022 21:08
@rapphil rapphil requested review from a team and jpkrohling December 5, 2022 21:08
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

Having read through the code a bit, I would prefer the optional settings to be configurable via flags like:

otelcol --tls-ca-cert-path=file://infra/testinfra:named=testenv 
   --tls-ca-cert-path=file://infra/prodinfra:named=prodenv 
   --config=https://internal.infra.io/config:tls-bundle=testenv

However, I am not sure how feasible that might be.

confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider_test.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/README.md Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
@mx-psi
Copy link
Member

mx-psi commented Jan 4, 2023

From the SIG, let's have a first PR with an https provider without any configurable parameters and move the discussion on how to configure a confmap provider to an issue.

@rapphil rapphil force-pushed the rapphil-add-https-confimap branch from 04bc6e3 to 9d3e66a Compare January 5, 2023 16:44
@rapphil rapphil changed the title Add support to https in the httpprovider Add the httpsprovider component Jan 5, 2023
@rapphil rapphil force-pushed the rapphil-add-https-confimap branch from 29723df to 9d804d8 Compare January 5, 2023 17:31
@rapphil rapphil requested review from Aneurysm9, MovieStoreGuy and bryan-aguilar and removed request for jpkrohling, Aneurysm9 and MovieStoreGuy January 9, 2023 18:38
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Only one very small comment otherwise LGTM

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

A couple minor doc comments. Not a big fan of configurablehttpprovider as a package name, but it's internal and easy to change so not blocking here.

confmap/provider/httpprovider/provider.go Outdated Show resolved Hide resolved
confmap/provider/httpsprovider/provider.go Outdated Show resolved Hide resolved
@rapphil rapphil force-pushed the rapphil-add-https-confimap branch from ca7e6be to f01173e Compare January 20, 2023 00:40
@Aneurysm9 Aneurysm9 added the ready-to-merge Code review completed; ready to merge by maintainers label Jan 20, 2023
@rapphil
Copy link
Contributor Author

rapphil commented Jan 23, 2023

@codeboten, @bogdandrutu and @dmitryax, is there anything that I can do to get this merged?

rapphil and others added 8 commits January 24, 2023 00:47
Add the httpsprovider component that allows the collector to fetch
configuration from a web server using the https protocol.

Signed-off-by: Raphael Silva <rapphil@gmail.com>
Signed-off-by: Raphael Silva <rapphil@gmail.com>
Signed-off-by: Raphael Silva <rapphil@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Improve unit tests to check for error while parsing test URL.

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Signed-off-by: Raphael Silva <rapphil@gmail.com>
@rapphil rapphil force-pushed the rapphil-add-https-confimap branch from f01173e to 0adcd04 Compare January 24, 2023 00:53
Signed-off-by: Raphael Silva <rapphil@gmail.com>
@rapphil rapphil requested review from bogdandrutu and removed request for MovieStoreGuy and bryan-aguilar January 24, 2023 01:16
Signed-off-by: Raphael Silva <rapphil@gmail.com>
@rapphil rapphil requested a review from bogdandrutu January 24, 2023 20:55
Signed-off-by: Raphael Silva <rapphil@gmail.com>
Signed-off-by: Raphael Silva <rapphil@gmail.com>
@rapphil rapphil requested a review from bogdandrutu January 25, 2023 21:43
@bogdandrutu bogdandrutu merged commit 6f1cbd8 into open-telemetry:main Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants