Validate central patch better (#1360)

stackrox · Oct 16, 2023 · ee5a704 · ee5a704
1 parent cf837b3
commit ee5a704
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 13 deletions.
diff --git a/internal/dinosaur/pkg/gitops/config.go b/internal/dinosaur/pkg/gitops/config.go
@@ -3,9 +3,7 @@ package gitops
 
 import (
 	"github.com/stackrox/acs-fleet-manager/fleetshard/pkg/central/operator"
-	"github.com/stackrox/rox/operator/apis/platform/v1alpha1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	"sigs.k8s.io/yaml"
 )
 
 // Config represents the gitops configuration
@@ -63,15 +61,51 @@ func validatePatch(path *field.Path, patch string) field.ErrorList {
 	var errs field.ErrorList
 	if len(patch) == 0 {
 		errs = append(errs, field.Required(path, "patch is required"))
+		return errs
 	}
-	// try to unmarshal the patch into a Central instance to validate it
-	if err := yaml.Unmarshal([]byte(patch), &v1alpha1.Central{}); err != nil {
+	if err := tryRenderDummyCentralWithPatch(patch); err != nil {
 		errs = append(errs, field.Invalid(path, patch, "invalid patch: "+err.Error()))
-		return errs
 	}
 	return errs
 }
 
+// tryRenderDummyCentralWithPatch renders a dummy Central instance with the given patch.
+// useful to test that a patch is valid.
+func tryRenderDummyCentralWithPatch(patch string) error {
+	var dummyParams = CentralParams{
+		ID:               "id",
+		Name:             "name",
+		Namespace:        "namespace",
+		Region:           "region",
+		ClusterID:        "clusterId",
+		CloudProvider:    "cloudProvider",
+		CloudAccountID:   "cloudAccountId",
+		SubscriptionID:   "subscriptionId",
+		Owner:            "owner",
+		OwnerAccountID:   "ownerAccountId",
+		OwnerUserID:      "ownerUserId",
+		Host:             "host",
+		OrganizationID:   "organizationId",
+		OrganizationName: "organizationName",
+		InstanceType:     "instanceType",
+		IsInternal:       false,
+	}
+	dummyConfig := Config{
+		Centrals: CentralsConfig{
+			Overrides: []CentralOverride{
+				{
+					Patch:       patch,
+					InstanceIDs: []string{"*"},
+				},
+			},
+		},
+	}
+	if _, err := renderCentral(dummyParams, dummyConfig); err != nil {
+		return err
+	}
+	return nil
+}
+
 func validateInstanceIDs(path *field.Path, instanceIDs []string) field.ErrorList {
 	var errs field.ErrorList
 	var seenInstanceIDs = make(map[string]struct{})

diff --git a/internal/dinosaur/pkg/gitops/config_test.go b/internal/dinosaur/pkg/gitops/config_test.go
@@ -41,7 +41,7 @@ centrals:
 			name: "invalid yaml in patch",
 			assert: func(t *testing.T, c *Config, err field.ErrorList) {
 				require.Len(t, err, 1)
-				assert.Equal(t, field.Invalid(field.NewPath("centrals", "overrides").Index(0).Child("patch"), "foo", "invalid patch: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal string into Go value of type v1alpha1.Central"), err[0])
+				assert.Equal(t, field.Invalid(field.NewPath("centrals", "overrides").Index(0).Child("patch"), "foo", "invalid patch: failed to apply override to Central instance: invalid JSON document"), err[0])
 			},
 			yaml: `
 centrals:
@@ -54,7 +54,7 @@ centrals:
 			name: "patch contains un-mergeable fields",
 			assert: func(t *testing.T, c *Config, err field.ErrorList) {
 				require.Len(t, err, 1)
-				assert.Equal(t, field.Invalid(field.NewPath("centrals", "overrides").Index(0).Child("patch"), "spec: 123\n", "invalid patch: error unmarshaling JSON: while decoding JSON: json: cannot unmarshal number into Go struct field Central.spec of type v1alpha1.CentralSpec"), err[0])
+				assert.Equal(t, field.Invalid(field.NewPath("centrals", "overrides").Index(0).Child("patch"), "spec: 123\n", "invalid patch: failed to unmarshal Central instance: json: cannot unmarshal number into Go struct field Central.spec of type v1alpha1.CentralSpec"), err[0])
 			},
 			yaml: `
 centrals:
@@ -67,14 +67,32 @@ centrals:
 			name: "invalid operator config and central config",
 			assert: func(t *testing.T, c *Config, err field.ErrorList) {
 				require.Len(t, err, 1)
-				assert.Contains(t, err.ToAggregate().Errors()[0].Error(), "cannot unmarshal string into Go value of type v1alpha1.Central", "central config was not validated")
+				assert.Contains(t, err.ToAggregate().Errors()[0].Error(), "invalid patch: failed to apply override to Central instance: invalid JSON document", "central config was not validated")
 			},
 			yaml: `
 centrals:
   overrides:
   - instanceIds:
     - id1
     patch: invalid
+`,
+		}, {
+			name: "invalid memory request",
+			assert: func(t *testing.T, c *Config, err field.ErrorList) {
+				require.Len(t, err, 1)
+				assert.Contains(t, err.ToAggregate().Errors()[0].Error(), "invalid patch: failed to unmarshal Central instance: quantities must match the regular expression", "central config was not validated")
+			},
+			yaml: `
+centrals:
+  overrides:
+  - instanceIds:
+    - id1
+    patch: |
+      spec:
+        central:
+          resources:
+            requests:
+              memory: "a"
 `,
 		},
 	}

diff --git a/internal/dinosaur/pkg/gitops/service.go b/internal/dinosaur/pkg/gitops/service.go
@@ -27,6 +27,22 @@ func NewService(configProvider ConfigProvider) Service {
 
 // GetCentral returns a Central instance with the given parameters.
 func (s *service) GetCentral(params CentralParams) (v1alpha1.Central, error) {
+	cfg, err := s.configProvider.Get()
+	if err != nil {
+		return v1alpha1.Central{}, errors.Wrap(err, "failed to get GitOps configuration")
+	}
+	return renderCentral(params, cfg)
+}
+
+func renderCentral(params CentralParams, config Config) (v1alpha1.Central, error) {
+	central, err := renderDefaultCentral(params)
+	if err != nil {
+		return v1alpha1.Central{}, errors.Wrap(err, "failed to get default Central instance")
+	}
+	return applyConfigToCentral(config, central, params)
+}
+
+func renderDefaultCentral(params CentralParams) (v1alpha1.Central, error) {
 	wr := new(strings.Builder)
 	if err := defaultTemplate.Execute(wr, params); err != nil {
 		return v1alpha1.Central{}, errors.Wrap(err, "failed to render default template")
@@ -35,11 +51,7 @@ func (s *service) GetCentral(params CentralParams) (v1alpha1.Central, error) {
 	if err := yaml.Unmarshal([]byte(wr.String()), &central); err != nil {
 		return v1alpha1.Central{}, errors.Wrap(err, "failed to unmarshal default central")
 	}
-	cfg, err := s.configProvider.Get()
-	if err != nil {
-		return v1alpha1.Central{}, errors.Wrap(err, "failed to get GitOps configuration")
-	}
-	return applyConfigToCentral(cfg, central, params)
+	return central, nil
 }
 
 // CentralParams represents the parameters for a Central instance.