From 2cf0fbd916bf892f32f60e3ed02c47c97fb047a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraci=20Paix=C3=A3o=20Kr=C3=B6hling?= Date: Tue, 26 Oct 2021 18:29:12 +0200 Subject: [PATCH] Remove protocol-specific authenticator interfaces (#4255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove protocol-specific authenticator interfaces This PR removes the gRPC and HTTP-specific interfaces from the client authenticators. Implementations should now comply with the main top-level interface, which defines the functions previously defined at the individual interfaces. Fixes #4239 Signed-off-by: Juraci Paixão Kröhling * Changed test case description Signed-off-by: Juraci Paixão Kröhling --- CHANGELOG.md | 3 + config/configauth/clientauth.go | 12 +-- config/configauth/configauth.go | 27 ++---- config/configauth/configauth_test.go | 108 +++++++++++++++++----- config/configauth/mock_clientauth.go | 5 +- config/configauth/mock_serverauth.go | 18 ++-- config/configauth/mock_serverauth_test.go | 4 +- config/configgrpc/configgrpc.go | 2 +- config/configgrpc/configgrpc_test.go | 2 +- config/confighttp/confighttp.go | 2 +- 10 files changed, 113 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c02b9c860db..e3d193abaa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +## 🛑 Breaking changes 🛑 +- Removed `configauth.HTTPClientAuthenticator` and `configauth.GRPCClientAuthenticator` in favor of `configauth.ClientAuthenticator` (#4255) + ## 🛑 Breaking changes 🛑 - Rename `parserprovider.MapProvider` as `config.MapProvider` (#4178) diff --git a/config/configauth/clientauth.go b/config/configauth/clientauth.go index 5bd384eafd2..c8944737fe1 100644 --- a/config/configauth/clientauth.go +++ b/config/configauth/clientauth.go @@ -27,18 +27,10 @@ import ( // names from the Authentication configuration. type ClientAuthenticator interface { component.Extension -} -// HTTPClientAuthenticator is a ClientAuthenticator that can be used as an authenticator -// for the configauth.Authentication option for HTTP clients. -type HTTPClientAuthenticator interface { - ClientAuthenticator + // RoundTripper returns a RoundTripper that can be used to authenticate HTTP requests. RoundTripper(base http.RoundTripper) (http.RoundTripper, error) -} -// GRPCClientAuthenticator is a ClientAuthenticator that can be used as an authenticator for -// the configauth.Authentication option for gRPC clients. -type GRPCClientAuthenticator interface { - ClientAuthenticator + // PerRPCCredentials returns a PerRPCCredentials that can be used to authenticate gRPC requests. PerRPCCredentials() (credentials.PerRPCCredentials, error) } diff --git a/config/configauth/configauth.go b/config/configauth/configauth.go index 1c2a96d0e56..92f53e2e85b 100644 --- a/config/configauth/configauth.go +++ b/config/configauth/configauth.go @@ -23,7 +23,9 @@ import ( ) var ( - errAuthenticatorNotFound = errors.New("authenticator not found") + errAuthenticatorNotFound = errors.New("authenticator not found") + errNotClientAuthenticator = errors.New("requested authenticator is not a client authenticator") + errNotServerAuthenticator = errors.New("requested authenticator is not a server authenticator") ) // Authentication defines the auth settings for the receiver. @@ -39,34 +41,21 @@ func (a Authentication) GetServerAuthenticator(extensions map[config.ComponentID if auth, ok := ext.(ServerAuthenticator); ok { return auth, nil } - return nil, fmt.Errorf("requested authenticator is not a server authenticator") + return nil, errNotServerAuthenticator } return nil, fmt.Errorf("failed to resolve authenticator %q: %w", a.AuthenticatorID, errAuthenticatorNotFound) } -// GetHTTPClientAuthenticator attempts to select the appropriate HTTPClientAuthenticator from the list of extensions, +// GetClientAuthenticator attempts to select the appropriate ClientAuthenticator 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) { +func (a Authentication) GetClientAuthenticator(extensions map[config.ComponentID]component.Extension) (ClientAuthenticator, error) { if ext, found := extensions[a.AuthenticatorID]; found { - if auth, ok := ext.(HTTPClientAuthenticator); ok { + if auth, ok := ext.(ClientAuthenticator); ok { return auth, nil } - return nil, fmt.Errorf("requested authenticator is not a HTTPClientAuthenticator") - } - return nil, fmt.Errorf("failed to resolve authenticator %q: %w", a.AuthenticatorID, 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) { - if ext, found := extensions[a.AuthenticatorID]; found { - if auth, ok := ext.(GRPCClientAuthenticator); ok { - return auth, nil - } - return nil, fmt.Errorf("requested authenticator is not a GRPCClientAuthenticator") + return nil, errNotClientAuthenticator } return nil, fmt.Errorf("failed to resolve authenticator %q: %w", a.AuthenticatorID, errAuthenticatorNotFound) } diff --git a/config/configauth/configauth_test.go b/config/configauth/configauth_test.go index 059523cb58c..9c91326c4b1 100644 --- a/config/configauth/configauth_test.go +++ b/config/configauth/configauth_test.go @@ -23,43 +23,103 @@ import ( "go.opentelemetry.io/collector/config" ) -func TestGetAuthenticator(t *testing.T) { - // prepare - cfg := &Authentication{ - AuthenticatorID: config.NewComponentID("mock"), +func TestGetServerAuthenticator(t *testing.T) { + testCases := []struct { + desc string + authenticator component.Extension + expected error + }{ + { + desc: "obtain server authenticator", + authenticator: &MockServerAuthenticator{}, + expected: nil, + }, + { + desc: "not a server authenticator", + authenticator: &MockClientAuthenticator{}, + expected: errNotServerAuthenticator, + }, } - ext := map[config.ComponentID]component.Extension{ - config.NewComponentID("mock"): &MockAuthenticator{}, + for _, tC := range testCases { + t.Run(tC.desc, func(t *testing.T) { + // prepare + cfg := &Authentication{ + AuthenticatorID: config.NewComponentID("mock"), + } + ext := map[config.ComponentID]component.Extension{ + config.NewComponentID("mock"): tC.authenticator, + } + + authenticator, err := cfg.GetServerAuthenticator(ext) + + // verify + if tC.expected != nil { + assert.ErrorIs(t, err, tC.expected) + assert.Nil(t, authenticator) + } else { + assert.NoError(t, err) + assert.NotNil(t, authenticator) + } + }) } +} - authenticator, err := cfg.GetServerAuthenticator(ext) +func TestGetServerAuthenticatorFails(t *testing.T) { + cfg := &Authentication{ + AuthenticatorID: config.NewComponentID("does-not-exist"), + } - // verify - assert.NoError(t, err) - assert.NotNil(t, authenticator) + authenticator, err := cfg.GetServerAuthenticator(map[config.ComponentID]component.Extension{}) + assert.ErrorIs(t, err, errAuthenticatorNotFound) + assert.Nil(t, authenticator) } -func TestGetAuthenticatorFails(t *testing.T) { +func TestGetClientAuthenticator(t *testing.T) { testCases := []struct { - desc string - cfg *Authentication - ext map[config.ComponentID]component.Extension - expected error + desc string + authenticator component.Extension + expected error }{ { - desc: "ServerAuthenticator not found", - cfg: &Authentication{ - AuthenticatorID: config.NewComponentID("does-not-exist"), - }, - ext: map[config.ComponentID]component.Extension{}, - expected: errAuthenticatorNotFound, + desc: "obtain client authenticator", + authenticator: &MockClientAuthenticator{}, + expected: nil, + }, + { + desc: "not a client authenticator", + authenticator: &MockServerAuthenticator{}, + expected: errNotClientAuthenticator, }, } for _, tC := range testCases { t.Run(tC.desc, func(t *testing.T) { - authenticator, err := tC.cfg.GetServerAuthenticator(tC.ext) - assert.ErrorIs(t, err, tC.expected) - assert.Nil(t, authenticator) + // prepare + cfg := &Authentication{ + AuthenticatorID: config.NewComponentID("mock"), + } + ext := map[config.ComponentID]component.Extension{ + config.NewComponentID("mock"): tC.authenticator, + } + + authenticator, err := cfg.GetClientAuthenticator(ext) + + // verify + if tC.expected != nil { + assert.ErrorIs(t, err, tC.expected) + assert.Nil(t, authenticator) + } else { + assert.NoError(t, err) + assert.NotNil(t, authenticator) + } }) } } + +func TestGetClientAuthenticatorFails(t *testing.T) { + cfg := &Authentication{ + AuthenticatorID: config.NewComponentID("does-not-exist"), + } + authenticator, err := cfg.GetClientAuthenticator(map[config.ComponentID]component.Extension{}) + assert.ErrorIs(t, err, errAuthenticatorNotFound) + assert.Nil(t, authenticator) +} diff --git a/config/configauth/mock_clientauth.go b/config/configauth/mock_clientauth.go index cd28cf999ea..a62ac041983 100644 --- a/config/configauth/mock_clientauth.go +++ b/config/configauth/mock_clientauth.go @@ -25,9 +25,8 @@ import ( ) var ( - _ HTTPClientAuthenticator = (*MockClientAuthenticator)(nil) - _ GRPCClientAuthenticator = (*MockClientAuthenticator)(nil) - errMockError = errors.New("mock Error") + _ ClientAuthenticator = (*MockClientAuthenticator)(nil) + errMockError = errors.New("mock Error") ) // MockClientAuthenticator provides a mock implementation of GRPCClientAuthenticator and HTTPClientAuthenticator interfaces diff --git a/config/configauth/mock_serverauth.go b/config/configauth/mock_serverauth.go index e8bf209a459..3fdd735fe1e 100644 --- a/config/configauth/mock_serverauth.go +++ b/config/configauth/mock_serverauth.go @@ -23,19 +23,19 @@ import ( ) var ( - _ ServerAuthenticator = (*MockAuthenticator)(nil) - _ component.Extension = (*MockAuthenticator)(nil) + _ ServerAuthenticator = (*MockServerAuthenticator)(nil) + _ component.Extension = (*MockServerAuthenticator)(nil) ) -// MockAuthenticator provides a testing mock for code dealing with authentication. -type MockAuthenticator struct { +// MockServerAuthenticator provides a testing mock for code dealing with authentication. +type MockServerAuthenticator struct { // AuthenticateFunc to use during the authentication phase of this mock. Optional. AuthenticateFunc AuthenticateFunc // TODO: implement the other funcs } // Authenticate executes the mock's AuthenticateFunc, if provided, or just returns the given context unchanged. -func (m *MockAuthenticator) Authenticate(ctx context.Context, headers map[string][]string) (context.Context, error) { +func (m *MockServerAuthenticator) Authenticate(ctx context.Context, headers map[string][]string) (context.Context, error) { if m.AuthenticateFunc == nil { return context.Background(), nil } @@ -43,21 +43,21 @@ func (m *MockAuthenticator) Authenticate(ctx context.Context, headers map[string } // GRPCUnaryServerInterceptor isn't currently implemented and always returns nil. -func (m *MockAuthenticator) GRPCUnaryServerInterceptor(context.Context, interface{}, *grpc.UnaryServerInfo, grpc.UnaryHandler) (interface{}, error) { +func (m *MockServerAuthenticator) GRPCUnaryServerInterceptor(context.Context, interface{}, *grpc.UnaryServerInfo, grpc.UnaryHandler) (interface{}, error) { return nil, nil } // GRPCStreamServerInterceptor isn't currently implemented and always returns nil. -func (m *MockAuthenticator) GRPCStreamServerInterceptor(interface{}, grpc.ServerStream, *grpc.StreamServerInfo, grpc.StreamHandler) error { +func (m *MockServerAuthenticator) GRPCStreamServerInterceptor(interface{}, grpc.ServerStream, *grpc.StreamServerInfo, grpc.StreamHandler) error { return nil } // Start isn't currently implemented and always returns nil. -func (m *MockAuthenticator) Start(context.Context, component.Host) error { +func (m *MockServerAuthenticator) Start(context.Context, component.Host) error { return nil } // Shutdown isn't currently implemented and always returns nil. -func (m *MockAuthenticator) Shutdown(ctx context.Context) error { +func (m *MockServerAuthenticator) Shutdown(ctx context.Context) error { return nil } diff --git a/config/configauth/mock_serverauth_test.go b/config/configauth/mock_serverauth_test.go index 329a406c907..3ae9efc37bb 100644 --- a/config/configauth/mock_serverauth_test.go +++ b/config/configauth/mock_serverauth_test.go @@ -23,7 +23,7 @@ import ( func TestAuthenticateFunc(t *testing.T) { // prepare - m := &MockAuthenticator{} + m := &MockServerAuthenticator{} called := false m.AuthenticateFunc = func(c context.Context, m map[string][]string) (context.Context, error) { called = true @@ -41,7 +41,7 @@ func TestAuthenticateFunc(t *testing.T) { func TestNilOperations(t *testing.T) { // prepare - m := &MockAuthenticator{} + m := &MockServerAuthenticator{} // test and verify origCtx := context.Background() diff --git a/config/configgrpc/configgrpc.go b/config/configgrpc/configgrpc.go index 3ba99ad8e35..9f58844452c 100644 --- a/config/configgrpc/configgrpc.go +++ b/config/configgrpc/configgrpc.go @@ -222,7 +222,7 @@ func (gcs *GRPCClientSettings) ToDialOptions(host component.Host) ([]grpc.DialOp return nil, fmt.Errorf("no extensions configuration available") } - grpcAuthenticator, cerr := gcs.Auth.GetGRPCClientAuthenticator(host.GetExtensions()) + grpcAuthenticator, cerr := gcs.Auth.GetClientAuthenticator(host.GetExtensions()) if cerr != nil { return nil, cerr } diff --git a/config/configgrpc/configgrpc_test.go b/config/configgrpc/configgrpc_test.go index b4fab3806e9..1c1c94b200c 100644 --- a/config/configgrpc/configgrpc_test.go +++ b/config/configgrpc/configgrpc_test.go @@ -137,7 +137,7 @@ func TestGrpcServerAuthSettings(t *testing.T) { } host := &mockHost{ ext: map[config.ComponentID]component.Extension{ - config.NewComponentID("mock"): &configauth.MockAuthenticator{}, + config.NewComponentID("mock"): &configauth.MockServerAuthenticator{}, }, } opts, err := gss.ToServerOption(host, componenttest.NewNopTelemetrySettings()) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index f3d6d2c237b..423e70027a7 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -90,7 +90,7 @@ func (hcs *HTTPClientSettings) ToClient(ext map[config.ComponentID]component.Ext return nil, fmt.Errorf("extensions configuration not found") } - httpCustomAuthRoundTripper, aerr := hcs.Auth.GetHTTPClientAuthenticator(ext) + httpCustomAuthRoundTripper, aerr := hcs.Auth.GetClientAuthenticator(ext) if aerr != nil { return nil, aerr }