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

Refactor configauth, getters use the map instead of iteration #4234

Merged
merged 1 commit into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 0 additions & 34 deletions config/configauth/clientauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,11 @@
package configauth // import "go.opentelemetry.io/collector/config/configauth"

import (
"fmt"
"net/http"

"google.golang.org/grpc/credentials"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config"
)

// ClientAuthenticator is an Extension that can be used as an authenticator for the configauth.Authentication option.
Expand All @@ -44,35 +42,3 @@ type GRPCClientAuthenticator interface {
ClientAuthenticator
PerRPCCredentials() (credentials.PerRPCCredentials, error)
}

// GetHTTPClientAuthenticator attempts to select the appropriate HTTPClientAuthenticator from the list of extensions,
// based on the component id of the extension. If an authenticator is not found, an error is returned.
// This should be only used by HTTP clients.
func GetHTTPClientAuthenticator(extensions map[config.ComponentID]component.Extension,
componentID config.ComponentID) (HTTPClientAuthenticator, error) {
for id, ext := range extensions {
if id == componentID {
if auth, ok := ext.(HTTPClientAuthenticator); ok {
return auth, nil
}
return nil, fmt.Errorf("requested authenticator is not for HTTP clients")
}
}
return nil, fmt.Errorf("failed to resolve authenticator %q: %w", componentID.String(), errAuthenticatorNotFound)
}

// GetGRPCClientAuthenticator attempts to select the appropriate GRPCClientAuthenticator from the list of extensions,
// based on the component id of the extension. If an authenticator is not found, an error is returned.
// This should only be used by gRPC clients.
func GetGRPCClientAuthenticator(extensions map[config.ComponentID]component.Extension,
componentID config.ComponentID) (GRPCClientAuthenticator, error) {
for id, ext := range extensions {
if id == componentID {
if auth, ok := ext.(GRPCClientAuthenticator); ok {
return auth, nil
}
return nil, fmt.Errorf("requested authenticator is not for gRPC clients")
}
}
return nil, fmt.Errorf("failed to resolve authenticator %q: %w", componentID.String(), errAuthenticatorNotFound)
}
54 changes: 47 additions & 7 deletions config/configauth/configauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,56 @@ type Authentication struct {
AuthenticatorName string `mapstructure:"authenticator"`
}

