From 068f2c91aba762a951ce71e2dc895ce78b294d4f Mon Sep 17 00:00:00 2001 From: KsaweryZietara Date: Mon, 23 Dec 2024 22:27:05 +0100 Subject: [PATCH] Make additional worker node pool fields mandatory --- common/runtime/model.go | 13 +- internal/broker/instance_create.go | 2 +- internal/broker/instance_create_test.go | 248 +++++++++--------------- internal/broker/instance_update.go | 2 +- internal/broker/instance_update_test.go | 140 +++++++------ internal/broker/plans.go | 2 +- 6 files changed, 190 insertions(+), 217 deletions(-) diff --git a/common/runtime/model.go b/common/runtime/model.go index db6c379e62..bd0fee02f7 100644 --- a/common/runtime/model.go +++ b/common/runtime/model.go @@ -389,8 +389,15 @@ type ModuleDTO struct { } type AdditionalWorkerNodePool struct { - AutoScalerParameters `json:",inline"` + Name string `json:"name"` + MachineType string `json:"machineType"` + AutoScalerMin int `json:"autoScalerMin"` + AutoScalerMax int `json:"autoScalerMax"` +} - Name string `json:"name"` - MachineType *string `json:"machineType,omitempty"` +func (a AdditionalWorkerNodePool) Validate() error { + if a.AutoScalerMin > a.AutoScalerMax { + return fmt.Errorf("AutoScalerMax %v should be larger than AutoScalerMin %v for %s worker node pool", a.AutoScalerMax, a.AutoScalerMin, a.Name) + } + return nil } diff --git a/internal/broker/instance_create.go b/internal/broker/instance_create.go index 9390a92ad4..b3bb8f1682 100644 --- a/internal/broker/instance_create.go +++ b/internal/broker/instance_create.go @@ -298,7 +298,7 @@ func (b *ProvisionEndpoint) validateAndExtract(details domain.ProvisionDetails, if IsPreviewPlan(details.PlanID) { for _, workerNodePool := range parameters.AdditionalWorkerNodePools { - if err := workerNodePool.AutoScalerParameters.Validate(autoscalerMin, autoscalerMax); err != nil { + if err := workerNodePool.Validate(); err != nil { return ersContext, parameters, apiresponses.NewFailureResponse(err, http.StatusUnprocessableEntity, err.Error()) } } diff --git a/internal/broker/instance_create_test.go b/internal/broker/instance_create_test.go index ceeb38b8cb..b37158f484 100644 --- a/internal/broker/instance_create_test.go +++ b/internal/broker/instance_create_test.go @@ -1491,165 +1491,109 @@ func TestProvision_Provision(t *testing.T) { // then require.EqualError(t, err, "while validating input parameters: region: region must be one of the following: \"me-central2\"") }) +} - t.Run("Should pass with additional worker node pools", func(t *testing.T) { - // given - memoryStorage := storage.NewMemoryStorage() - - queue := &automock.Queue{} - queue.On("Add", mock.AnythingOfType("string")) - - factoryBuilder := &automock.PlanValidator{} - factoryBuilder.On("IsPlanSupport", broker.PreviewPlanID).Return(true) - - planDefaults := func(planID string, platformProvider pkg.CloudProvider, provider *pkg.CloudProvider) (*gqlschema.ClusterConfigInput, error) { - return &gqlschema.ClusterConfigInput{}, nil - } - kcBuilder := &kcMock.KcBuilder{} - kcBuilder.On("GetServerURL", "").Return("", fmt.Errorf("error")) - // #create provisioner endpoint - provisionEndpoint := broker.NewProvision( - broker.Config{ - EnablePlans: []string{"preview"}, - URL: brokerURL, - OnlySingleTrialPerGA: true, - EnableKubeconfigURLLabel: true, - }, - gardener.Config{Project: "test", ShootDomain: "example.com", DNSProviders: fixDNSProviders()}, - memoryStorage.Operations(), - memoryStorage.Instances(), - memoryStorage.InstancesArchived(), - queue, - factoryBuilder, - broker.PlansConfig{}, - planDefaults, - log, - dashboardConfig, - kcBuilder, - whitelist.Set{}, - &broker.OneForAllConvergedCloudRegionsProvider{}, - ) - - additionalWorkerNodePools := `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]` - - // when - _, err := provisionEndpoint.Provision(fixRequestContext(t, "cf-sa30"), instanceID, domain.ProvisionDetails{ - ServiceID: serviceID, - PlanID: broker.PreviewPlanID, - RawParameters: json.RawMessage(fmt.Sprintf(`{"name": "%s", "region": "%s","additionalWorkerNodePools": %s }`, clusterName, "eu-central-1", additionalWorkerNodePools)), - RawContext: json.RawMessage(fmt.Sprintf(`{"globalaccount_id": "%s", "subaccount_id": "%s", "user_id": "%s"}`, "any-global-account-id", subAccountID, "Test@Test.pl")), - }, true) - t.Logf("%+v\n", *provisionEndpoint) - - // then - require.NoError(t, err) - }) - - t.Run("Should pass with empty additional worker node pools", func(t *testing.T) { - // given - memoryStorage := storage.NewMemoryStorage() - - queue := &automock.Queue{} - queue.On("Add", mock.AnythingOfType("string")) - - factoryBuilder := &automock.PlanValidator{} - factoryBuilder.On("IsPlanSupport", broker.PreviewPlanID).Return(true) - - planDefaults := func(planID string, platformProvider pkg.CloudProvider, provider *pkg.CloudProvider) (*gqlschema.ClusterConfigInput, error) { - return &gqlschema.ClusterConfigInput{}, nil - } - kcBuilder := &kcMock.KcBuilder{} - kcBuilder.On("GetServerURL", "").Return("", fmt.Errorf("error")) - // #create provisioner endpoint - provisionEndpoint := broker.NewProvision( - broker.Config{ - EnablePlans: []string{"preview"}, - URL: brokerURL, - OnlySingleTrialPerGA: true, - EnableKubeconfigURLLabel: true, - }, - gardener.Config{Project: "test", ShootDomain: "example.com", DNSProviders: fixDNSProviders()}, - memoryStorage.Operations(), - memoryStorage.Instances(), - memoryStorage.InstancesArchived(), - queue, - factoryBuilder, - broker.PlansConfig{}, - planDefaults, - log, - dashboardConfig, - kcBuilder, - whitelist.Set{}, - &broker.OneForAllConvergedCloudRegionsProvider{}, - ) - - additionalWorkerNodePools := "[]" - - // when - _, err := provisionEndpoint.Provision(fixRequestContext(t, "cf-sa30"), instanceID, domain.ProvisionDetails{ - ServiceID: serviceID, - PlanID: broker.PreviewPlanID, - RawParameters: json.RawMessage(fmt.Sprintf(`{"name": "%s", "region": "%s","additionalWorkerNodePools": %s }`, clusterName, "eu-central-1", additionalWorkerNodePools)), - RawContext: json.RawMessage(fmt.Sprintf(`{"globalaccount_id": "%s", "subaccount_id": "%s", "user_id": "%s"}`, "any-global-account-id", subAccountID, "Test@Test.pl")), - }, true) - t.Logf("%+v\n", *provisionEndpoint) - - // then - require.NoError(t, err) - }) - - t.Run("Should fail for autoScalerMin bigger than autoScalerMax", func(t *testing.T) { - // given - memoryStorage := storage.NewMemoryStorage() +func TestAdditionalWorkerNodePools(t *testing.T) { + for tn, tc := range map[string]struct { + additionalWorkerNodePools string + expectedError bool + }{ + "Valid additional worker node pools": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 0, "autoScalerMax": 0}]`, + expectedError: false, + }, + "Empty additional worker node pools": { + additionalWorkerNodePools: "[]", + expectedError: false, + }, + "Empty name": { + additionalWorkerNodePools: `[{"name": "", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing name": { + additionalWorkerNodePools: `[{"machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Empty machine type": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing machine type": { + additionalWorkerNodePools: `[{"name": "name-1", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing autoScalerMin": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMax": 3}]`, + expectedError: true, + }, + "Missing autoScalerMax": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20}]`, + expectedError: true, + }, + "AutoScalerMax bigger than 300": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 301}]`, + expectedError: true, + }, + "AutoScalerMin bigger than autoScalerMax": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20, "autoScalerMax": 3}]`, + expectedError: true, + }, + } { + t.Run(tn, func(t *testing.T) { + // given + log := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{ + Level: slog.LevelInfo, + })) - queue := &automock.Queue{} - queue.On("Add", mock.AnythingOfType("string")) + memoryStorage := storage.NewMemoryStorage() - factoryBuilder := &automock.PlanValidator{} - factoryBuilder.On("IsPlanSupport", broker.PreviewPlanID).Return(true) + queue := &automock.Queue{} + queue.On("Add", mock.AnythingOfType("string")) - planDefaults := func(planID string, platformProvider pkg.CloudProvider, provider *pkg.CloudProvider) (*gqlschema.ClusterConfigInput, error) { - return &gqlschema.ClusterConfigInput{}, nil - } - kcBuilder := &kcMock.KcBuilder{} - kcBuilder.On("GetServerURL", "").Return("", fmt.Errorf("error")) - // #create provisioner endpoint - provisionEndpoint := broker.NewProvision( - broker.Config{ - EnablePlans: []string{"preview"}, - URL: brokerURL, - OnlySingleTrialPerGA: true, - EnableKubeconfigURLLabel: true, - }, - gardener.Config{Project: "test", ShootDomain: "example.com", DNSProviders: fixDNSProviders()}, - memoryStorage.Operations(), - memoryStorage.Instances(), - memoryStorage.InstancesArchived(), - queue, - factoryBuilder, - broker.PlansConfig{}, - planDefaults, - log, - dashboardConfig, - kcBuilder, - whitelist.Set{}, - &broker.OneForAllConvergedCloudRegionsProvider{}, - ) + factoryBuilder := &automock.PlanValidator{} + factoryBuilder.On("IsPlanSupport", broker.PreviewPlanID).Return(true) - additionalWorkerNodePools := `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20, "autoScalerMax": 3}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]` + planDefaults := func(planID string, platformProvider pkg.CloudProvider, provider *pkg.CloudProvider) (*gqlschema.ClusterConfigInput, error) { + return &gqlschema.ClusterConfigInput{}, nil + } + kcBuilder := &kcMock.KcBuilder{} + kcBuilder.On("GetServerURL", "").Return("", fmt.Errorf("error")) + // #create provisioner endpoint + provisionEndpoint := broker.NewProvision( + broker.Config{ + EnablePlans: []string{"preview"}, + URL: brokerURL, + OnlySingleTrialPerGA: true, + EnableKubeconfigURLLabel: true, + }, + gardener.Config{Project: "test", ShootDomain: "example.com", DNSProviders: fixDNSProviders()}, + memoryStorage.Operations(), + memoryStorage.Instances(), + memoryStorage.InstancesArchived(), + queue, + factoryBuilder, + broker.PlansConfig{}, + planDefaults, + log, + dashboardConfig, + kcBuilder, + whitelist.Set{}, + &broker.OneForAllConvergedCloudRegionsProvider{}, + ) - // when - _, err := provisionEndpoint.Provision(fixRequestContext(t, "cf-sa30"), instanceID, domain.ProvisionDetails{ - ServiceID: serviceID, - PlanID: broker.PreviewPlanID, - RawParameters: json.RawMessage(fmt.Sprintf(`{"name": "%s", "region": "%s","additionalWorkerNodePools": %s }`, clusterName, "eu-central-1", additionalWorkerNodePools)), - RawContext: json.RawMessage(fmt.Sprintf(`{"globalaccount_id": "%s", "subaccount_id": "%s", "user_id": "%s"}`, "any-global-account-id", subAccountID, "Test@Test.pl")), - }, true) - t.Logf("%+v\n", *provisionEndpoint) + // when + _, err := provisionEndpoint.Provision(fixRequestContext(t, "cf-sa30"), instanceID, domain.ProvisionDetails{ + ServiceID: serviceID, + PlanID: broker.PreviewPlanID, + RawParameters: json.RawMessage(fmt.Sprintf(`{"name": "%s", "region": "%s","additionalWorkerNodePools": %s }`, clusterName, "eu-central-1", tc.additionalWorkerNodePools)), + RawContext: json.RawMessage(fmt.Sprintf(`{"globalaccount_id": "%s", "subaccount_id": "%s", "user_id": "%s"}`, "any-global-account-id", subAccountID, "Test@Test.pl")), + }, true) + t.Logf("%+v\n", *provisionEndpoint) - // then - require.EqualError(t, err, "AutoScalerMax 3 should be larger than AutoScalerMin 20. User provided values min:20, max:3; plan defaults min:0, max:0") - }) + // then + assert.Equal(t, tc.expectedError, err != nil) + }) + } } func TestNetworkingValidation(t *testing.T) { diff --git a/internal/broker/instance_update.go b/internal/broker/instance_update.go index d355ca5d35..7b06b7ebde 100644 --- a/internal/broker/instance_update.go +++ b/internal/broker/instance_update.go @@ -266,7 +266,7 @@ func (b *UpdateEndpoint) processUpdateParameters(instance *internal.Instance, de if IsPreviewPlan(details.PlanID) { for _, workerNodePool := range params.AdditionalWorkerNodePools { - if err := workerNodePool.AutoScalerParameters.Validate(autoscalerMin, autoscalerMax); err != nil { + if err := workerNodePool.Validate(); err != nil { return domain.UpdateServiceSpec{}, apiresponses.NewFailureResponse(err, http.StatusBadRequest, err.Error()) } } diff --git a/internal/broker/instance_update_test.go b/internal/broker/instance_update_test.go index d1c91c7c79..cc71e638b3 100644 --- a/internal/broker/instance_update_test.go +++ b/internal/broker/instance_update_test.go @@ -591,66 +591,88 @@ func TestUpdateEndpoint_UpdateParameters(t *testing.T) { assert.Equal(t, expectedErr.ValidatedStatusCode(nil), apierr.ValidatedStatusCode(nil)) assert.Equal(t, expectedErr.LoggerAction(), apierr.LoggerAction()) }) +} - t.Run("Should pass with additional worker node pools", func(t *testing.T) { - // given - additionalWorkerNodePools := `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]` - - // when - _, err := svc.Update(context.Background(), instanceID, domain.UpdateDetails{ - ServiceID: "", - PlanID: PreviewPlanID, - RawParameters: json.RawMessage("{\"additionalWorkerNodePools\":" + additionalWorkerNodePools + "}"), - PreviousValues: domain.PreviousValues{}, - RawContext: json.RawMessage("{\"globalaccount_id\":\"globalaccount_id_1\", \"active\":true}"), - MaintenanceInfo: nil, - }, true) - - // then - require.NoError(t, err) - }) - - t.Run("Should pass with empty additional worker node pools", func(t *testing.T) { - // given - additionalWorkerNodePools := "[]" - - // when - _, err := svc.Update(context.Background(), instanceID, domain.UpdateDetails{ - ServiceID: "", - PlanID: PreviewPlanID, - RawParameters: json.RawMessage("{\"additionalWorkerNodePools\":" + additionalWorkerNodePools + "}"), - PreviousValues: domain.PreviousValues{}, - RawContext: json.RawMessage("{\"globalaccount_id\":\"globalaccount_id_1\", \"active\":true}"), - MaintenanceInfo: nil, - }, true) - - // then - require.NoError(t, err) - }) - - t.Run("Should fail for autoScalerMin bigger than autoScalerMax", func(t *testing.T) { - // given - additionalWorkerNodePools := `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20, "autoScalerMax": 3}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]` - errMsg := fmt.Errorf("AutoScalerMax 3 should be larger than AutoScalerMin 20. User provided values min:20, max:3; plan defaults min:0, max:0") - expectedErr := apiresponses.NewFailureResponse(errMsg, http.StatusBadRequest, errMsg.Error()) - - // when - _, err := svc.Update(context.Background(), instanceID, domain.UpdateDetails{ - ServiceID: "", - PlanID: PreviewPlanID, - RawParameters: json.RawMessage("{\"additionalWorkerNodePools\":" + additionalWorkerNodePools + "}"), - PreviousValues: domain.PreviousValues{}, - RawContext: json.RawMessage("{\"globalaccount_id\":\"globalaccount_id_1\", \"active\":true}"), - MaintenanceInfo: nil, - }, true) - - // then - require.Error(t, err) - assert.IsType(t, &apiresponses.FailureResponse{}, err) - apierr := err.(*apiresponses.FailureResponse) - assert.Equal(t, expectedErr.ValidatedStatusCode(nil), apierr.ValidatedStatusCode(nil)) - assert.Equal(t, expectedErr.LoggerAction(), apierr.LoggerAction()) - }) +func TestAdditionalWorkerNodePools(t *testing.T) { + for tn, tc := range map[string]struct { + additionalWorkerNodePools string + expectedError bool + }{ + "Valid additional worker node pools": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}, {"name": "name-2", "machineType": "m6i.large", "autoScalerMin": 0, "autoScalerMax": 0}]`, + expectedError: false, + }, + "Empty additional worker node pools": { + additionalWorkerNodePools: "[]", + expectedError: false, + }, + "Empty name": { + additionalWorkerNodePools: `[{"name": "", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing name": { + additionalWorkerNodePools: `[{"machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Empty machine type": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing machine type": { + additionalWorkerNodePools: `[{"name": "name-1", "autoScalerMin": 3, "autoScalerMax": 20}]`, + expectedError: true, + }, + "Missing autoScalerMin": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMax": 3}]`, + expectedError: true, + }, + "Missing autoScalerMax": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20}]`, + expectedError: true, + }, + "AutoScalerMax bigger than 300": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 3, "autoScalerMax": 301}]`, + expectedError: true, + }, + "AutoScalerMin bigger than autoScalerMax": { + additionalWorkerNodePools: `[{"name": "name-1", "machineType": "m6i.large", "autoScalerMin": 20, "autoScalerMax": 3}]`, + expectedError: true, + }, + } { + t.Run(tn, func(t *testing.T) { + // given + instance := fixture.FixInstance(instanceID) + instance.ServicePlanID = PreviewPlanID + st := storage.NewMemoryStorage() + err := st.Instances().Insert(instance) + require.NoError(t, err) + err = st.Operations().InsertProvisioningOperation(fixProvisioningOperation("provisioning01")) + require.NoError(t, err) + + handler := &handler{} + q := &automock.Queue{} + q.On("Add", mock.AnythingOfType("string")) + planDefaults := func(planID string, platformProvider pkg.CloudProvider, provider *pkg.CloudProvider) (*gqlschema.ClusterConfigInput, error) { + return &gqlschema.ClusterConfigInput{}, nil + } + kcBuilder := &kcMock.KcBuilder{} + svc := NewUpdate(Config{}, st.Instances(), st.RuntimeStates(), st.Operations(), handler, true, true, false, q, PlansConfig{}, + planDefaults, fixLogger(), dashboardConfig, kcBuilder, &OneForAllConvergedCloudRegionsProvider{}, fakeKcpK8sClient) + + // when + _, err = svc.Update(context.Background(), instanceID, domain.UpdateDetails{ + ServiceID: "", + PlanID: PreviewPlanID, + RawParameters: json.RawMessage("{\"additionalWorkerNodePools\":" + tc.additionalWorkerNodePools + "}"), + PreviousValues: domain.PreviousValues{}, + RawContext: json.RawMessage("{\"globalaccount_id\":\"globalaccount_id_1\", \"active\":true}"), + MaintenanceInfo: nil, + }, true) + + // then + assert.Equal(t, tc.expectedError, err != nil) + }) + } } func TestUpdateEndpoint_UpdateWithEnabledDashboard(t *testing.T) { diff --git a/internal/broker/plans.go b/internal/broker/plans.go index 456effd61d..da9b1dc777 100644 --- a/internal/broker/plans.go +++ b/internal/broker/plans.go @@ -547,7 +547,7 @@ func Plans(plans PlansConfig, provider pkg.CloudProvider, includeAdditionalParam FreemiumPlanID: defaultServicePlan(FreemiumPlanID, FreemiumPlanName, plans, freemiumSchema, FreemiumSchema(provider, azureRegionsDisplay, includeAdditionalParamsInSchema, true, euAccessRestricted)), TrialPlanID: defaultServicePlan(TrialPlanID, TrialPlanName, plans, trialSchema, TrialSchema(includeAdditionalParamsInSchema, true)), OwnClusterPlanID: defaultServicePlan(OwnClusterPlanID, OwnClusterPlanName, plans, ownClusterSchema, OwnClusterSchema(true)), - PreviewPlanID: defaultServicePlan(PreviewPlanID, PreviewPlanName, plans, previewCatalogSchema, AWSSchema(awsMachinesDisplay, awsRegionsDisplay, awsMachineNames, includeAdditionalParamsInSchema, true, euAccessRestricted, false)), + PreviewPlanID: defaultServicePlan(PreviewPlanID, PreviewPlanName, plans, previewCatalogSchema, PreviewSchema(awsMachinesDisplay, awsRegionsDisplay, awsMachineNames, includeAdditionalParamsInSchema, true, euAccessRestricted)), } if len(sapConvergedCloudRegions) != 0 {