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

OTLP exporter: Handle error case when no credentials supplied #1366

Merged
merged 7 commits into from
Nov 18, 2020

Conversation

jan25
Copy link
Contributor

@jan25 jan25 commented Nov 9, 2020

Signed-off-by: Abhilash Gnan abhilashgnan@gmail.com

Description

Env var support for OTLP exporter was added in #1101. There is a error case(#1101 (comment)) which needs to be handled when credentials is not available in case of insecure=False(default).

This PR adds a ValueError for such case. Also, modified sample code to have insecure=True.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

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

Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
@jan25 jan25 requested review from a team, toumorokoshi and lzchen and removed request for a team November 9, 2020 20:26
@jan25 jan25 changed the title OTLP exporter: Handle credentials env var error case OTLP exporter: Handle error case when no credentials supplied Nov 9, 2020
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
credentials is None
and Configuration().EXPORTER_OTLP_CERTIFICATE is None
):
raise ValueError("No credentials set in secure mode.")
Copy link
Contributor

Choose a reason for hiding this comment

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

any advantage to raising an exception here and not default to using grpc.ssl_channel_credentials()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is when insecure=False which is by default. Without this custom error handling it would break deep in stack inside grpc library without a useful message. Or can we handle this in a better way?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably reasonable. I don't know how bad the original error is but asserting invalid configurations is certainly helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jan25 my suggestion here was that using ssl_channel_credentials without any parameters would work in the case of a secure connection as it will load the root certificate from a default location:
https://grpc.github.io/grpc/python/grpc.html#create-client-credentials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, perhaps we need to update the spec before using default location? Also, i was wrong about grpc library erroring. Actually it was open(filename) thats erroring when no cert location is set and filename=None.

Copy link
Member

Choose a reason for hiding this comment

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

@jan25 if it violates the spec to look for ssl credentials in the secure case before failing, then agreed we should amend the spec.

Can you file an issue? Or do you want someone else to handle that conversation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @toumorokoshi I recently learnt grpc python library has the default certificate path support, so i don't have 100% context of this. Could someone else open an issue in spec repo about this?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

great, thanks!

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Sorry, can you add an entry in the changelog? It's not serious, but probably worth calling out new behavior of a raised ValueError.

@jan25
Copy link
Contributor Author

jan25 commented Nov 11, 2020

Thanks @toumorokoshi ! I've updated CHANGELOG.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Thanks! Would definitely be good to have a follow-up item on the use of default ssl credentials, great catch codeboten!

@jan25
Copy link
Contributor Author

jan25 commented Nov 17, 2020

Looks like a sdk test is flaky. Could we rerun the CI?

@lzchen lzchen merged commit a085c10 into open-telemetry:master Nov 18, 2020
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.

5 participants