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

Support insecure configuration #2350

Merged
merged 14 commits into from
Jan 11, 2022
Merged

Support insecure configuration #2350

merged 14 commits into from
Jan 11, 2022

Conversation

areveny
Copy link
Contributor

@areveny areveny commented Jan 2, 2022

Description

This updates the OTLP gRPC exporter behavior to better conform to the spec.

  • Enable HTTPS by default, i.e. nothing is set for insecure. (Not in the linked issue but in the spec: insecure:default=False.)
  • Enable HTTPS if the endpoint scheme is HTTPS even if insecure=True.
  • Allow setting the insecure flag by environment variable.

Fixes #1981

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Added unit tests to test_otlp_trace_exporter.py that ensure the correct endpoint and correct secure/insecure channel is being used. New cases ensure the default is set and the https scheme supersedes the insecure option and test the new environment variable.

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

@areveny areveny requested a review from a team as a code owner January 2, 2022 21:03
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 2, 2022

CLA Signed

The committers are authorized under a signed CLA.

@srikanthccv
Copy link
Member

srikanthccv commented Jan 9, 2022

I am having a little trouble with running some tests with tox:

You seem to have unrelated test failures and unit tests do not connect with actual collector instance. I will take a look at it.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

It appears we'll also need to add OTEL_EXPORTER_OTLP_SPAN_INSECURE and OTEL_EXPORTER_OTLP_METRIC_INSECURE to be compliant with the spec https://github.com/open-telemetry/opentelemetry-specification/pull/2240/files

@srikanthccv
Copy link
Member

It appears we'll also need to add OTEL_EXPORTER_OTLP_SPAN_INSECURE and OTEL_EXPORTER_OTLP_METRIC_INSECURE to be compliant with the spec https://github.com/open-telemetry/opentelemetry-specification/pull/2240/files

Do we need to support both even if didn't have support for env prior? My understanding is SIGs that already implemented OTEL_EXPORTER_OTLP_SPAN_INSECURE should continue to support both.

@lzchen
Copy link
Contributor

lzchen commented Jan 11, 2022

@codeboten @lonewolf3739

Do we need to support both even if didn't have support for env prior? My understanding is SIGs that already implemented OTEL_EXPORTER_OTLP_SPAN_INSECURE should continue to support both.

I believe since we did not support OTEL_EXPORTER_OTLP_SPAN_INSECURE before, we do not need to support it now. Simply adding OTEL_EXPORTER_OTLP_TRACES_INSECURE would be sufficient imo assuming the spec PR gets merged.

@areveny
Copy link
Contributor Author

areveny commented Jan 11, 2022

Thanks for the reviews folks. I believe all the feedback so far has been incorporated.

@areveny
Copy link
Contributor Author

areveny commented Jan 11, 2022

Modified HTTPS cases for tests for the recently merged metrics exporter #2323.

@lzchen
Copy link
Contributor

lzchen commented Jan 11, 2022

@areveny
Can you update your contrib repo SHA to point to 23394ccd80878a91534f8421b82a7410eb775e65

@lzchen
Copy link
Contributor

lzchen commented Jan 11, 2022

@areveny
Any reason to not add OTEL_EXPORTER_OTLP_METRICS_INSECURE functionality as well for the recently merged otlp metrics exporter?

@lzchen lzchen added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Jan 11, 2022
@srikanthccv
Copy link
Member

@areveny You pushed changes just before I edited the Leighton's comment. It should OTEL_EXPORTER_OTLP_METRICS_INSECURE not OTEL_EXPORTER_OTLP_METRIC_INSECURE

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM, please resolve conflicts and change *_METRIC_* -> *_METRICS_*.

@lzchen
Copy link
Contributor

lzchen commented Jan 11, 2022

@lonewolf3739
Could you create an issue to add support for this for logging otlp exporter?

@srikanthccv
Copy link
Member

Should we wait until they are added here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#configuration-options?

@lzchen lzchen merged commit 2a7e3fc into open-telemetry:main Jan 11, 2022
@areveny areveny deleted the support-insecure-configuration branch January 11, 2022 21:31
@areveny
Copy link
Contributor Author

areveny commented Jan 11, 2022

Thanks folks, really appreciate all the guidance.

I'll keep an eye out for this on the logging otlp exporter and take that up when it's ready.

@owais
Copy link
Contributor

owais commented Feb 4, 2022

We missed something here. Since we introduced a change in SDK that the exporter depends on, the exporter should have bumped it's minimum required SDK version to the in development version. See #2438 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for insecure configuration through param and environment variable in OTLP
6 participants