Refactor SecureReceiverSettings to use TLSSetting #1015

ccaraman · 2020-05-22T18:32:15Z

Description:
Address the second part of issue #933 to clean up existing server settings with TLS.
Removed SecureReceiverSettings and replaced instances with a reference to configtls.TLSSetting
Renamed LoadGRPCTLSCredentials -> LoadgRPCTLSClientCredentials
Created issue #1014 to track merging gRPC Server settings into a common struct.

Link to tracking Issue: #933 and handles issue #963

Testing: Ran all tests.

Documentation: Doc pr will come up next.

codecov · 2020-05-22T18:39:02Z

Codecov Report

Merging #1015 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1015      +/-   ##
==========================================
+ Coverage   86.33%   86.37%   +0.04%     
==========================================
  Files         199      198       -1     
  Lines       14072    14058      -14     
==========================================
- Hits        12149    12143       -6     
+ Misses       1461     1457       -4     
+ Partials      462      458       -4

Impacted Files	Coverage Δ
receiver/jaegerreceiver/config.go	`100.00% <ø> (ø)`
config/configgrpc/configgrpc.go	`100.00% <100.00%> (ø)`
config/configtls/configtls.go	`100.00% <100.00%> (ø)`
receiver/jaegerreceiver/factory.go	`91.66% <100.00%> (-1.67%)`	⬇️
receiver/opencensusreceiver/config.go	`94.44% <100.00%> (+9.44%)`	⬆️
receiver/opencensusreceiver/factory.go	`94.59% <100.00%> (-0.28%)`	⬇️
receiver/otlpreceiver/config.go	`94.44% <100.00%> (+9.44%)`	⬆️
receiver/otlpreceiver/factory.go	`94.59% <100.00%> (-0.28%)`	⬇️
translator/internaldata/resource_to_oc.go	`76.47% <0.00%> (+2.94%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c3125a...3ad3f7a. Read the comment docs.

jrcamp · 2020-05-22T19:38:58Z

receiver/jaegerreceiver/config.go

+	configmodels.ReceiverSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct
+	// Configures the receiver to use TLS.
+	// The default value is nil, which will cause the receiver to not use TLS.
+	TLSCredentials *configtls.TLSSetting `mapstructure:"tls_credentials, omitempty"`


tls_credentials makes sense for a server scenario but for a client they may just be be configuring the CA but no client certificates so there's no credentials. In those cases I think we'd want to name this tls_config or just tls. Would it make sense to have a universal config key for the TLS settings and call this the same thing here?

Yeah renaming the config field is an option and something that should be figured out in a separate issue. The goal of this pr is to deprecate the SecureReceiverSettings used by several server connection scenarios.

jrcamp · 2020-05-22T19:40:17Z

receiver/jaegerreceiver/config.go

+type SecureSetting struct {
+	configmodels.ReceiverSettings `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct
+	// Configures the receiver to use TLS.
+	// The default value is nil, which will cause the receiver to not use TLS.


I thought we decided that for whether TLS was enabled or not it would be part of the endpoint? Maybe we were discussing more in the context of clients but I think maybe that should also apply to listening receivers so that things are uniform?

Created this issue #1055 to track creating HTTPSettings so we can offer a config similar to the gRPCSettigs

tigrannajaryan · 2020-05-26T17:36:47Z

@jrcamp assigning to you since you already started reviewing this.

jrcamp · 2020-05-29T17:13:59Z

receiver/opencensusreceiver/config.go

