Treat the provider config as an override, keep the defaults in the code

Since storing the whole config in the database is problematic for a number of reasons, like hard patching or worse ability to change defaults, let's change the way we treat the provider configuration in the database. Instead of storing the full config, we treat the provider config as an override. To that end, we store a structure with defaults in the code and make all the fields in the protobuf messages optional which causes the resulting Go structures to have pointers to fields instead of direct attributes. We store the config override verbatim, after just having unmarshalled and marshalled back to get rid of any extra keys. Then we merge with the default config on retrieving the provider configuration from the database.
mindersec · Jun 13, 2024 · 3dde870 · 3dde870
1 parent c4d0ba9
commit 3dde870
Show file tree

Hide file tree

Showing 31 changed files with 1,760 additions and 1,703 deletions.
diff --git a/cmd/dev/app/image/list.go b/cmd/dev/app/image/list.go
@@ -22,6 +22,7 @@ import (
 
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
+	"google.golang.org/protobuf/proto"
 
 	"github.com/stacklok/minder/internal/providers/credentials"
 	"github.com/stacklok/minder/internal/providers/dockerhub"
@@ -75,15 +76,15 @@ func runCmdList(cmd *cobra.Command, _ []string) error {
 		var err error
 		cred := credentials.NewOAuth2TokenCredential(viper.GetString("auth.token"))
 		prov, err = dockerhub.New(cred, &minderv1.DockerHubProviderConfig{
-			Namespace: ns.Value.String(),
+			Namespace: proto.String(ns.Value.String()),
 		})
 		if err != nil {
 			return err
 		}
 	case "ghcr":
 		cred := credentials.NewOAuth2TokenCredential(viper.GetString("auth.token"))
 		prov = ghcr.New(cred, &minderv1.GHCRProviderConfig{
-			Namespace: ns.Value.String(),
+			Namespace: proto.String(ns.Value.String()),
 		})
 	default:
 		return fmt.Errorf("unknown provider: %s", pclass.Value.String())

diff --git a/docs/docs/ref/proto.md b/docs/docs/ref/proto.md
diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go
@@ -1032,7 +1032,7 @@ func (s *Server) processInstallationRepositoriesAppEvent(
 		return nil, fmt.Errorf("could not determine provider id: %v", err)
 	}
 
-	providerConfig, _, err := clients.ParseV1AppConfig(dbProv.Definition)
+	providerConfig, _, err := clients.ParseAndMergeV1AppConfig(dbProv.Definition)
 	if err != nil {
 		return nil, fmt.Errorf("could not parse provider config: %v", err)
 	}

diff --git a/internal/controlplane/handlers_oauth.go b/internal/controlplane/handlers_oauth.go
@@ -139,6 +139,8 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 		if err != nil {
 			return nil, status.Errorf(codes.InvalidArgument, "error marshalling config: %s", err)
 		}
+	} else {
+		confBytes = []byte("{}")
 	}
 
 	// Insert the new session state into the database along with the user's project ID

diff --git a/internal/controlplane/handlers_oauth_test.go b/internal/controlplane/handlers_oauth_test.go
@@ -498,8 +498,11 @@ func TestProviderCallback(t *testing.T) {
 		code: 403,
 		err:  "The provided login token was associated with a different user.\n",
 	}, {
-		name:                       "No existing provider",
-		redirectUrl:                "http://localhost:8080",
+		name:        "No existing provider",
+		redirectUrl: "http://localhost:8080",
+		// such fallback config is stored when generating the authorization URL, but here we mock
+		// the state response, so let's provide the fallback ourselves.
+		config:                     []byte(`{}`),
 		code:                       307,
 		projectIDBySessionNumCalls: 2,
 		storeMockSetup: func(store *mockdb.MockStore) {
@@ -513,7 +516,7 @@ func TestProviderCallback(t *testing.T) {
 		storeMockSetup: func(store *mockdb.MockStore) {
 			withProviderNotFound(store)
 		},
-		config: []byte(`{"missing_github_key": true}`),
+		config: []byte(`{`),
 		code:   http.StatusBadRequest,
 	}}
 

diff --git a/internal/controlplane/handlers_providers.go b/internal/controlplane/handlers_providers.go
@@ -59,6 +59,7 @@ func (s *Server) CreateProvider(
 			return nil, status.Errorf(codes.InvalidArgument, "error marshalling provider provConfig: %v", marshallErr)
 		}
 	} else {
+		provConfig = json.RawMessage([]byte("{}"))
 		zerolog.Ctx(ctx).Debug().Msg("no provider provConfig, will use default")
 	}
 

diff --git a/internal/controlplane/handlers_providers_test.go b/internal/controlplane/handlers_providers_test.go
@@ -132,11 +132,9 @@ func TestCreateProvider(t *testing.T) {
 			name:          "test-github-defaults",
 			providerClass: db.ProviderClassGithub,
 			expected: minder.Provider{
-				Name: "test-github-defaults",
-				Config: newPbStruct(t, map[string]interface{}{
-					"github": map[string]interface{}{},
-				}),
-				Class: string(db.ProviderClassGithub),
+				Name:   "test-github-defaults",
+				Config: newPbStruct(t, map[string]interface{}{}),
+				Class:  string(db.ProviderClassGithub),
 			},
 		},
 		{
@@ -162,11 +160,9 @@ func TestCreateProvider(t *testing.T) {
 			name:          "test-github-app-defaults",
 			providerClass: db.ProviderClassGithubApp,
 			expected: minder.Provider{
-				Name: "test-github-app-defaults",
-				Config: newPbStruct(t, map[string]interface{}{
-					"github_app": map[string]interface{}{},
-				}),
-				Class: string(db.ProviderClassGithubApp),
+				Name:   "test-github-app-defaults",
+				Config: newPbStruct(t, map[string]interface{}{}),
+				Class:  string(db.ProviderClassGithubApp),
 			},
 		},
 		{
@@ -255,17 +251,6 @@ func TestCreateProvider(t *testing.T) {
 			jsonConfig, err := scenario.expected.Config.MarshalJSON()
 			require.NoError(t, err)
 
-			var jsonUserConfig []byte
-			if scenario.userConfig != nil {
-				jsonUserConfig, err = scenario.userConfig.MarshalJSON()
-				require.NoError(t, err)
-			}
-
-			if scenario.providerClass == db.ProviderClassGithubApp || scenario.providerClass == db.ProviderClassGithub {
-				fakeServer.mockGhService.EXPECT().GetConfig(gomock.Any(), scenario.providerClass, jsonUserConfig).
-					Return(jsonConfig, nil)
-			}
-
 			fakeServer.mockStore.EXPECT().CreateProvider(gomock.Any(), partialCreateParamsMatcher{
 				value: db.CreateProviderParams{
 					Name:       scenario.name,
@@ -375,9 +360,6 @@ func TestCreateProviderFailures(t *testing.T) {
 			Provider: engine.Provider{Name: providerName},
 		})
 
-		fakeServer.mockGhService.EXPECT().GetConfig(gomock.Any(), db.ProviderClassGithub, gomock.Any()).
-			Return(json.RawMessage(`{ "github": {} }`), nil)
-
 		fakeServer.mockStore.EXPECT().CreateProvider(gomock.Any(), gomock.Any()).
 			Return(db.Provider{}, &pq.Error{Code: "23505"}) // unique_violation
 
@@ -427,7 +409,10 @@ func TestCreateProviderFailures(t *testing.T) {
 			},
 		})
 		assert.Error(t, err)
-		require.ErrorContains(t, err, "error validating DockerHub v1 provider config: namespace is required")
+		st, ok := status.FromError(err)
+		require.True(t, ok)
+		assert.Equal(t, codes.InvalidArgument, st.Code())
+		require.ErrorContains(t, err, "namespace is required")
 	})
 
 	t.Run("github-app-does-not-validate", func(t *testing.T) {
@@ -442,9 +427,6 @@ func TestCreateProviderFailures(t *testing.T) {
 		fakeServer := testServer(t, ctrl)
 		providerName := "bad-github-app"
 
-		fakeServer.mockGhService.EXPECT().GetConfig(gomock.Any(), db.ProviderClassGithubApp, gomock.Any()).
-			Return(json.RawMessage(`{ "auto_registration": { "entities": { "blah": {"enabled": true }}}, "github-app": {}}`), nil)
-
 		_, err := fakeServer.server.CreateProvider(context.Background(), &minder.CreateProviderRequest{
 			Context: &minder.Context{
 				Project:  &projectIDStr,

diff --git a/internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect_test.go b/internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect_test.go
@@ -54,7 +54,7 @@ func testGithubProvider(baseURL string) (provifv1.GitHub, error) {
 
 	return clients.NewRestClient(
 		&pb.GitHubProviderConfig{
-			Endpoint: baseURL,
+			Endpoint: &baseURL,
 		},
 		nil,
 		&ratecache.NoopRestClientCache{},

diff --git a/internal/engine/actions/remediate/pull_request/pull_request_test.go b/internal/engine/actions/remediate/pull_request/pull_request_test.go
@@ -35,6 +35,7 @@ import (
 	"github.com/google/go-github/v61/github"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/types/known/structpb"
 
@@ -91,7 +92,7 @@ var TestActionTypeValid interfaces.ActionType = "remediate-test"
 func testGithubProvider() (provifv1.GitHub, error) {
 	return clients.NewRestClient(
 		&pb.GitHubProviderConfig{
-			Endpoint: ghApiUrl + "/",
+			Endpoint: proto.String(ghApiUrl + "/"),
 		},
 		nil,
 		&ratecache.NoopRestClientCache{},

diff --git a/internal/engine/actions/remediate/remediate_test.go b/internal/engine/actions/remediate/remediate_test.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 
 	"github.com/stacklok/minder/internal/engine/actions/remediate"
 	"github.com/stacklok/minder/internal/engine/actions/remediate/noop"
@@ -142,7 +143,7 @@ func TestNewRuleRemediator(t *testing.T) {
 }
 
 func HTTPProvider() (provifv1.Provider, error) {
-	cfg := pb.RESTProviderConfig{BaseUrl: "https://api.github.com/"}
+	cfg := pb.RESTProviderConfig{BaseUrl: proto.String("https://api.github.com/")}
 	return httpclient.NewREST(
 		&cfg,
 		telemetry.NewNoopMetrics(),

diff --git a/internal/engine/actions/remediate/rest/rest_test.go b/internal/engine/actions/remediate/rest/rest_test.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
 	"google.golang.org/protobuf/types/known/structpb"
 
@@ -54,7 +55,7 @@ func testGithubProvider(baseURL string) (provifv1.REST, error) {
 
 	return clients.NewRestClient(
 		&pb.GitHubProviderConfig{
-			Endpoint: baseURL,
+			Endpoint: &baseURL,
 		},
 		nil,
 		&ratecache.NoopRestClientCache{},
@@ -141,7 +142,7 @@ func TestNewRestRemediate(t *testing.T) {
 
 			restProvider, err := httpclient.NewREST(
 				&pb.RESTProviderConfig{
-					BaseUrl: "https://api.github.com/",
+					BaseUrl: proto.String("https://api.github.com/"),
 				},
 				telemetry.NewNoopMetrics(),
 				credentials.NewGitHubTokenCredential("token"),

diff --git a/internal/engine/ingester/artifact/artifact_test.go b/internal/engine/ingester/artifact/artifact_test.go
@@ -46,7 +46,7 @@ func testGithubProvider() (provinfv1.GitHub, error) {
 
 	return clients.NewRestClient(
 		&pb.GitHubProviderConfig{
-			Endpoint: baseURL,
+			Endpoint: &baseURL,
 		},
 		nil,
 		&ratecache.NoopRestClientCache{},

diff --git a/internal/engine/ingester/rest/rest_test.go b/internal/engine/ingester/rest/rest_test.go
@@ -26,6 +26,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/reflect/protoreflect"
 
 	engif "github.com/stacklok/minder/internal/engine/interfaces"
@@ -85,7 +86,7 @@ func TestNewRestRuleDataIngest(t *testing.T) {
 
 			rest, err := httpclient.NewREST(
 				&pb.RESTProviderConfig{
-					BaseUrl: "https://api.github.com/",
+					BaseUrl: proto.String("https://api.github.com/"),
 				},
 				telemetry.NewNoopMetrics(),
 				credentials.NewGitHubTokenCredential("token"),
@@ -112,7 +113,7 @@ func testGithubProviderBuilder(baseURL string) (provifv1.REST, error) {
 
 	return clients.NewRestClient(
 		&pb.GitHubProviderConfig{
-			Endpoint: baseURL,
+			Endpoint: &baseURL,
 		},
 		nil,
 		&ratecache.NoopRestClientCache{},

diff --git a/internal/providers/dockerhub/dockerhub.go b/internal/providers/dockerhub/dockerhub.go
@@ -106,10 +106,17 @@ func ParseV1Config(rawCfg json.RawMessage) (*minderv1.DockerHubProviderConfig, e
 }
 
 // MarshalV1Config marshals the DockerHubProviderConfig struct into a raw config
-func MarshalV1Config(cfg *minderv1.DockerHubProviderConfig) (json.RawMessage, error) {
-	w := dhConfigWrapper{
-		DockerHub: cfg,
+func MarshalV1Config(rawCfg json.RawMessage) (json.RawMessage, error) {
+	var w dhConfigWrapper
+	if err := json.Unmarshal(rawCfg, &w); err != nil {
+		return nil, err
+	}
+
+	err := w.DockerHub.Validate()
+	if err != nil {
+		return nil, fmt.Errorf("error validating provider config: %w", err)
 	}
+
 	return json.Marshal(w)
 }
 

diff --git a/internal/providers/dockerhub/manager.go b/internal/providers/dockerhub/manager.go
@@ -114,35 +114,12 @@ func (m *providerClassManager) getProviderCredentials(
 	return credentials.NewOAuth2TokenCredential(decryptedToken.AccessToken), nil
 }
 
-func (m *providerClassManager) GetConfig(
-	_ context.Context, class db.ProviderClass, userConfig json.RawMessage,
-) (json.RawMessage, error) {
-	if !slices.Contains(m.GetSupportedClasses(), class) {
-		return nil, fmt.Errorf("provider does not implement %s", string(class))
-	}
-
-	const defaultConfig = `{"dockerhub": {}}`
-
-	if len(userConfig) == 0 {
-		return json.RawMessage(defaultConfig), nil
-	}
-
-	// in the future, we may want to validate the user config and merge it with the default config. Right now
-	// we just return the user config as is
-	return userConfig, nil
-}
-
 func (m *providerClassManager) MarshallConfig(
 	_ context.Context, class db.ProviderClass, config json.RawMessage,
 ) (json.RawMessage, error) {
 	if !slices.Contains(m.GetSupportedClasses(), class) {
 		return nil, fmt.Errorf("provider does not implement %s", string(class))
 	}
 
-	dc, err := ParseV1Config(config)
-	if err != nil {
-		return nil, err
-	}
-
-	return MarshalV1Config(dc)
+	return MarshalV1Config(config)
 }