From ee5a704ce6afdefa5cd7dd835c2d00f5246490ac Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Mon, 16 Oct 2023 13:18:06 +0200 Subject: [PATCH] Validate central patch better (#1360) --- internal/dinosaur/pkg/gitops/config.go | 44 ++++++++++++++++++--- internal/dinosaur/pkg/gitops/config_test.go | 24 +++++++++-- internal/dinosaur/pkg/gitops/service.go | 22 ++++++++--- 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/internal/dinosaur/pkg/gitops/config.go b/internal/dinosaur/pkg/gitops/config.go index 19d1e66508..2f24f2c031 100644 --- 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 index 1120aa2f45..c4e84db290 100644 --- 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,7 +67,7 @@ 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: @@ -75,6 +75,24 @@ centrals: - 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 index 19f6c232d0..90aea7903d 100644 --- 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()), ¢ral); 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.