Remove protocol-specific authenticator interfaces (open-telemetry#4255)

* 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 open-telemetry#4239 Signed-off-by: Juraci Paixão Kröhling <[email protected]> * Changed test case description Signed-off-by: Juraci Paixão Kröhling <[email protected]>
mantian2020 · Oct 26, 2021 · 2cf0fbd · 2cf0fbd
1 parent c64e6ae
commit 2cf0fbd
Show file tree

Hide file tree

Showing 10 changed files with 113 additions and 70 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -23,41 +23,41 @@ 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
 	}
 	return m.AuthenticateFunc(ctx, headers)
 }
 
 // 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
@@ -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
@@ -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
@@ -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
@@ -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
 		}