// GetServerAuthenticator attempts to select the appropriate from the list of extensions, based on the requested extension name.
// If an authenticator is not found, an error is returned.
func GetServerAuthenticator(extensions map[config.ComponentID]component.Extension, componentID config.ComponentID) (ServerAuthenticator, error) {
for id, ext := range extensions {
// GetServerAuthenticator attempts to select the appropriate ServerAuthenticator from the list of extensions,
// based on the requested extension name. If an authenticator is not found, an error is returned.
func (a Authentication) GetServerAuthenticator(extensions map[config.ComponentID]component.Extension) (ServerAuthenticator, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this eventually be split into a GetHTTPServerAuthenticator and a GetGRPCServerAuthentiator?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, I had exactly the same question in my mind while doing the refactoring. @jpkrohling for you to see this comment.

Copy link
Member

Choose a reason for hiding this comment

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

I just created a new issue, to keep track of it.

componentID, err := config.NewComponentIDFromString(a.AuthenticatorName)
if err != nil {
return nil, err
}

if ext, found := extensions[componentID]; found {
if auth, ok := ext.(ServerAuthenticator); ok {
if id == componentID {
return auth, nil
}
return auth, nil
}
return nil, fmt.Errorf("requested authenticator is not a server authenticator")
}

return nil, fmt.Errorf("failed to resolve authenticator %q: %w", componentID.String(), errAuthenticatorNotFound)
}

// GetHTTPClientAuthenticator attempts to select the appropriate HTTPClientAuthenticator from the list of extensions,
// based on the component id of the extension. If an authenticator is not found, an error is returned.
// This should be only used by HTTP clients.
func (a Authentication) GetHTTPClientAuthenticator(extensions map[config.ComponentID]component.Extension) (HTTPClientAuthenticator, error) {
componentID, err := config.NewComponentIDFromString(a.AuthenticatorName)
if err != nil {
return nil, err
}

if ext, found := extensions[componentID]; found {
if auth, ok := ext.(HTTPClientAuthenticator); ok {
return auth, nil
}
return nil, fmt.Errorf("requested authenticator is not a HTTPClientAuthenticator")
}
return nil, fmt.Errorf("failed to resolve authenticator %q: %w", componentID.String(), errAuthenticatorNotFound)
}

// GetGRPCClientAuthenticator attempts to select the appropriate GRPCClientAuthenticator from the list of extensions,
// based on the component id of the extension. If an authenticator is not found, an error is returned.
// This should only be used by gRPC clients.
func (a Authentication) GetGRPCClientAuthenticator(extensions map[config.ComponentID]component.Extension) (GRPCClientAuthenticator, error) {
componentID, err := config.NewComponentIDFromString(a.AuthenticatorName)
if err != nil {
return nil, err
}

if ext, found := extensions[componentID]; found {
if auth, ok := ext.(GRPCClientAuthenticator); ok {
return auth, nil
}
return nil, fmt.Errorf("requested authenticator is not a GRPCClientAuthenticator")
}
return nil, fmt.Errorf("failed to resolve authenticator %q: %w", componentID.String(), errAuthenticatorNotFound)
}
10 changes: 2 additions & 8 deletions config/configauth/configauth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ func TestGetAuthenticator(t *testing.T) {
config.NewComponentID("mock"): &MockAuthenticator{},
}

// test
componentID, err := config.NewComponentIDFromString(cfg.AuthenticatorName)
assert.NoError(t, err)

authenticator, err := GetServerAuthenticator(ext, componentID)
authenticator, err := cfg.GetServerAuthenticator(ext)

// verify
assert.NoError(t, err)
Expand All @@ -61,9 +57,7 @@ func TestGetAuthenticatorFails(t *testing.T) {
}
for _, tC := range testCases {
t.Run(tC.desc, func(t *testing.T) {
componentID, err := config.NewComponentIDFromString(tC.cfg.AuthenticatorName)
assert.NoError(t, err)
authenticator, err := GetServerAuthenticator(tC.ext, componentID)
authenticator, err := tC.cfg.GetServerAuthenticator(tC.ext)
assert.ErrorIs(t, err, tC.expected)
assert.Nil(t, authenticator)
})
Expand Down
19 changes: 4 additions & 15 deletions config/configgrpc/configgrpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"google.golang.org/grpc/keepalive"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/config"
"go.opentelemetry.io/collector/config/configauth"
"go.opentelemetry.io/collector/config/confignet"
"go.opentelemetry.io/collector/config/configtls"
Expand Down Expand Up @@ -223,12 +222,7 @@ func (gcs *GRPCClientSettings) ToDialOptions(host component.Host) ([]grpc.DialOp
return nil, fmt.Errorf("no extensions configuration available")
}

componentID, cperr := config.NewComponentIDFromString(gcs.Auth.AuthenticatorName)
if cperr != nil {
return nil, cperr
}

grpcAuthenticator, cerr := configauth.GetGRPCClientAuthenticator(host.GetExtensions(), componentID)
grpcAuthenticator, cerr := gcs.Auth.GetGRPCClientAuthenticator(host.GetExtensions())
if cerr != nil {
return nil, cerr
}
Expand Down Expand Up @@ -325,16 +319,11 @@ func (gss *GRPCServerSettings) ToServerOption(host component.Host, settings comp
}
}

uInterceptors := []grpc.UnaryServerInterceptor{}
sInterceptors := []grpc.StreamServerInterceptor{}
var uInterceptors []grpc.UnaryServerInterceptor
var sInterceptors []grpc.StreamServerInterceptor

if gss.Auth != nil {
componentID, cperr := config.NewComponentIDFromString(gss.Auth.AuthenticatorName)
if cperr != nil {
return nil, cperr
}

authenticator, err := configauth.GetServerAuthenticator(host.GetExtensions(), componentID)
authenticator, err := gss.Auth.GetServerAuthenticator(host.GetExtensions())
if err != nil {
return nil, err
}
Expand Down
7 changes: 1 addition & 6 deletions config/confighttp/confighttp.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,7 @@ func (hcs *HTTPClientSettings) ToClient(ext map[config.ComponentID]component.Ext
return nil, fmt.Errorf("extensions configuration not found")
}

componentID, cperr := config.NewComponentIDFromString(hcs.Auth.AuthenticatorName)
if cperr != nil {
return nil, cperr
}

httpCustomAuthRoundTripper, aerr := configauth.GetHTTPClientAuthenticator(ext, componentID)
httpCustomAuthRoundTripper, aerr := hcs.Auth.GetHTTPClientAuthenticator(ext)
if aerr != nil {
return nil, aerr
}
Expand Down