From 114fe00f5662ff19e860efd9e390fcefcf122d11 Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Fri, 27 Jan 2023 16:54:43 +0000 Subject: [PATCH 1/2] Allow multiple apps in manifests And fix the behaviour of creating a web process during manifest application or app creation to match cf on VMs Issue: https://github.com/cloudfoundry/korifi/issues/1661 Co-authored-by: Giuseppe Capizzi --- api/actions/manifest.go | 19 +++-- api/actions/manifest/normalizer.go | 9 +-- api/actions/manifest/normalizer_test.go | 13 +++- api/actions/manifest_test.go | 93 +++++++++++++---------- api/handlers/app.go | 10 +++ api/handlers/app_test.go | 21 +++++ api/handlers/fake/cfprocess_repository.go | 78 +++++++++++++++++++ api/handlers/process.go | 1 + api/payloads/manifest.go | 2 +- tests/e2e/apps_test.go | 7 +- tests/e2e/e2e_suite_test.go | 16 ++-- tests/e2e/spaces_test.go | 43 +++++------ 12 files changed, 219 insertions(+), 93 deletions(-) diff --git a/api/actions/manifest.go b/api/actions/manifest.go index 3ef3db987..4fe3a08ee 100644 --- a/api/actions/manifest.go +++ b/api/actions/manifest.go @@ -45,20 +45,25 @@ func NewManifest(domainRepo shared.CFDomainRepository, defaultDomainName string, } } -func (a *Manifest) Apply(ctx context.Context, authInfo authorization.Info, spaceGUID string, manifest payloads.Manifest) error { +func (a *Manifest) Apply(ctx context.Context, authInfo authorization.Info, spaceGUID string, manifesto payloads.Manifest) error { err := a.ensureDefaultDomainConfigured(ctx, authInfo) if err != nil { return err } - appInfo := manifest.Applications[0] - appState, err := a.stateCollector.CollectState(ctx, authInfo, appInfo.Name, spaceGUID) - if err != nil { - return err + for _, appInfo := range manifesto.Applications { + appState, err := a.stateCollector.CollectState(ctx, authInfo, appInfo.Name, spaceGUID) + if err != nil { + return err + } + appInfo = a.normalizer.Normalize(appInfo, appState) + err = a.applier.Apply(ctx, authInfo, spaceGUID, appInfo, appState) + if err != nil { + return err + } } - appInfo = a.normalizer.Normalize(appInfo, appState) - return a.applier.Apply(ctx, authInfo, spaceGUID, appInfo, appState) + return nil } func (a *Manifest) ensureDefaultDomainConfigured(ctx context.Context, authInfo authorization.Info) error { diff --git a/api/actions/manifest/normalizer.go b/api/actions/manifest/normalizer.go index 9dba11b6a..538610393 100644 --- a/api/actions/manifest/normalizer.go +++ b/api/actions/manifest/normalizer.go @@ -73,15 +73,14 @@ func (n Normalizer) normalizeProcesses(appInfo payloads.ManifestApplication, app break } } + if webProc == nil { + processes = append(processes, payloads.ManifestApplicationProcess{Type: korifiv1alpha1.ProcessTypeWeb}) + webProc = &processes[len(processes)-1] + } if appInfo.Memory != nil || appInfo.DiskQuota != nil || appInfo.Instances != nil || appInfo.Command != nil || appInfo.HealthCheckHTTPEndpoint != nil || appInfo.HealthCheckType != nil || appInfo.HealthCheckInvocationTimeout != nil || appInfo.Timeout != nil { - if webProc == nil { - processes = append(processes, payloads.ManifestApplicationProcess{Type: korifiv1alpha1.ProcessTypeWeb}) - webProc = &processes[len(processes)-1] - } - webProc.Memory = procValIfSet(appInfo.Memory, webProc.Memory) webProc.DiskQuota = procValIfSet(appInfo.DiskQuota, webProc.DiskQuota) webProc.Instances = procValIfSet(appInfo.Instances, webProc.Instances) diff --git a/api/actions/manifest/normalizer_test.go b/api/actions/manifest/normalizer_test.go index c65d37390..d4dfbb045 100644 --- a/api/actions/manifest/normalizer_test.go +++ b/api/actions/manifest/normalizer_test.go @@ -117,8 +117,15 @@ var _ = Describe("Normalizer", func() { } }) + It("always creates a web process", func() { + Expect(normalizedAppInfo.Processes).To(ContainElement(payloads.ManifestApplicationProcess{Type: "web"})) + }) + It("preserves existing processes", func() { - Expect(normalizedAppInfo.Processes).To(ConsistOf(payloads.ManifestApplicationProcess{Type: "bob"})) + Expect(normalizedAppInfo.Processes).To(ConsistOf( + payloads.ManifestApplicationProcess{Type: "web"}, + payloads.ManifestApplicationProcess{Type: "bob"}, + )) }) DescribeTable("when app-level values are provided", @@ -159,7 +166,7 @@ var _ = Describe("Normalizer", func() { Expect(webProc.Timeout).To(Equal(effective.Timeout)) }, - // without an existing web process + // without an explicit web process in the manifest Entry("app-level command only", appParams{Command: tools.PtrTo("echo boo")}, prcParams{}, expParams{Command: tools.PtrTo("echo boo")}), @@ -188,7 +195,7 @@ var _ = Describe("Normalizer", func() { appParams{Memory: tools.PtrTo("512M"), DiskQuota: tools.PtrTo("2G")}, prcParams{}, expParams{Memory: tools.PtrTo("512M"), DiskQuota: tools.PtrTo("2G")}), - // with an existing web process without the given value set + // with an explicit web process in the manifest, but without the app-level value set Entry("empty proc with app memory", appParams{Memory: tools.PtrTo("512M")}, prcParams{Instances: tools.PtrTo(3)}, diff --git a/api/actions/manifest_test.go b/api/actions/manifest_test.go index bc5239692..2580bbf0e 100644 --- a/api/actions/manifest_test.go +++ b/api/actions/manifest_test.go @@ -21,12 +21,10 @@ var _ = Describe("ApplyManifest", func() { manifestAction *actions.Manifest applyErr error - domainRepository *reposfake.CFDomainRepository - stateCollector *fake.StateCollector - normalizer *fake.Normalizer - applier *fake.Applier - appState manifest.AppState - normalizedManifest payloads.ManifestApplication + domainRepository *reposfake.CFDomainRepository + stateCollector *fake.StateCollector + normalizer *fake.Normalizer + applier *fake.Applier appManifest payloads.Manifest ) @@ -37,22 +35,31 @@ var _ = Describe("ApplyManifest", func() { normalizer = new(fake.Normalizer) applier = new(fake.Applier) - appState = manifest.AppState{ + stateCollector.CollectStateReturnsOnCall(0, manifest.AppState{ App: repositories.AppRecord{ - GUID: "app-guid", - Name: "app-name", + GUID: "app1-guid", + Name: "app1", }, - } - stateCollector.CollectStateReturns(appState, nil) + }, nil) + stateCollector.CollectStateReturnsOnCall(1, manifest.AppState{ + App: repositories.AppRecord{ + GUID: "app2-guid", + Name: "app2", + }, + }, nil) - normalizedManifest = payloads.ManifestApplication{ - Name: "app-name", - } - normalizer.NormalizeReturns(normalizedManifest) + normalizer.NormalizeReturnsOnCall(0, payloads.ManifestApplication{ + Name: "normalized-app1", + }) + normalizer.NormalizeReturnsOnCall(1, payloads.ManifestApplication{ + Name: "normalized-app2", + }) appManifest = payloads.Manifest{ Applications: []payloads.ManifestApplication{{ - Name: "app-name", + Name: "app1", + }, { + Name: "app2", }}, } @@ -63,14 +70,38 @@ var _ = Describe("ApplyManifest", func() { applyErr = manifestAction.Apply(context.Background(), authorization.Info{}, "space-guid", appManifest) }) - It("succeeds", func() { + It("normalizes the manifest and then applies it", func() { Expect(applyErr).NotTo(HaveOccurred()) - }) - It("ensures the default domain is configured", func() { Expect(domainRepository.GetDomainByNameCallCount()).To(Equal(1)) _, _, actualDomain := domainRepository.GetDomainByNameArgsForCall(0) Expect(actualDomain).To(Equal("my.domain")) + + Expect(stateCollector.CollectStateCallCount()).To(Equal(2)) + _, _, actualAppName, actualSpaceGUID := stateCollector.CollectStateArgsForCall(0) + Expect(actualAppName).To(Equal("app1")) + Expect(actualSpaceGUID).To(Equal("space-guid")) + _, _, actualAppName, actualSpaceGUID = stateCollector.CollectStateArgsForCall(1) + Expect(actualAppName).To(Equal("app2")) + Expect(actualSpaceGUID).To(Equal("space-guid")) + + Expect(normalizer.NormalizeCallCount()).To(Equal(2)) + actualAppInManifest, actualState := normalizer.NormalizeArgsForCall(0) + Expect(actualAppInManifest.Name).To(Equal("app1")) + Expect(actualState.App.GUID).To(Equal("app1-guid")) + actualAppInManifest, actualState = normalizer.NormalizeArgsForCall(1) + Expect(actualAppInManifest.Name).To(Equal("app2")) + Expect(actualState.App.GUID).To(Equal("app2-guid")) + + Expect(applier.ApplyCallCount()).To(Equal(2)) + _, _, actualSpaceGUID, actualAppInManifest, actualState = applier.ApplyArgsForCall(0) + Expect(actualSpaceGUID).To(Equal("space-guid")) + Expect(actualAppInManifest.Name).To(Equal("normalized-app1")) + Expect(actualState.App.GUID).To(Equal("app1-guid")) + _, _, actualSpaceGUID, actualAppInManifest, actualState = applier.ApplyArgsForCall(1) + Expect(actualSpaceGUID).To(Equal("space-guid")) + Expect(actualAppInManifest.Name).To(Equal("normalized-app2")) + Expect(actualState.App.GUID).To(Equal("app2-guid")) }) When("the default domain does not exist", func() { @@ -93,16 +124,9 @@ var _ = Describe("ApplyManifest", func() { }) }) - It("collects the app state", func() { - Expect(stateCollector.CollectStateCallCount()).To(Equal(1)) - _, _, actualAppName, actualSpaceGUID := stateCollector.CollectStateArgsForCall(0) - Expect(actualAppName).To(Equal("app-name")) - Expect(actualSpaceGUID).To(Equal("space-guid")) - }) - When("collecting the app state fails", func() { BeforeEach(func() { - stateCollector.CollectStateReturns(manifest.AppState{}, errors.New("collect-state-err")) + stateCollector.CollectStateReturnsOnCall(0, manifest.AppState{}, errors.New("collect-state-err")) }) It("returns the error", func() { @@ -110,21 +134,6 @@ var _ = Describe("ApplyManifest", func() { }) }) - It("normalizes the manifest", func() { - Expect(normalizer.NormalizeCallCount()).To(Equal(1)) - actualAppInManifest, actualState := normalizer.NormalizeArgsForCall(0) - Expect(actualAppInManifest.Name).To(Equal("app-name")) - Expect(actualState.App.GUID).To(Equal("app-guid")) - }) - - It("applies the normalized manifest", func() { - Expect(applier.ApplyCallCount()).To(Equal(1)) - _, _, actualSpaceGUID, actualAppInManifest, actualState := applier.ApplyArgsForCall(0) - Expect(actualSpaceGUID).To(Equal("space-guid")) - Expect(actualAppInManifest.Name).To(Equal("app-name")) - Expect(actualState.App.GUID).To(Equal("app-guid")) - }) - When("applying the normalized manifest fails", func() { BeforeEach(func() { applier.ApplyReturns(errors.New("apply-err")) diff --git a/api/handlers/app.go b/api/handlers/app.go index ddd3363a8..17c486715 100644 --- a/api/handlers/app.go +++ b/api/handlers/app.go @@ -15,6 +15,7 @@ import ( "code.cloudfoundry.org/korifi/api/presenter" "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/routing" + korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/tools" "github.com/go-logr/logr" @@ -131,6 +132,15 @@ func (h *App) create(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, err, "Failed to create app", "App Name", payload.Name) } + err = h.processRepo.CreateProcess(r.Context(), authInfo, repositories.CreateProcessMessage{ + AppGUID: appRecord.GUID, + SpaceGUID: spaceGUID, + Type: korifiv1alpha1.ProcessTypeWeb, + }) + if err != nil { + return nil, apierrors.LogAndReturn(logger, err, "Failed to create web process", "App Name", payload.Name) + } + return routing.NewResponse(http.StatusCreated).WithBody(presenter.ForApp(appRecord, h.serverURL)), nil } diff --git a/api/handlers/app_test.go b/api/handlers/app_test.go index e46ff4d13..c768a1bc3 100644 --- a/api/handlers/app_test.go +++ b/api/handlers/app_test.go @@ -316,6 +316,27 @@ var _ = Describe("App", func() { }`, defaultServerURL, appGUID, spaceGUID)), "Response body matches response:") }) + It("creates the `web` process", func() { + Expect(processRepo.CreateProcessCallCount()).To(Equal(1)) + _, actualAuthInfo, actualMsg := processRepo.CreateProcessArgsForCall(0) + Expect(actualAuthInfo).To(Equal(authInfo)) + Expect(actualMsg).To(Equal(repositories.CreateProcessMessage{ + AppGUID: appGUID, + SpaceGUID: spaceGUID, + Type: "web", + })) + }) + + When("creating the process fails", func() { + BeforeEach(func() { + processRepo.CreateProcessReturns(errors.New("create-process-err")) + }) + + It("returns an error", func() { + expectUnknownError() + }) + }) + When("creating the app fails", func() { BeforeEach(func() { appRepo.CreateAppReturns(repositories.AppRecord{}, errors.New("create-app-err")) diff --git a/api/handlers/fake/cfprocess_repository.go b/api/handlers/fake/cfprocess_repository.go index c7d890bc9..511ad6528 100644 --- a/api/handlers/fake/cfprocess_repository.go +++ b/api/handlers/fake/cfprocess_repository.go @@ -11,6 +11,19 @@ import ( ) type CFProcessRepository struct { + CreateProcessStub func(context.Context, authorization.Info, repositories.CreateProcessMessage) error + createProcessMutex sync.RWMutex + createProcessArgsForCall []struct { + arg1 context.Context + arg2 authorization.Info + arg3 repositories.CreateProcessMessage + } + createProcessReturns struct { + result1 error + } + createProcessReturnsOnCall map[int]struct { + result1 error + } GetProcessStub func(context.Context, authorization.Info, string) (repositories.ProcessRecord, error) getProcessMutex sync.RWMutex getProcessArgsForCall []struct { @@ -77,6 +90,69 @@ type CFProcessRepository struct { invocationsMutex sync.RWMutex } +func (fake *CFProcessRepository) CreateProcess(arg1 context.Context, arg2 authorization.Info, arg3 repositories.CreateProcessMessage) error { + fake.createProcessMutex.Lock() + ret, specificReturn := fake.createProcessReturnsOnCall[len(fake.createProcessArgsForCall)] + fake.createProcessArgsForCall = append(fake.createProcessArgsForCall, struct { + arg1 context.Context + arg2 authorization.Info + arg3 repositories.CreateProcessMessage + }{arg1, arg2, arg3}) + stub := fake.CreateProcessStub + fakeReturns := fake.createProcessReturns + fake.recordInvocation("CreateProcess", []interface{}{arg1, arg2, arg3}) + fake.createProcessMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *CFProcessRepository) CreateProcessCallCount() int { + fake.createProcessMutex.RLock() + defer fake.createProcessMutex.RUnlock() + return len(fake.createProcessArgsForCall) +} + +func (fake *CFProcessRepository) CreateProcessCalls(stub func(context.Context, authorization.Info, repositories.CreateProcessMessage) error) { + fake.createProcessMutex.Lock() + defer fake.createProcessMutex.Unlock() + fake.CreateProcessStub = stub +} + +func (fake *CFProcessRepository) CreateProcessArgsForCall(i int) (context.Context, authorization.Info, repositories.CreateProcessMessage) { + fake.createProcessMutex.RLock() + defer fake.createProcessMutex.RUnlock() + argsForCall := fake.createProcessArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3 +} + +func (fake *CFProcessRepository) CreateProcessReturns(result1 error) { + fake.createProcessMutex.Lock() + defer fake.createProcessMutex.Unlock() + fake.CreateProcessStub = nil + fake.createProcessReturns = struct { + result1 error + }{result1} +} + +func (fake *CFProcessRepository) CreateProcessReturnsOnCall(i int, result1 error) { + fake.createProcessMutex.Lock() + defer fake.createProcessMutex.Unlock() + fake.CreateProcessStub = nil + if fake.createProcessReturnsOnCall == nil { + fake.createProcessReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.createProcessReturnsOnCall[i] = struct { + result1 error + }{result1} +} + func (fake *CFProcessRepository) GetProcess(arg1 context.Context, arg2 authorization.Info, arg3 string) (repositories.ProcessRecord, error) { fake.getProcessMutex.Lock() ret, specificReturn := fake.getProcessReturnsOnCall[len(fake.getProcessArgsForCall)] @@ -346,6 +422,8 @@ func (fake *CFProcessRepository) PatchProcessReturnsOnCall(i int, result1 reposi func (fake *CFProcessRepository) Invocations() map[string][][]interface{} { fake.invocationsMutex.RLock() defer fake.invocationsMutex.RUnlock() + fake.createProcessMutex.RLock() + defer fake.createProcessMutex.RUnlock() fake.getProcessMutex.RLock() defer fake.getProcessMutex.RUnlock() fake.getProcessByAppTypeAndSpaceMutex.RLock() diff --git a/api/handlers/process.go b/api/handlers/process.go index b44b26d9a..b6acaabcc 100644 --- a/api/handlers/process.go +++ b/api/handlers/process.go @@ -31,6 +31,7 @@ type CFProcessRepository interface { ListProcesses(context.Context, authorization.Info, repositories.ListProcessesMessage) ([]repositories.ProcessRecord, error) GetProcessByAppTypeAndSpace(context.Context, authorization.Info, string, string, string) (repositories.ProcessRecord, error) PatchProcess(context.Context, authorization.Info, repositories.PatchProcessMessage) (repositories.ProcessRecord, error) + CreateProcess(context.Context, authorization.Info, repositories.CreateProcessMessage) error } //counterfeiter:generate -o fake -fake-name ProcessScaler . ProcessScaler diff --git a/api/payloads/manifest.go b/api/payloads/manifest.go index a29c1df51..2be9599c3 100644 --- a/api/payloads/manifest.go +++ b/api/payloads/manifest.go @@ -10,7 +10,7 @@ import ( type Manifest struct { Version int `yaml:"version"` - Applications []ManifestApplication `yaml:"applications" validate:"max=1,dive"` + Applications []ManifestApplication `yaml:"applications"` } type ManifestApplication struct { diff --git a/tests/e2e/apps_test.go b/tests/e2e/apps_test.go index 5daa11b33..57a6c1f10 100644 --- a/tests/e2e/apps_test.go +++ b/tests/e2e/apps_test.go @@ -233,7 +233,7 @@ var _ = Describe("Apps", func() { }) Describe("List app processes", func() { - var result resourceList + var result typedResourceList BeforeEach(func() { createSpaceRole("space_developer", certUserName, space1GUID) @@ -246,9 +246,10 @@ var _ = Describe("Apps", func() { Expect(err).NotTo(HaveOccurred()) }) - It("successfully lists the empty set of processes", func() { + It("successfully lists the default web process", func() { Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) - Expect(result.Resources).To(BeEmpty()) + Expect(result.Resources).To(HaveLen(1)) + Expect(result.Resources[0].Type).To(Equal("web")) }) }) diff --git a/tests/e2e/e2e_suite_test.go b/tests/e2e/e2e_suite_test.go index 9a404a694..4750e60a4 100644 --- a/tests/e2e/e2e_suite_test.go +++ b/tests/e2e/e2e_suite_test.go @@ -164,6 +164,7 @@ type manifestResource struct { type applicationResource struct { Name string `yaml:"name"` + Command string `yaml:"command"` DefaultRoute bool `yaml:"default-route"` RandomRoute bool `yaml:"random-route"` NoRoute bool `yaml:"no-route"` @@ -230,14 +231,11 @@ type cfErrs struct { Errors []cfErr } -type processResourceList struct { - Resources []processResource `json:"resources"` -} - type processResource struct { resource `json:",inline"` Type string `json:"type"` Instances int `json:"instances"` + Command string `yaml:"command"` } type metadataPatch struct { @@ -533,19 +531,17 @@ func getEnv(appName string) map[string]interface{} { } func getProcess(appGUID, processType string) processResource { - var processList processResourceList + var process processResource EventuallyWithOffset(1, func(g Gomega) { resp, err := adminClient.R(). - SetResult(&processList). - Get("/v3/processes?app_guids=" + appGUID) + SetResult(&process). + Get("/v3/apps/" + appGUID + "/processes/" + processType) g.Expect(err).NotTo(HaveOccurred()) g.Expect(resp).To(HaveRestyStatusCode(http.StatusOK)) - g.Expect(processList.Resources).NotTo(BeEmpty()) }).Should(Succeed()) - ExpectWithOffset(1, processList.Resources).To(HaveLen(1)) - return processList.Resources[0] + return process } func createServiceInstance(spaceGUID, name string) string { diff --git a/tests/e2e/spaces_test.go b/tests/e2e/spaces_test.go index a7f1e84a7..1f49b827b 100644 --- a/tests/e2e/spaces_test.go +++ b/tests/e2e/spaces_test.go @@ -299,31 +299,33 @@ var _ = Describe("Spaces", func() { Describe("manifests", func() { var ( - spaceGUID string - resultErr cfErrs - manifestBytes []byte - manifest manifestResource - appName string + spaceGUID string + resultErr cfErrs + manifestBytes []byte + manifest manifestResource + app1Name, app2Name string ) BeforeEach(func() { spaceGUID = createSpace(generateGUID("space"), commonTestOrgGUID) resultErr = cfErrs{} - appName = generateGUID("manifested-app") + app1Name = generateGUID("manifested-app-1") + app2Name = generateGUID("manifested-app-2") - route := fmt.Sprintf("%s.%s", appName, appFQDN) - command := "whatever" + route := fmt.Sprintf("%s.%s", app1Name, appFQDN) manifest = manifestResource{ Version: 1, Applications: []applicationResource{{ - Name: appName, - Processes: []manifestApplicationProcessResource{{ - Type: "web", - Command: &command, - }}, + Name: app1Name, + Command: "whatever", Routes: []manifestRouteResource{{ Route: &route, }}, + }, { + Name: app2Name, + Processes: []manifestApplicationProcessResource{{ + Type: "bob", + }}, }}, } }) @@ -363,16 +365,13 @@ var _ = Describe("Spaces", func() { g.Expect(string(jobResp.Body())).To(ContainSubstring("COMPLETE")) }).Should(Succeed()) - var apps resourceList - appsResp, err := restyClient.R(). - SetResult(&apps). - Get("/v3/apps?names=" + appName) - Expect(err).NotTo(HaveOccurred()) - Expect(appsResp).To(HaveRestyStatusCode(http.StatusOK)) - Expect(apps.Resources).To(HaveLen(1)) - - process := getProcess(apps.Resources[0].GUID, "web") + app1GUID := getAppGUIDFromName(app1Name) + process := getProcess(app1GUID, "web") Expect(process.Instances).To(Equal(1)) + Expect(process.Command).To(Equal("whatever")) + app2GUID := getAppGUIDFromName(app2Name) + Expect(getProcess(app2GUID, "web").Instances).To(Equal(1)) + Expect(getProcess(app2GUID, "bob").Instances).To(Equal(0)) }) }) From 8135b7072ac36f9be513bf4fff2a8af85a2166db Mon Sep 17 00:00:00 2001 From: Kieron Browne Date: Mon, 30 Jan 2023 11:38:00 +0000 Subject: [PATCH 2/2] Fix manifest validation Test for e.g. app memory > 0 were failing for the wrong reason. 0 fails to be parsed as an amount with a unit by the bytefmt library as it doesn't contain a byte unit. Adding tests for missing units showed the errors message could be improved by splitting validation into that for having a unit and that for being a positive amount. go-playground/validator doesn't allow us to give the true path to the erroneous value unfortunately. E.g. the cf on vms errors would be along the lines of `For application 'test-app': Process "web": Disk quota must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB`. So best effort on field identification for now, until we can rethink the validation code. --- api/handlers/shared.go | 45 ++++++++++++++++--- api/handlers/space_manifest_test.go | 68 +++++++++++++++++++---------- api/payloads/manifest.go | 14 +++--- 3 files changed, 92 insertions(+), 35 deletions(-) diff --git a/api/handlers/shared.go b/api/handlers/shared.go index d8f317cc3..2f637313c 100644 --- a/api/handlers/shared.go +++ b/api/handlers/shared.go @@ -107,7 +107,12 @@ func wireValidator() (*validator.Validate, ut.Translator, error) { return nil, nil, err } // Register custom validators - err = v.RegisterValidation("megabytestring", megabyteFormattedString, true) + err = v.RegisterValidation("amountWithUnit", amountWithUnit, true) + if err != nil { + return nil, nil, err + } + + err = v.RegisterValidation("positiveAmountWithUnit", positiveAmountWithUnit, true) if err != nil { return nil, nil, err } @@ -140,6 +145,26 @@ func wireValidator() (*validator.Validate, ut.Translator, error) { v.RegisterStructValidation(checkRoleTypeAndOrgSpace, payloads.RoleCreate{}) + err = v.RegisterTranslation("amountWithUnit", trans, func(ut ut.Translator) error { + return ut.Add("amountWithUnit", "{0} must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB", false) + }, func(ut ut.Translator, fe validator.FieldError) string { + t, _ := ut.T("amountWithUnit", fe.Field()) + return t + }) + if err != nil { + return nil, nil, err + } + + err = v.RegisterTranslation("positiveAmountWithUnit", trans, func(ut ut.Translator) error { + return ut.Add("positiveAmountWithUnit", "{0} must be greater than 0MB", false) + }, func(ut ut.Translator, fe validator.FieldError) string { + t, _ := ut.T("positiveAmountWithUnit", fe.Field()) + return t + }) + if err != nil { + return nil, nil, err + } + err = v.RegisterTranslation("cannot_have_both_org_and_space_set", trans, func(ut ut.Translator) error { return ut.Add("cannot_have_both_org_and_space_set", "Cannot pass both 'organization' and 'space' in a create role request", false) }, func(ut ut.Translator, fe validator.FieldError) string { @@ -262,15 +287,25 @@ func checkRoleTypeAndOrgSpace(sl validator.StructLevel) { } } -// Custom field validators -func megabyteFormattedString(fl validator.FieldLevel) bool { +var unitAmount = regexp.MustCompile(`^\d+(?:B|K|KB|M|MB|G|GB|T|TB)$`) + +func amountWithUnit(fl validator.FieldLevel) bool { + val, ok := fl.Field().Interface().(string) + if !ok { + return true // the value is optional, and is set to nil + } + + return unitAmount.MatchString(val) +} + +func positiveAmountWithUnit(fl validator.FieldLevel) bool { val, ok := fl.Field().Interface().(string) if !ok { return true // the value is optional, and is set to nil } - _, err := bytefmt.ToMegabytes(val) - return err == nil + mbs, err := bytefmt.ToMegabytes(val) + return err == nil && mbs > 0 } func routeString(fl validator.FieldLevel) bool { diff --git a/api/handlers/space_manifest_test.go b/api/handlers/space_manifest_test.go index ecfabc77a..2f617973b 100644 --- a/api/handlers/space_manifest_test.go +++ b/api/handlers/space_manifest_test.go @@ -128,18 +128,17 @@ var _ = Describe("SpaceManifest", func() { }) }) - When("the manifest contains multiple apps", func() { + When("the application name is missing", func() { BeforeEach(func() { requestBody = strings.NewReader(`--- version: 1 applications: - - name: app1 - - name: app2 + - {} `) }) It("response with an unprocessable entity error", func() { - expectUnprocessableEntityError("Applications must contain at maximum 1 item") + expectUnprocessableEntityError("Name is a required field") }) It("doesn't call applyManifestAction", func() { @@ -147,21 +146,18 @@ var _ = Describe("SpaceManifest", func() { }) }) - When("the application name is missing", func() { + When("the application memory does not have a unit", func() { BeforeEach(func() { requestBody = strings.NewReader(`--- version: 1 applications: - - {} + - name: test-app + memory: 1024 `) }) It("response with an unprocessable entity error", func() { - expectUnprocessableEntityError("Name is a required field") - }) - - It("doesn't call applyManifestAction", func() { - Expect(manifestApplier.ApplyCallCount()).To(Equal(0)) + expectUnprocessableEntityError("Memory must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB") }) }) @@ -171,16 +167,12 @@ var _ = Describe("SpaceManifest", func() { version: 1 applications: - name: test-app - memory: 0 + memory: 0M `) }) It("response with an unprocessable entity error", func() { - expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Memory' Error:Field validation for 'Memory' failed on the 'megabytestring' tag") - }) - - It("doesn't call applyManifestAction", func() { - Expect(manifestApplier.ApplyCallCount()).To(Equal(0)) + expectUnprocessableEntityError("Memory must be greater than 0MB") }) }) @@ -225,6 +217,23 @@ var _ = Describe("SpaceManifest", func() { }) }) + When("the application process disk doesn't supply a unit", func() { + BeforeEach(func() { + requestBody = strings.NewReader(`--- + version: 1 + applications: + - name: test-app + processes: + - type: web + disk_quota: 1024 + `) + }) + + It("response with an unprocessable entity error", func() { + expectUnprocessableEntityError("DiskQuota must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB") + }) + }) + When("the application process disk is not a positive integer", func() { BeforeEach(func() { requestBody = strings.NewReader(`--- @@ -233,16 +242,29 @@ var _ = Describe("SpaceManifest", func() { - name: test-app processes: - type: web - disk_quota: 0 + disk_quota: 0M `) }) It("response with an unprocessable entity error", func() { - expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Processes[0].DiskQuota' Error:Field validation for 'DiskQuota' failed on the 'megabytestring' tag") + expectUnprocessableEntityError("DiskQuota must be greater than 0MB") }) + }) - It("doesn't call applyManifestAction", func() { - Expect(manifestApplier.ApplyCallCount()).To(Equal(0)) + When("the application process memory doesn't supply a unit", func() { + BeforeEach(func() { + requestBody = strings.NewReader(`--- + version: 1 + applications: + - name: test-app + processes: + - type: web + memory: 1024 + `) + }) + + It("response with an unprocessable entity error", func() { + expectUnprocessableEntityError("Memory must use a supported unit: B, K, KB, M, MB, G, GB, T, or TB") }) }) @@ -254,12 +276,12 @@ var _ = Describe("SpaceManifest", func() { - name: test-app processes: - type: web - memory: 0 + memory: 0GB `) }) It("response with an unprocessable entity error", func() { - expectUnprocessableEntityError("Key: 'Manifest.Applications[0].Processes[0].Memory' Error:Field validation for 'Memory' failed on the 'megabytestring' tag") + expectUnprocessableEntityError("Memory must be greater than 0MB") }) It("doesn't call applyManifestAction", func() { diff --git a/api/payloads/manifest.go b/api/payloads/manifest.go index 2be9599c3..72d274525 100644 --- a/api/payloads/manifest.go +++ b/api/payloads/manifest.go @@ -10,7 +10,7 @@ import ( type Manifest struct { Version int `yaml:"version"` - Applications []ManifestApplication `yaml:"applications"` + Applications []ManifestApplication `yaml:"applications" validate:"dive"` } type ManifestApplication struct { @@ -21,13 +21,13 @@ type ManifestApplication struct { NoRoute bool `yaml:"no-route"` Command *string `yaml:"command"` Instances *int `yaml:"instances" validate:"omitempty,gte=0"` - Memory *string `yaml:"memory" validate:"megabytestring"` - DiskQuota *string `yaml:"disk_quota" validate:"megabytestring"` + Memory *string `yaml:"memory" validate:"amountWithUnit,positiveAmountWithUnit"` + DiskQuota *string `yaml:"disk_quota" validate:"amountWithUnit,positiveAmountWithUnit"` // AltDiskQuota supports `disk-quota` with a hyphen for backwards compatibility. // Do not set both DiskQuota and AltDiskQuota. // // Deprecated: Use DiskQuota instead - AltDiskQuota *string `yaml:"disk-quota" validate:"megabytestring"` + AltDiskQuota *string `yaml:"disk-quota" validate:"amountWithUnit,positiveAmountWithUnit"` HealthCheckHTTPEndpoint *string `yaml:"health-check-http-endpoint"` HealthCheckInvocationTimeout *int64 `yaml:"health-check-invocation-timeout" validate:"omitempty,gte=1"` HealthCheckType *string `yaml:"health-check-type" validate:"omitempty,oneof=none process port http"` @@ -42,17 +42,17 @@ type ManifestApplication struct { type ManifestApplicationProcess struct { Type string `yaml:"type" validate:"required"` Command *string `yaml:"command"` - DiskQuota *string `yaml:"disk_quota" validate:"megabytestring"` + DiskQuota *string `yaml:"disk_quota" validate:"amountWithUnit,positiveAmountWithUnit"` // AltDiskQuota supports `disk-quota` with a hyphen for backwards compatibility. // Do not set both DiskQuota and AltDiskQuota. // // Deprecated: Use DiskQuota instead - AltDiskQuota *string `yaml:"disk-quota" validate:"megabytestring"` + AltDiskQuota *string `yaml:"disk-quota" validate:"amountWithUnit,positiveAmountWithUnit"` HealthCheckHTTPEndpoint *string `yaml:"health-check-http-endpoint"` HealthCheckInvocationTimeout *int64 `yaml:"health-check-invocation-timeout" validate:"omitempty,gte=1"` HealthCheckType *string `yaml:"health-check-type" validate:"omitempty,oneof=none process port http"` Instances *int `yaml:"instances" validate:"omitempty,gte=0"` - Memory *string `yaml:"memory" validate:"megabytestring"` + Memory *string `yaml:"memory" validate:"amountWithUnit,positiveAmountWithUnit"` Timeout *int64 `yaml:"timeout" validate:"omitempty,gte=1"` }