+	if rOpts.TLSCredentials != nil {
+		tlsCredsOptions, er := rOpts.TLSCredentials.LoadgRPCTLSServerCredentials()
+		if er != nil {
+			return nil, fmt.Errorf("error initializing OpenCensus receiver %q TLS Credentials: %v", rOpts.NameVal, er)
+		}
+		opts = append(opts, WithGRPCServerOptions(tlsCredsOptions))


Might be better to use all named return values or none at all to avoid having the er which might be easy to mix up.

Suggested change

if rOpts.TLSCredentials != nil {

tlsCredsOptions, er := rOpts.TLSCredentials.LoadgRPCTLSServerCredentials()

if er != nil {

return nil, fmt.Errorf("error initializing OpenCensus receiver %q TLS Credentials: %v", rOpts.NameVal, er)

}

opts = append(opts, WithGRPCServerOptions(tlsCredsOptions))

if rOpts.TLSCredentials != nil {

var tlsCredOptions someType

tlsCredsOptions, err = rOpts.TLSCredentials.LoadgRPCTLSServerCredentials()

if err != nil {

return

}

opts = append(opts, WithGRPCServerOptions(tlsCredsOptions))

Then at the bottom can also just do a plain return to use the named arguments.

Going to go to no named return values since when using err there it complains shadow: declaration of "err" shadows declaration at line 79 (govet)

I think it should only shadow if you're assigning err with :=, but not using named is fine as well.

yeah but then the vars for tlsCredOptions looks a bit weird having it declared above when there is no error and it is the only case where an error can be returned.

jrcamp · 2020-05-29T17:14:33Z

receiver/opencensusreceiver/config_test.go

+		},
+	}
+	_, err := cfg.buildOptions()
+	assert.EqualError(t, err, "error initializing OpenCensus receiver \"IncorrectTLS\" TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither")


Suggested change

assert.EqualError(t, err, "error initializing OpenCensus receiver \"IncorrectTLS\" TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither")

assert.EqualError(t, err, `error initializing OpenCensus receiver "IncorrectTLS" TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither`)

jrcamp · 2020-05-29T17:14:47Z

receiver/otlpreceiver/config.go

-		opts = append(opts, tlsCredsOption)
+	if rOpts.TLSCredentials != nil {
+		tlsCredsOptions, er := rOpts.TLSCredentials.LoadgRPCTLSServerCredentials()
+		if er != nil {


ccaraman · 2020-05-29T22:20:26Z

@jrcamp - updated to address your comments.

jrcamp · 2020-05-29T22:28:02Z

@tigrannajaryan ptal

config/configtls/configtls.go

tigrannajaryan · 2020-06-02T15:26:21Z

OK to merge this?

* Refactor SecureReceiverSettings to use TLSSetting * Address test code coverage failure * Update file to use new license format * Address few small comments

…try#1015) * set gomemlimit correctly * minor fixes * update chart version * update var * remove unittest * remove unittest

ccaraman requested review from bogdandrutu, dmitryax, flands, owais, pjanotti, rghetia, songy23 and tigrannajaryan as code owners May 22, 2020 18:32

jrcamp reviewed May 22, 2020

View reviewed changes

ccaraman force-pushed the feature/ccaraman/refactor_server_tls branch from cccfe1c to 13de6e2 Compare May 26, 2020 17:22

tigrannajaryan assigned jrcamp May 26, 2020

jrcamp reviewed May 29, 2020

View reviewed changes

ccaraman added 4 commits May 29, 2020 11:24

Refactor SecureReceiverSettings to use TLSSetting

c84d85d

Address test code coverage failure

b0d29cd

Update file to use new license format

fcccd42

Address few small comments

3ad3f7a

ccaraman force-pushed the feature/ccaraman/refactor_server_tls branch from 13de6e2 to 3ad3f7a Compare May 29, 2020 18:25

jrcamp approved these changes May 29, 2020

View reviewed changes

jrcamp removed their assignment May 29, 2020

bogdandrutu approved these changes May 30, 2020

View reviewed changes

config/configtls/configtls.go Show resolved Hide resolved

bogdandrutu merged commit ed5a8de into open-telemetry:master Jun 2, 2020

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Remove unnecessary semicolon in ETW exporter (open-telemetry#1015)

096c08b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor SecureReceiverSettings to use TLSSetting #1015

Refactor SecureReceiverSettings to use TLSSetting #1015

ccaraman commented May 22, 2020

codecov bot commented May 22, 2020 •

edited

Loading

jrcamp May 22, 2020 •

edited

Loading

ccaraman May 22, 2020

jrcamp May 22, 2020

ccaraman May 29, 2020 •

edited

Loading

tigrannajaryan commented May 26, 2020

jrcamp May 29, 2020

ccaraman May 29, 2020

jrcamp May 29, 2020

ccaraman May 29, 2020

jrcamp May 29, 2020

jrcamp May 29, 2020

ccaraman commented May 29, 2020

jrcamp commented May 29, 2020

tigrannajaryan commented Jun 2, 2020

	assert.EqualError(t, err, "error initializing OpenCensus receiver \"IncorrectTLS\" TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither")
	assert.EqualError(t, err, `error initializing OpenCensus receiver "IncorrectTLS" TLS Credentials: failed to load TLS config: for auth via TLS, either both certificate and key must be supplied, or neither`)

Refactor SecureReceiverSettings to use TLSSetting #1015

Refactor SecureReceiverSettings to use TLSSetting #1015

Conversation

ccaraman commented May 22, 2020

codecov bot commented May 22, 2020 • edited Loading

Codecov Report

jrcamp May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccaraman May 29, 2020 • edited Loading

Choose a reason for hiding this comment

tigrannajaryan commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccaraman commented May 29, 2020

jrcamp commented May 29, 2020

tigrannajaryan commented Jun 2, 2020

codecov bot commented May 22, 2020 •

edited

Loading

jrcamp May 22, 2020 •

edited

Loading

ccaraman May 29, 2020 •

edited

Loading