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

Allow http/https scheme in otlpexporter endpoint configuration #3575

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Jul 7, 2021

Description: otlpexporter endpoint configuration allows for an http or https scheme. The scheme is stripped off when instantiating the gRPC channel. A scheme of https indicates a secure channel. This aligns the collector with this hopefully soon-to-be-merged specification PR open-telemetry/opentelemetry-specification#1729

Link to tracking Issue: #2539

Documentation: Updated readme

@alanwest alanwest requested a review from a team as a code owner July 7, 2021 00:24
@alanwest alanwest requested a review from jpkrohling July 7, 2021 00:24
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

@alanwest thanks for working on this. Let's merge after open-telemetry/opentelemetry-specification#1729 is merged.

[here](https://github.com/grpc/grpc/blob/master/doc/naming.md)
[here](https://github.com/grpc/grpc/blob/master/doc/naming.md).
Including a scheme of `http` or `https` is allowed in the `endpoint`, but not required.
If a scheme of `https` is used then client transport security overriding the `insecure` setting.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please clarify this sentence? I am not sure I understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yes I've left out some words, so this sentence makes no sense! Fixed.

The idea is that if you use the scheme https then the insecure setting is ignored. This is inline with the outstanding spec PR open-telemetry/opentelemetry-specification#1729

@alanwest
Copy link
Member Author

@tigrannajaryan I've added a test validating the exporter when the endpoint has an http scheme. I'd like to add a test using the https scheme as well. This would require the test suite to be able to set up its mock receiver configured with TLS.

I noted that the testdata directory contains a certificate but not the corresponding key. Would it be ok to add a cert/key pair in the testdata directory?

The code that sets up the mock receiver is here

rcv := &mockTracesReceiver{
mockReceiver: mockReceiver{
srv: grpc.NewServer(),
},
}

I essentially need to do something like the following to run the test using TLS:

sopts := []grpc.ServerOption{}
creds, _ := credentials.NewServerTLSFromFile(grpctestdata.Path("testdata/test_cert.pem"), grpctestdata.Path("testdata/test_cert_key.pem"))
sopts = append(sopts, grpc.Creds(creds))

rcv := &mockTracesReceiver{
	mockReceiver: mockReceiver{
		srv: grpc.NewServer(sopts...),
	},
}

@tigrannajaryan
Copy link
Member

I noted that the testdata directory contains a certificate but not the corresponding key. Would it be ok to add a cert/key pair in the testdata directory?

Yes, I think it is ok.

@bogdandrutu
Copy link
Member

Please rebase

@alanwest
Copy link
Member Author

Fixed merge conflicts and added a test using the scheme https. I had to create a new certificate in order to generate the private key. I replaced the existing cert in the testdata directory since it did not seem that it was used anywhere. Is this all good?

Copy link
Member

@tigrannajaryan tigrannajaryan 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 @alanwest

@tigrannajaryan tigrannajaryan merged commit f1d4497 into open-telemetry:main Jul 21, 2021
@alanwest alanwest deleted the alanwest/otlp-endpoint-require-scheme branch July 27, 2022 20:51
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.

None yet

4 participants