From cc58eae60664cdf0a02bd7b19fb8fe74de7590e3 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 4 Jun 2021 11:16:08 -0400 Subject: [PATCH 01/13] adjusts sidecar parameters due to adjusted conversion logic --- go.mod | 1 + go.sum | 3 + .../cloudformation/stack/backend_svc.go | 2 +- .../deploy/cloudformation/stack/lb_web_svc.go | 2 +- .../cloudformation/stack/scheduled_job.go | 2 +- .../cloudformation/stack/transformers.go | 20 +- .../cloudformation/stack/transformers_test.go | 188 +++++++++++++++++- 7 files changed, 213 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index d59cdb7e7f0..996aa1c5254 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 github.com/xlab/treeprint v1.1.0 + github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26 golang.org/x/mod v0.4.1 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9 gopkg.in/ini.v1 v1.62.0 diff --git a/go.sum b/go.sum index ca04b4fd3a8..613f27abb3b 100644 --- a/go.sum +++ b/go.sum @@ -340,6 +340,7 @@ github.com/gobuffalo/logger v1.0.3 h1:YaXOTHNPCvkqqA7w05A4v0k2tCdpr+sgFlgINbQ6gq github.com/gobuffalo/logger v1.0.3/go.mod h1:SoeejUwldiS7ZsyCBphOGURmWdwUFXs0J7TCjEhjKxM= github.com/gobuffalo/packd v1.0.0 h1:6ERZvJHfe24rfFmA9OaoKBdC7+c9sydrytMg8SdFGBM= github.com/gobuffalo/packd v1.0.0/go.mod h1:6VTc4htmJRFB7u1m/4LeMTWjFoYrUiBkU9Fdec9hrhI= +github.com/gobuffalo/packr v1.30.1 h1:hu1fuVR3fXEZR7rXNW3h8rqSML8EVAf6KNm0NKO/wKg= github.com/gobuffalo/packr/v2 v2.8.1 h1:tkQpju6i3EtMXJ9uoF5GT6kB+LMTimDWD8Xvbz6zDVA= github.com/gobuffalo/packr/v2 v2.8.1/go.mod h1:c/PLlOuTU+p3SybaJATW3H6lX/iK7xEz5OeMf+NnJpg= github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8= @@ -911,6 +912,8 @@ github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q github.com/xlab/treeprint v1.1.0 h1:G/1DjNkPpfZCFt9CSh6b5/nY4VimlbHF3Rh4obvtzDk= github.com/xlab/treeprint v1.1.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0= github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q= +github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26 h1:4u7nCRnWizT8R6xOP7cGaq+Ov0oBGkKMsLWZKiwDFas= +github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26/go.mod h1:Rfzr+sqaDreiCaoQbFCu3sTXxeFq/9kXRuyOoSlGQHE= github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= diff --git a/internal/pkg/deploy/cloudformation/stack/backend_svc.go b/internal/pkg/deploy/cloudformation/stack/backend_svc.go index dedfbdb59b6..889af79dcd7 100644 --- a/internal/pkg/deploy/cloudformation/stack/backend_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/backend_svc.go @@ -81,7 +81,7 @@ func (s *BackendService) Template() (string, error) { if err != nil { return "", err } - sidecars, err := convertSidecar(s.manifest.Sidecars) + sidecars, err := convertSidecar(s.manifest.Sidecars, s.manifest.ImageConfig.Image, *s.manifest.Name) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for service %s: %w", s.name, err) } diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go index 4d306fc7e39..af6de1db57e 100644 --- a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go @@ -106,7 +106,7 @@ func (s *LoadBalancedWebService) Template() (string, error) { if err != nil { return "", err } - sidecars, err := convertSidecar(s.manifest.Sidecars) + sidecars, err := convertSidecar(s.manifest.Sidecars, s.manifest.ImageConfig.Image, *s.manifest.Name) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for service %s: %w", s.name, err) } diff --git a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go index d7ebb153f6a..140f77d53f3 100644 --- a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go +++ b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go @@ -125,7 +125,7 @@ func (j *ScheduledJob) Template() (string, error) { return "", err } - sidecars, err := convertSidecar(j.manifest.Sidecars) + sidecars, err := convertSidecar(j.manifest.Sidecars, j.manifest.ImageConfig, *j.manifest.Name) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for job %s: %w", j.name, err) } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 5a0eb9d8e9d..6775a635357 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -51,10 +51,13 @@ var ( ) // convertSidecar converts the manifest sidecar configuration into a format parsable by the templates pkg. -func convertSidecar(s map[string]*manifest.SidecarConfig) ([]*template.SidecarOpts, error) { +func convertSidecar(s map[string]*manifest.SidecarConfig, i manifest.Image, m string) ([]*template.SidecarOpts, error) { if s == nil { return nil, nil } + if err := validateNoCircularDependencies(s, i, m); err != nil { + return nil, err + } var sidecars []*template.SidecarOpts for name, config := range s { port, protocol, err := parsePortMapping(config.Port) @@ -64,6 +67,9 @@ func convertSidecar(s map[string]*manifest.SidecarConfig) ([]*template.SidecarOp if err := validateSidecarMountPoints(config.MountPoints); err != nil { return nil, err } + if err := validateSidecarDependsOn(*config, name, s, m); err != nil { + return nil, err + } mp := convertSidecarMountPoints(config.MountPoints) sidecars = append(sidecars, &template.SidecarOpts{ @@ -77,11 +83,23 @@ func convertSidecar(s map[string]*manifest.SidecarConfig) ([]*template.SidecarOp Variables: config.Variables, MountPoints: mp, DockerLabels: config.DockerLabels, + DependsOn: config.DependsOn, }) } return sidecars, nil } +// convertDependsOn converts an Image DependsOn field to a template DependsOn version +func convertDependsOn(i *manifest.Image, s map[string]*manifest.SidecarConfig, m string) (map[string]string, error) { + if i == nil || i.DependsOn == nil { + return nil, nil + } + if err := validateImageDependsOn(*i, s, m); err != nil { + return nil, err + } + return i.DependsOn, nil +} + // Valid sidecar portMapping example: 2000/udp, or 2000 (default to be tcp). func parsePortMapping(s *string) (port *string, protocol *string, err error) { if s == nil { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index d0115313173..5bf41a1274b 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -16,12 +16,15 @@ import ( func Test_convertSidecar(t *testing.T) { mockImage := aws.String("mockImage") + mockManifestName := "frontend" mockMap := map[string]string{"foo": "bar"} mockCredsParam := aws.String("mockCredsParam") testCases := map[string]struct { inPort string inEssential bool inLabels map[string]string + inDependsOn map[string]string + inImg manifest.Image wanted *template.SidecarOpts wantedErr error @@ -60,6 +63,84 @@ func Test_convertSidecar(t *testing.T) { Essential: aws.Bool(true), }, }, + "invalid container dependency due to circularly depending on itself": { + inPort: "2000", + inEssential: true, + inDependsOn: map[string]string{ + "foo": "start", + }, + + wantedErr: errCircularDependency, + }, + "invalid container dependency due to circularly depending on another container": { + inPort: "2000", + inEssential: true, + inDependsOn: map[string]string{ + "frontend": "start", + }, + inImg: manifest.Image{ + DependsOn: map[string]string{ + "foo": "start", + }, + }, + wantedErr: errCircularDependency, + }, + "invalid container dependency status": { + inPort: "2000", + inEssential: true, + inDependsOn: map[string]string{ + "frontend": "never", + }, + wantedErr: errInvalidDependsOnStatus, + }, + "invalid essential container dependency status": { + inPort: "2000", + inEssential: true, + inDependsOn: map[string]string{ + "frontend": "complete", + }, + wantedErr: errEssentialContainerStatus, + }, + "good essential container dependencies": { + inPort: "2000", + inEssential: true, + inDependsOn: map[string]string{ + "frontend": "start", + }, + + wanted: &template.SidecarOpts{ + Name: aws.String("foo"), + Port: aws.String("2000"), + CredsParam: mockCredsParam, + Image: mockImage, + Secrets: mockMap, + Variables: mockMap, + Essential: aws.Bool(true), + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + }, + "good nonessential container dependencies": { + inPort: "2000", + inEssential: false, + inDependsOn: map[string]string{ + "frontend": "start", + }, + + wanted: &template.SidecarOpts{ + Name: aws.String("foo"), + Port: aws.String("2000"), + CredsParam: mockCredsParam, + Image: mockImage, + Secrets: mockMap, + Variables: mockMap, + Essential: aws.Bool(false), + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + }, "specify essential as false": { inPort: "2000", inEssential: false, @@ -92,9 +173,10 @@ func Test_convertSidecar(t *testing.T) { Essential: aws.Bool(tc.inEssential), Port: aws.String(tc.inPort), DockerLabels: tc.inLabels, + DependsOn: tc.inDependsOn, }, } - got, err := convertSidecar(sidecar) + got, err := convertSidecar(sidecar, tc.inImg, mockManifestName) if tc.wantedErr != nil { require.EqualError(t, err, tc.wantedErr.Error()) @@ -1155,3 +1237,107 @@ func Test_convertEphemeral(t *testing.T) { }) } } + +func Test_convertDependsOn(t *testing.T) { + mockManifestName := "frontend" + testCases := map[string]struct { + inImage *manifest.Image + inSidecars map[string]*manifest.SidecarConfig + + wanted map[string]string + wantedError error + }{ + "no container dependencies": { + inImage: &manifest.Image{}, + wanted: nil, + }, + "invalid container dependency due to circular dependency on itself": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "frontend": "end", + }, + }, + wantedError: errCircularDependency, + }, + "invalid container dependency due to circular dependency on a sidecar": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "sidecar2": "start", + }, + }, + "sidecar2": { + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + }, + wantedError: errCircularDependency, + }, + "invalid container dependency due to status": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "end", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + Essential: aws.Bool(false), + }, + }, + wantedError: errInvalidDependsOnStatus, + }, + "invalid implied essential container depdendency": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "complete", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantedError: errEssentialContainerStatus, + }, + "invalid set essential container depdendency": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "complete", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + Essential: aws.Bool(true), + }, + }, + wantedError: errEssentialContainerStatus, + }, + "good essential container dependency": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wanted: map[string]string{ + "sidecar": "start", + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + got, err := convertDependsOn(tc.inImage, tc.inSidecars, mockManifestName) + if tc.wantedError != nil { + require.EqualError(t, err, tc.wantedError.Error()) + } else { + require.Equal(t, got, tc.wanted) + } + }) + } +} From c3e3f25837b165b4044b4c12b67b142f61f70089 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 4 Jun 2021 11:14:49 -0400 Subject: [PATCH 02/13] adds dependency validation logic --- .../deploy/cloudformation/stack/validate.go | 139 +++++++++ .../cloudformation/stack/validate_test.go | 279 ++++++++++++++++++ 2 files changed, 418 insertions(+) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 2a784f0b1ec..766cac81340 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/manifest" + "github.com/yourbasic/graph" ) // Validation errors when rendering manifest into template. @@ -29,6 +30,15 @@ var ( errInvalidUIDGIDConfig = errors.New("must specify both UID and GID, or neither") errInvalidEFSConfig = errors.New("bad EFS configuration: cannot specify both bool and config") errReservedUID = errors.New("UID must not be 0") + errCircularDependency = errors.New("Bad dependency name: circular dependency present") + errInvalidContainer = errors.New("Container dependency does not exist") + errInvalidDependsOnStatus = errors.New("Container dependency status must be one of < start | complete | success >") + errEssentialContainerStatus = errors.New("Essential containers dependencies can only have status 'start'") +) + +var ( + // Container dependency status options + DependsOnStatus = []string{"start", "complete", "success"} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -100,6 +110,135 @@ func validateSidecarMountPoints(in []manifest.SidecarMountPoint) error { return nil } +func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sidecars map[string]*manifest.SidecarConfig, manifestName string) error { + if in.DependsOn == nil { + return nil + } + + for name, status := range in.DependsOn { + // don't let a sidecar depend on itself + if name == sidecarName { + return errCircularDependency + } + // status must be one of < start | complete | success > + if !isValidStatus(status) { + return errInvalidDependsOnStatus + } + // essential containers must have 'start' as a status + if name == manifestName || sidecars[name].Essential == nil || *sidecars[name].Essential == true { + if status != "start" { + return errEssentialContainerStatus + } + } + } + + return nil +} + +func validateNoCircularDependencies(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, manifestName string) error { + // don't let anything circularly depend on each other + dependencies := buildDependencyGraph(sidecars, img, manifestName) + + // don't let invalid containers happen + if dependencies == nil { + return errInvalidContainer + } + + // don't let circular dependencies happen + if !graph.Acyclic(dependencies) { + return errCircularDependency + } + + return nil +} + +func buildDependencyGraph(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, manifestName string) *graph.Mutable { + var currNode, depNode int + last := 1 + nodes := map[string]int{} + dependencies := graph.New(len(sidecars) + 1) + + // sidecar dependencies + for name, sidecar := range sidecars { + // add any existing dependency to the graph + if len(sidecar.DependsOn) != 0 { + currNode, nodes, last = getNode(name, nodes, last) + + for dep := range sidecar.DependsOn { + // containers being depended on must exist + if sidecars[dep] == nil && dep != manifestName { + return nil + } + + depNode, nodes, last = getNode(dep, nodes, last) + dependencies.Add(currNode, depNode) + } + } + } + + // add any image dependencies to the graph + if len(img.DependsOn) != 0 { + currNode, nodes, last = getNode(manifestName, nodes, last) + + for dep := range img.DependsOn { + // containers being depended on must exist + if sidecars[dep] == nil { + return nil + } + + depNode, nodes, last = getNode(dep, nodes, last) + dependencies.Add(currNode, depNode) + } + } + + return dependencies +} + +func getNode(name string, nodes map[string]int, last int) (int, map[string]int, int) { + if nodes[name] != 0 { + return nodes[name] - 1, nodes, last + } + nodes[name] = last + return nodes[name] - 1, nodes, last + 1 +} + +func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.SidecarConfig, manifestName string) error { + if img.DependsOn == nil { + return nil + } + + for name, status := range img.DependsOn { + // don't let image depend on itself + if name == manifestName { + return errCircularDependency + } + // status must be one of < start | complete | success > + if !isValidStatus(status) { + return errInvalidDependsOnStatus + } + // essential containers must have 'start' as a status + if sidecars != nil { + if sidecars[name].Essential == nil || *sidecars[name].Essential == true { + if status != "start" { + return errEssentialContainerStatus + } + } + } + } + + return validateNoCircularDependencies(sidecars, img, manifestName) +} + +func isValidStatus(s string) bool { + for _, allowed := range DependsOnStatus { + if s == allowed { + return true + } + } + + return false +} + func validateEFSConfig(in manifest.Volume) error { // EFS is implicitly disabled. We don't use the attached EmptyVolume function here // because it may hide invalid config. diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index c2478050832..5d84b472681 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -152,3 +152,282 @@ func Test_validateEFSConfig(t *testing.T) { }) } } + +func TestValidateSidecarDependsOn(t *testing.T) { + mockSidecarName := "sidecar" + mockManifestName := "frontend" + testCases := map[string]struct { + inSidecar *manifest.SidecarConfig + allSidecars map[string]*manifest.SidecarConfig + + wantErr error + }{ + "No sidecar dependencies": { + inSidecar: &manifest.SidecarConfig{}, + wantErr: nil, + }, + "Working set essential sidecar with container dependency": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "sidecar1": "start", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar1": { + Essential: aws.Bool(true), + }, + }, + wantErr: nil, + }, + "Working implied essential container with container dependency": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: nil, + }, + "Working non-essential sidecar with container dependency": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "sidecar2": "complete", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + "sidecar2": { + Essential: aws.Bool(false), + }, + }, + wantErr: nil, + }, + "Error when sidecar container dependency status is invalid": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "sidecar2": "end", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + "sidecar2": { + Essential: aws.Bool(false), + }, + }, + wantErr: errInvalidDependsOnStatus, + }, + "Error when set essential sidecar has a status besides start": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "sidecar2": "complete", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + "sidecar2": { + Essential: aws.Bool(true), + }, + }, + wantErr: errEssentialContainerStatus, + }, + "Error when implied essential sidecar has a status besides start": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "frontend": "complete", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: errEssentialContainerStatus, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + gotErr := validateSidecarDependsOn(*tc.inSidecar, mockSidecarName, tc.allSidecars, mockManifestName) + if tc.wantErr == nil { + require.NoError(t, gotErr) + } else { + require.EqualError(t, gotErr, tc.wantErr.Error()) + } + }) + } +} + +func TestValidateNoCircularDependency(t *testing.T) { + mockManifestName := "frontend" + image := manifest.Image{} + testCases := map[string]struct { + allSidecars map[string]*manifest.SidecarConfig + + wantErr error + }{ + "No sidecar dependencies": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: nil, + }, + "Working sidecars with container dependency": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + }, + wantErr: nil, + }, + "Error when sidecar depends on itself": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + }, + wantErr: errCircularDependency, + }, + "Error when sidecars circularly depend on each other": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + "frontend": { + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + }, + wantErr: errCircularDependency, + }, + "Error when sidecars inadvertently depend on each other": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "secondCar": "start", + }, + }, + "secondCar": { + DependsOn: map[string]string{ + "thirdCar": "start", + }, + }, + "thirdCar": { + DependsOn: map[string]string{ + "fourthCar": "start", + }, + }, + "fourthCar": { + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + }, + wantErr: errCircularDependency, + }, + "Error when container doesn't exist": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "something": "start", + }, + }, + }, + wantErr: errInvalidContainer, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + gotErr := validateNoCircularDependencies(tc.allSidecars, image, mockManifestName) + if tc.wantErr == nil { + require.NoError(t, gotErr) + } else { + require.EqualError(t, gotErr, tc.wantErr.Error()) + } + }) + } +} + +func TestValidateImageDependsOn(t *testing.T) { + mockManifestName := "frontend" + testCases := map[string]struct { + inImage *manifest.Image + inSidecars map[string]*manifest.SidecarConfig + + wantErr error + }{ + "No image container dependencies": { + inImage: &manifest.Image{}, + wantErr: nil, + }, + "Working image with container dependency": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "start", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: nil, + }, + "Error when image depends on itself": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "frontend": "start", + }, + }, + wantErr: errCircularDependency, + }, + "Error when image container dependency status is invalid": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "end", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: errInvalidDependsOnStatus, + }, + "Error when set essential container has a status besides start": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "complete", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + Essential: aws.Bool(true), + }, + }, + wantErr: errEssentialContainerStatus, + }, + "Error when implied essential container has a status besides start": { + inImage: &manifest.Image{ + DependsOn: map[string]string{ + "sidecar": "complete", + }, + }, + inSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, + wantErr: errEssentialContainerStatus, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + gotErr := validateImageDependsOn(*tc.inImage, tc.inSidecars, mockManifestName) + if tc.wantErr == nil { + require.NoError(t, gotErr) + } else { + require.EqualError(t, gotErr, tc.wantErr.Error()) + } + }) + } +} From a6567cee7f560fad8ec2d3d104c8a208969a519b Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 4 Jun 2021 13:19:55 -0400 Subject: [PATCH 03/13] simplifies booleans --- internal/pkg/deploy/cloudformation/stack/validate.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 766cac81340..cc07f064e76 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -125,7 +125,7 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sid return errInvalidDependsOnStatus } // essential containers must have 'start' as a status - if name == manifestName || sidecars[name].Essential == nil || *sidecars[name].Essential == true { + if name == manifestName || sidecars[name].Essential == nil || *sidecars[name].Essential { if status != "start" { return errEssentialContainerStatus } @@ -218,7 +218,7 @@ func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.Si } // essential containers must have 'start' as a status if sidecars != nil { - if sidecars[name].Essential == nil || *sidecars[name].Essential == true { + if sidecars[name].Essential == nil || *sidecars[name].Essential { if status != "start" { return errEssentialContainerStatus } From ed913b2e9c0da14266a3ceb00f81380496a26242 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Fri, 4 Jun 2021 16:54:14 -0400 Subject: [PATCH 04/13] adjusts formatting clarifies convertDependsOn adjusts errors to be lowercase --- .../cloudformation/stack/transformers.go | 2 +- .../cloudformation/stack/transformers_test.go | 4 +- .../deploy/cloudformation/stack/validate.go | 8 ++-- .../cloudformation/stack/validate_test.go | 38 +++++++++---------- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 6775a635357..fb6d3d36694 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -90,7 +90,7 @@ func convertSidecar(s map[string]*manifest.SidecarConfig, i manifest.Image, m st } // convertDependsOn converts an Image DependsOn field to a template DependsOn version -func convertDependsOn(i *manifest.Image, s map[string]*manifest.SidecarConfig, m string) (map[string]string, error) { +func convertImageDependsOn(i *manifest.Image, s map[string]*manifest.SidecarConfig, m string) (map[string]string, error) { if i == nil || i.DependsOn == nil { return nil, nil } diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 5bf41a1274b..f9b62eb497f 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1238,7 +1238,7 @@ func Test_convertEphemeral(t *testing.T) { } } -func Test_convertDependsOn(t *testing.T) { +func Test_convertImageDependsOn(t *testing.T) { mockManifestName := "frontend" testCases := map[string]struct { inImage *manifest.Image @@ -1332,7 +1332,7 @@ func Test_convertDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - got, err := convertDependsOn(tc.inImage, tc.inSidecars, mockManifestName) + got, err := convertImageDependsOn(tc.inImage, tc.inSidecars, mockManifestName) if tc.wantedError != nil { require.EqualError(t, err, tc.wantedError.Error()) } else { diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index cc07f064e76..c522c24f443 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -30,10 +30,10 @@ var ( errInvalidUIDGIDConfig = errors.New("must specify both UID and GID, or neither") errInvalidEFSConfig = errors.New("bad EFS configuration: cannot specify both bool and config") errReservedUID = errors.New("UID must not be 0") - errCircularDependency = errors.New("Bad dependency name: circular dependency present") - errInvalidContainer = errors.New("Container dependency does not exist") - errInvalidDependsOnStatus = errors.New("Container dependency status must be one of < start | complete | success >") - errEssentialContainerStatus = errors.New("Essential containers dependencies can only have status 'start'") + errCircularDependency = errors.New("bad dependency name: circular dependency present") + errInvalidContainer = errors.New("container dependency does not exist") + errInvalidDependsOnStatus = errors.New("container dependency status must be one of < start | complete | success >") + errEssentialContainerStatus = errors.New("essential container dependencies can only have status 'start'") ) var ( diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 5d84b472681..4b3fefb7277 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -162,11 +162,11 @@ func TestValidateSidecarDependsOn(t *testing.T) { wantErr error }{ - "No sidecar dependencies": { + "no sidecar dependencies": { inSidecar: &manifest.SidecarConfig{}, wantErr: nil, }, - "Working set essential sidecar with container dependency": { + "working set essential sidecar with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "sidecar1": "start", @@ -179,7 +179,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { }, wantErr: nil, }, - "Working implied essential container with container dependency": { + "working implied essential container with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "frontend": "start", @@ -190,7 +190,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { }, wantErr: nil, }, - "Working non-essential sidecar with container dependency": { + "working non-essential sidecar with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "sidecar2": "complete", @@ -204,7 +204,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { }, wantErr: nil, }, - "Error when sidecar container dependency status is invalid": { + "error when sidecar container dependency status is invalid": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "sidecar2": "end", @@ -218,7 +218,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { }, wantErr: errInvalidDependsOnStatus, }, - "Error when set essential sidecar has a status besides start": { + "error when set essential sidecar has a status besides start": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "sidecar2": "complete", @@ -232,7 +232,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { }, wantErr: errEssentialContainerStatus, }, - "Error when implied essential sidecar has a status besides start": { + "error when implied essential sidecar has a status besides start": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "frontend": "complete", @@ -264,13 +264,13 @@ func TestValidateNoCircularDependency(t *testing.T) { wantErr error }{ - "No sidecar dependencies": { + "no sidecar dependencies": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, wantErr: nil, }, - "Working sidecars with container dependency": { + "working sidecars with container dependency": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ @@ -280,7 +280,7 @@ func TestValidateNoCircularDependency(t *testing.T) { }, wantErr: nil, }, - "Error when sidecar depends on itself": { + "error when sidecar depends on itself": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ @@ -290,7 +290,7 @@ func TestValidateNoCircularDependency(t *testing.T) { }, wantErr: errCircularDependency, }, - "Error when sidecars circularly depend on each other": { + "error when sidecars circularly depend on each other": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ @@ -305,7 +305,7 @@ func TestValidateNoCircularDependency(t *testing.T) { }, wantErr: errCircularDependency, }, - "Error when sidecars inadvertently depend on each other": { + "error when sidecars inadvertently depend on each other": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ @@ -330,7 +330,7 @@ func TestValidateNoCircularDependency(t *testing.T) { }, wantErr: errCircularDependency, }, - "Error when container doesn't exist": { + "error when container doesn't exist": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ @@ -361,11 +361,11 @@ func TestValidateImageDependsOn(t *testing.T) { wantErr error }{ - "No image container dependencies": { + "no image container dependencies": { inImage: &manifest.Image{}, wantErr: nil, }, - "Working image with container dependency": { + "working image with container dependency": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "start", @@ -376,7 +376,7 @@ func TestValidateImageDependsOn(t *testing.T) { }, wantErr: nil, }, - "Error when image depends on itself": { + "error when image depends on itself": { inImage: &manifest.Image{ DependsOn: map[string]string{ "frontend": "start", @@ -384,7 +384,7 @@ func TestValidateImageDependsOn(t *testing.T) { }, wantErr: errCircularDependency, }, - "Error when image container dependency status is invalid": { + "error when image container dependency status is invalid": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "end", @@ -395,7 +395,7 @@ func TestValidateImageDependsOn(t *testing.T) { }, wantErr: errInvalidDependsOnStatus, }, - "Error when set essential container has a status besides start": { + "error when set essential container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "complete", @@ -408,7 +408,7 @@ func TestValidateImageDependsOn(t *testing.T) { }, wantErr: errEssentialContainerStatus, }, - "Error when implied essential container has a status besides start": { + "error when implied essential container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "complete", From f20095d471c69ce5e2f7073251b0179ff5189dc8 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Mon, 7 Jun 2021 15:59:18 -0400 Subject: [PATCH 05/13] adjusts graph, addreses comments adjusts graph to custom version adds datastructure for convert sidecar adds convertImageDependsOn to jobs Adjusts essential check to use aws.bool Removes unecessary circular dependency check Makes invalid container errors return themselves and not nil --- go.mod | 1 - go.sum | 3 +- .../cloudformation/stack/backend_svc.go | 12 +- .../deploy/cloudformation/stack/lb_web_svc.go | 12 +- .../cloudformation/stack/scheduled_job.go | 14 ++- .../cloudformation/stack/transformers.go | 24 ++-- .../cloudformation/stack/transformers_test.go | 18 ++- .../deploy/cloudformation/stack/validate.go | 115 +++++++++++------- .../cloudformation/stack/validate_test.go | 33 ++++- 9 files changed, 158 insertions(+), 74 deletions(-) diff --git a/go.mod b/go.mod index 996aa1c5254..d59cdb7e7f0 100644 --- a/go.mod +++ b/go.mod @@ -28,7 +28,6 @@ require ( github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 github.com/xlab/treeprint v1.1.0 - github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26 golang.org/x/mod v0.4.1 golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9 gopkg.in/ini.v1 v1.62.0 diff --git a/go.sum b/go.sum index 613f27abb3b..5000230fcfb 100644 --- a/go.sum +++ b/go.sum @@ -912,8 +912,6 @@ github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q github.com/xlab/treeprint v1.1.0 h1:G/1DjNkPpfZCFt9CSh6b5/nY4VimlbHF3Rh4obvtzDk= github.com/xlab/treeprint v1.1.0/go.mod h1:gj5Gd3gPdKtR1ikdDK6fnFLdmIS0X30kTTuNd/WEJu0= github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q= -github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26 h1:4u7nCRnWizT8R6xOP7cGaq+Ov0oBGkKMsLWZKiwDFas= -github.com/yourbasic/graph v0.0.0-20170921192928-40eb135c0b26/go.mod h1:Rfzr+sqaDreiCaoQbFCu3sTXxeFq/9kXRuyOoSlGQHE= github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= @@ -1220,6 +1218,7 @@ golang.org/x/tools v0.0.0-20200501065659-ab2804fb9c9d/go.mod h1:EkVYQZoAsY45+roY golang.org/x/tools v0.0.0-20200502202811-ed308ab3e770/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200512131952-2bc93b1c0c88/go.mod h1:EkVYQZoAsY45+roYkvgYkIh4xh/qjgUK9TdY2XT94GE= golang.org/x/tools v0.0.0-20200804011535-6c149bb5ef0d/go.mod h1:njjCfa9FT2d7l9Bc6FUM5FLjQPp3cFF28FI3qnDFljA= +golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e h1:4nW4NLDYnU28ojHaHO8OVxFHk/aQ33U01a9cjED+pzE= golang.org/x/tools v0.0.0-20201224043029-2b0845dc783e/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/pkg/deploy/cloudformation/stack/backend_svc.go b/internal/pkg/deploy/cloudformation/stack/backend_svc.go index 889af79dcd7..57cc6ca26c9 100644 --- a/internal/pkg/deploy/cloudformation/stack/backend_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/backend_svc.go @@ -81,10 +81,19 @@ func (s *BackendService) Template() (string, error) { if err != nil { return "", err } - sidecars, err := convertSidecar(s.manifest.Sidecars, s.manifest.ImageConfig.Image, *s.manifest.Name) + convSidecarOpts := convertSidecarOpts{ + sidecarConfig: s.manifest.Sidecars, + imageConfig: &s.manifest.ImageConfig.Image, + workloadName: *s.manifest.Name, + } + sidecars, err := convertSidecar(convSidecarOpts) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for service %s: %w", s.name, err) } + dependencies, err := convertImageDependsOn(convSidecarOpts) + if err != nil { + return "", fmt.Errorf("convert the container dependency for service %s: %w", s.name, err) + } advancedCount, err := convertAdvancedCount(&s.manifest.Count.AdvancedCount) if err != nil { @@ -132,6 +141,7 @@ func (s *BackendService) Template() (string, error) { Network: convertNetworkConfig(s.manifest.Network), EntryPoint: entrypoint, Command: command, + DependsOn: dependencies, }) if err != nil { return "", fmt.Errorf("parse backend service template: %w", err) diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go index af6de1db57e..7e50854f3ea 100644 --- a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go @@ -106,10 +106,19 @@ func (s *LoadBalancedWebService) Template() (string, error) { if err != nil { return "", err } - sidecars, err := convertSidecar(s.manifest.Sidecars, s.manifest.ImageConfig.Image, *s.manifest.Name) + convSidecarOpts := convertSidecarOpts{ + sidecarConfig: s.manifest.Sidecars, + imageConfig: &s.manifest.ImageConfig.Image, + workloadName: *s.manifest.Name, + } + sidecars, err := convertSidecar(convSidecarOpts) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for service %s: %w", s.name, err) } + dependencies, err := convertImageDependsOn(convSidecarOpts) + if err != nil { + return "", fmt.Errorf("convert the container dependency for service %s: %w", s.name, err) + } advancedCount, err := convertAdvancedCount(&s.manifest.Count.AdvancedCount) if err != nil { @@ -168,6 +177,7 @@ func (s *LoadBalancedWebService) Template() (string, error) { Network: convertNetworkConfig(s.manifest.Network), EntryPoint: entrypoint, Command: command, + DependsOn: dependencies, }) if err != nil { return "", err diff --git a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go index 140f77d53f3..ef37dca1ced 100644 --- a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go +++ b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go @@ -124,12 +124,19 @@ func (j *ScheduledJob) Template() (string, error) { if err != nil { return "", err } - - sidecars, err := convertSidecar(j.manifest.Sidecars, j.manifest.ImageConfig, *j.manifest.Name) + convSidecarOpts := convertSidecarOpts{ + sidecarConfig: j.manifest.Sidecars, + imageConfig: &j.manifest.ImageConfig, + workloadName: *j.manifest.Name, + } + sidecars, err := convertSidecar(convSidecarOpts) if err != nil { return "", fmt.Errorf("convert the sidecar configuration for job %s: %w", j.name, err) } - + dependencies, err := convertImageDependsOn(convSidecarOpts) + if err != nil { + return "", fmt.Errorf("convert container dependency for job %s: %w", j.name, err) + } schedule, err := j.awsSchedule() if err != nil { return "", fmt.Errorf("convert schedule for job %s: %w", j.name, err) @@ -171,6 +178,7 @@ func (j *ScheduledJob) Template() (string, error) { Network: convertNetworkConfig(j.manifest.Network), EntryPoint: entrypoint, Command: command, + DependsOn: dependencies, EnvControllerLambda: envControllerLambda.String(), }) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index fb6d3d36694..26ddb46dae6 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -51,15 +51,15 @@ var ( ) // convertSidecar converts the manifest sidecar configuration into a format parsable by the templates pkg. -func convertSidecar(s map[string]*manifest.SidecarConfig, i manifest.Image, m string) ([]*template.SidecarOpts, error) { - if s == nil { +func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { + if s.sidecarConfig == nil { return nil, nil } - if err := validateNoCircularDependencies(s, i, m); err != nil { + if err := validateNoCircularDependencies(s.sidecarConfig, *s.imageConfig, s.workloadName); err != nil { return nil, err } var sidecars []*template.SidecarOpts - for name, config := range s { + for name, config := range s.sidecarConfig { port, protocol, err := parsePortMapping(config.Port) if err != nil { return nil, err @@ -67,7 +67,7 @@ func convertSidecar(s map[string]*manifest.SidecarConfig, i manifest.Image, m st if err := validateSidecarMountPoints(config.MountPoints); err != nil { return nil, err } - if err := validateSidecarDependsOn(*config, name, s, m); err != nil { + if err := validateSidecarDependsOn(*config, name, s.sidecarConfig, s.workloadName); err != nil { return nil, err } mp := convertSidecarMountPoints(config.MountPoints) @@ -90,14 +90,20 @@ func convertSidecar(s map[string]*manifest.SidecarConfig, i manifest.Image, m st } // convertDependsOn converts an Image DependsOn field to a template DependsOn version -func convertImageDependsOn(i *manifest.Image, s map[string]*manifest.SidecarConfig, m string) (map[string]string, error) { - if i == nil || i.DependsOn == nil { +func convertImageDependsOn(s convertSidecarOpts) (map[string]string, error) { + if s.imageConfig == nil || s.imageConfig.DependsOn == nil { return nil, nil } - if err := validateImageDependsOn(*i, s, m); err != nil { + if err := validateImageDependsOn(*s.imageConfig, s.sidecarConfig, s.workloadName); err != nil { return nil, err } - return i.DependsOn, nil + return s.imageConfig.DependsOn, nil +} + +type convertSidecarOpts struct { + sidecarConfig map[string]*manifest.SidecarConfig + imageConfig *manifest.Image + workloadName string } // Valid sidecar portMapping example: 2000/udp, or 2000 (default to be tcp). diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index f9b62eb497f..ad8f9d63802 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -16,7 +16,7 @@ import ( func Test_convertSidecar(t *testing.T) { mockImage := aws.String("mockImage") - mockManifestName := "frontend" + mockWorkloadName := "frontend" mockMap := map[string]string{"foo": "bar"} mockCredsParam := aws.String("mockCredsParam") testCases := map[string]struct { @@ -176,7 +176,11 @@ func Test_convertSidecar(t *testing.T) { DependsOn: tc.inDependsOn, }, } - got, err := convertSidecar(sidecar, tc.inImg, mockManifestName) + got, err := convertSidecar(convertSidecarOpts{ + sidecarConfig: sidecar, + imageConfig: &tc.inImg, + workloadName: mockWorkloadName, + }) if tc.wantedErr != nil { require.EqualError(t, err, tc.wantedErr.Error()) @@ -1239,7 +1243,7 @@ func Test_convertEphemeral(t *testing.T) { } func Test_convertImageDependsOn(t *testing.T) { - mockManifestName := "frontend" + mockWorkloadName := "frontend" testCases := map[string]struct { inImage *manifest.Image inSidecars map[string]*manifest.SidecarConfig @@ -1254,7 +1258,7 @@ func Test_convertImageDependsOn(t *testing.T) { "invalid container dependency due to circular dependency on itself": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "frontend": "end", + "frontend": "start", }, }, wantedError: errCircularDependency, @@ -1332,7 +1336,11 @@ func Test_convertImageDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - got, err := convertImageDependsOn(tc.inImage, tc.inSidecars, mockManifestName) + got, err := convertImageDependsOn(convertSidecarOpts{ + sidecarConfig: tc.inSidecars, + imageConfig: tc.inImage, + workloadName: mockWorkloadName, + }) if tc.wantedError != nil { require.EqualError(t, err, tc.wantedError.Error()) } else { diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index c522c24f443..1dbb520389f 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -9,10 +9,14 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/copilot-cli/internal/pkg/manifest" - "github.com/yourbasic/graph" ) -// Validation errors when rendering manifest into template. +// container dependency status constants. +const ( + dependsOnStart = "start" + dependsOnComplete = "complete" + dependsOnSuccess = "success" +) // Empty field errors. var ( @@ -38,7 +42,7 @@ var ( var ( // Container dependency status options - DependsOnStatus = []string{"start", "complete", "success"} + DependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -110,22 +114,18 @@ func validateSidecarMountPoints(in []manifest.SidecarMountPoint) error { return nil } -func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sidecars map[string]*manifest.SidecarConfig, manifestName string) error { +func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sidecars map[string]*manifest.SidecarConfig, workloadName string) error { if in.DependsOn == nil { return nil } for name, status := range in.DependsOn { - // don't let a sidecar depend on itself - if name == sidecarName { - return errCircularDependency - } // status must be one of < start | complete | success > if !isValidStatus(status) { return errInvalidDependsOnStatus } // essential containers must have 'start' as a status - if name == manifestName || sidecars[name].Essential == nil || *sidecars[name].Essential { + if name == workloadName || sidecars[name].Essential == nil || aws.BoolValue(sidecars[name].Essential) { if status != "start" { return errEssentialContainerStatus } @@ -135,83 +135,106 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sid return nil } -func validateNoCircularDependencies(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, manifestName string) error { - // don't let anything circularly depend on each other - dependencies := buildDependencyGraph(sidecars, img, manifestName) +func validateNoCircularDependencies(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, workloadName string) error { + used := make(map[string]bool) + path := make(map[string]bool) + dependencies, err := buildDependencyGraph(sidecars, img, workloadName) - // don't let invalid containers happen - if dependencies == nil { - return errInvalidContainer + // don't let nonexistent containers pass + if err != nil { + return err } // don't let circular dependencies happen - if !graph.Acyclic(dependencies) { - return errCircularDependency + for node := range dependencies.nodes { + if !used[node] && hasCycles(dependencies, used, path, node) { + return errCircularDependency + } } return nil } -func buildDependencyGraph(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, manifestName string) *graph.Mutable { - var currNode, depNode int - last := 1 - nodes := map[string]int{} - dependencies := graph.New(len(sidecars) + 1) +func hasCycles(graph *Graph, used map[string]bool, path map[string]bool, currNode string) bool { + used[currNode] = true + path[currNode] = true + + for _, node := range graph.nodes[currNode] { + if !used[node] && hasCycles(graph, used, path, node) { + return true + } else if path[node] { + return true + } + } + + path[currNode] = false + return false +} + +func buildDependencyGraph(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, workloadName string) (*Graph, error) { + dependencyGraph := Graph{nodes: make(map[string][]string)} // sidecar dependencies for name, sidecar := range sidecars { // add any existing dependency to the graph if len(sidecar.DependsOn) != 0 { - currNode, nodes, last = getNode(name, nodes, last) - for dep := range sidecar.DependsOn { // containers being depended on must exist - if sidecars[dep] == nil && dep != manifestName { - return nil + if sidecars[dep] == nil && dep != workloadName { + return nil, errInvalidContainer } - depNode, nodes, last = getNode(dep, nodes, last) - dependencies.Add(currNode, depNode) + dependencyGraph.Add(name, dep) } } } // add any image dependencies to the graph if len(img.DependsOn) != 0 { - currNode, nodes, last = getNode(manifestName, nodes, last) - for dep := range img.DependsOn { // containers being depended on must exist - if sidecars[dep] == nil { - return nil + if sidecars[dep] == nil && dep != workloadName { + return nil, errInvalidContainer } - depNode, nodes, last = getNode(dep, nodes, last) - dependencies.Add(currNode, depNode) + dependencyGraph.Add(workloadName, dep) } } - return dependencies + return &dependencyGraph, nil } -func getNode(name string, nodes map[string]int, last int) (int, map[string]int, int) { - if nodes[name] != 0 { - return nodes[name] - 1, nodes, last +type Graph struct { + nodes map[string][]string +} + +func (graph *Graph) Add(fromNode, toNode string) { + hasNode := false + + // add origin node if doesn't exist + if graph.nodes[fromNode] == nil { + graph.nodes[fromNode] = []string{} + } + + // check if edge exists between from and to nodes + for _, node := range graph.nodes[fromNode] { + if node == toNode { + hasNode = true + } + } + + // add edge if not there already + if !hasNode { + graph.nodes[fromNode] = append(graph.nodes[fromNode], toNode) } - nodes[name] = last - return nodes[name] - 1, nodes, last + 1 } -func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.SidecarConfig, manifestName string) error { +func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.SidecarConfig, workloadName string) error { if img.DependsOn == nil { return nil } for name, status := range img.DependsOn { - // don't let image depend on itself - if name == manifestName { - return errCircularDependency - } // status must be one of < start | complete | success > if !isValidStatus(status) { return errInvalidDependsOnStatus @@ -226,7 +249,7 @@ func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.Si } } - return validateNoCircularDependencies(sidecars, img, manifestName) + return validateNoCircularDependencies(sidecars, img, workloadName) } func isValidStatus(s string) bool { diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 4b3fefb7277..aacb20980e6 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -155,7 +155,7 @@ func Test_validateEFSConfig(t *testing.T) { func TestValidateSidecarDependsOn(t *testing.T) { mockSidecarName := "sidecar" - mockManifestName := "frontend" + mockWorkloadName := "frontend" testCases := map[string]struct { inSidecar *manifest.SidecarConfig allSidecars map[string]*manifest.SidecarConfig @@ -246,7 +246,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateSidecarDependsOn(*tc.inSidecar, mockSidecarName, tc.allSidecars, mockManifestName) + gotErr := validateSidecarDependsOn(*tc.inSidecar, mockSidecarName, tc.allSidecars, mockWorkloadName) if tc.wantErr == nil { require.NoError(t, gotErr) } else { @@ -257,7 +257,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { } func TestValidateNoCircularDependency(t *testing.T) { - mockManifestName := "frontend" + mockWorkloadName := "frontend" image := manifest.Image{} testCases := map[string]struct { allSidecars map[string]*manifest.SidecarConfig @@ -280,6 +280,27 @@ func TestValidateNoCircularDependency(t *testing.T) { }, wantErr: nil, }, + "working sidecars with complex container dependencies": { + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": { + DependsOn: map[string]string{ + "secondCar": "start", + }, + }, + "secondCar": { + DependsOn: map[string]string{ + "thirdCar": "start", + }, + }, + "thirdCar": { + DependsOn: map[string]string{ + "fourthCar": "start", + }, + }, + "fourthCar": {}, + }, + wantErr: nil, + }, "error when sidecar depends on itself": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { @@ -343,7 +364,7 @@ func TestValidateNoCircularDependency(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateNoCircularDependencies(tc.allSidecars, image, mockManifestName) + gotErr := validateNoCircularDependencies(tc.allSidecars, image, mockWorkloadName) if tc.wantErr == nil { require.NoError(t, gotErr) } else { @@ -354,7 +375,7 @@ func TestValidateNoCircularDependency(t *testing.T) { } func TestValidateImageDependsOn(t *testing.T) { - mockManifestName := "frontend" + mockWorkloadName := "frontend" testCases := map[string]struct { inImage *manifest.Image inSidecars map[string]*manifest.SidecarConfig @@ -422,7 +443,7 @@ func TestValidateImageDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateImageDependsOn(*tc.inImage, tc.inSidecars, mockManifestName) + gotErr := validateImageDependsOn(*tc.inImage, tc.inSidecars, mockWorkloadName) if tc.wantErr == nil { require.NoError(t, gotErr) } else { From 2850882c7a168f20fb7d03dc7c8384c82bb38668 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Mon, 7 Jun 2021 17:01:31 -0400 Subject: [PATCH 06/13] adjusts functions for use of convertSidecarOpts adjusts functions to use convertSidecarOpts instead of individual parameters --- .../cloudformation/stack/transformers.go | 6 ++-- .../deploy/cloudformation/stack/validate.go | 34 +++++++++---------- .../cloudformation/stack/validate_test.go | 21 +++++++++--- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 26ddb46dae6..74b035335a3 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -55,7 +55,7 @@ func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { if s.sidecarConfig == nil { return nil, nil } - if err := validateNoCircularDependencies(s.sidecarConfig, *s.imageConfig, s.workloadName); err != nil { + if err := validateNoCircularDependencies(s); err != nil { return nil, err } var sidecars []*template.SidecarOpts @@ -67,7 +67,7 @@ func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { if err := validateSidecarMountPoints(config.MountPoints); err != nil { return nil, err } - if err := validateSidecarDependsOn(*config, name, s.sidecarConfig, s.workloadName); err != nil { + if err := validateSidecarDependsOn(*config, name, s); err != nil { return nil, err } mp := convertSidecarMountPoints(config.MountPoints) @@ -94,7 +94,7 @@ func convertImageDependsOn(s convertSidecarOpts) (map[string]string, error) { if s.imageConfig == nil || s.imageConfig.DependsOn == nil { return nil, nil } - if err := validateImageDependsOn(*s.imageConfig, s.sidecarConfig, s.workloadName); err != nil { + if err := validateImageDependsOn(s); err != nil { return nil, err } return s.imageConfig.DependsOn, nil diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 1dbb520389f..4a79822ea1f 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -114,7 +114,7 @@ func validateSidecarMountPoints(in []manifest.SidecarMountPoint) error { return nil } -func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sidecars map[string]*manifest.SidecarConfig, workloadName string) error { +func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s convertSidecarOpts) error { if in.DependsOn == nil { return nil } @@ -125,7 +125,7 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sid return errInvalidDependsOnStatus } // essential containers must have 'start' as a status - if name == workloadName || sidecars[name].Essential == nil || aws.BoolValue(sidecars[name].Essential) { + if name == s.workloadName || s.sidecarConfig[name].Essential == nil || aws.BoolValue(s.sidecarConfig[name].Essential) { if status != "start" { return errEssentialContainerStatus } @@ -135,10 +135,10 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, sid return nil } -func validateNoCircularDependencies(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, workloadName string) error { +func validateNoCircularDependencies(s convertSidecarOpts) error { used := make(map[string]bool) path := make(map[string]bool) - dependencies, err := buildDependencyGraph(sidecars, img, workloadName) + dependencies, err := buildDependencyGraph(s) // don't let nonexistent containers pass if err != nil { @@ -171,16 +171,16 @@ func hasCycles(graph *Graph, used map[string]bool, path map[string]bool, currNod return false } -func buildDependencyGraph(sidecars map[string]*manifest.SidecarConfig, img manifest.Image, workloadName string) (*Graph, error) { +func buildDependencyGraph(s convertSidecarOpts) (*Graph, error) { dependencyGraph := Graph{nodes: make(map[string][]string)} // sidecar dependencies - for name, sidecar := range sidecars { + for name, sidecar := range s.sidecarConfig { // add any existing dependency to the graph if len(sidecar.DependsOn) != 0 { for dep := range sidecar.DependsOn { // containers being depended on must exist - if sidecars[dep] == nil && dep != workloadName { + if s.sidecarConfig[dep] == nil && dep != s.workloadName { return nil, errInvalidContainer } @@ -190,14 +190,14 @@ func buildDependencyGraph(sidecars map[string]*manifest.SidecarConfig, img manif } // add any image dependencies to the graph - if len(img.DependsOn) != 0 { - for dep := range img.DependsOn { + if len(s.imageConfig.DependsOn) != 0 { + for dep := range s.imageConfig.DependsOn { // containers being depended on must exist - if sidecars[dep] == nil && dep != workloadName { + if s.sidecarConfig[dep] == nil && dep != s.workloadName { return nil, errInvalidContainer } - dependencyGraph.Add(workloadName, dep) + dependencyGraph.Add(s.workloadName, dep) } } @@ -229,19 +229,19 @@ func (graph *Graph) Add(fromNode, toNode string) { } } -func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.SidecarConfig, workloadName string) error { - if img.DependsOn == nil { +func validateImageDependsOn(s convertSidecarOpts) error { + if s.imageConfig.DependsOn == nil { return nil } - for name, status := range img.DependsOn { + for name, status := range s.imageConfig.DependsOn { // status must be one of < start | complete | success > if !isValidStatus(status) { return errInvalidDependsOnStatus } // essential containers must have 'start' as a status - if sidecars != nil { - if sidecars[name].Essential == nil || *sidecars[name].Essential { + if s.sidecarConfig != nil { + if s.sidecarConfig[name].Essential == nil || *s.sidecarConfig[name].Essential { if status != "start" { return errEssentialContainerStatus } @@ -249,7 +249,7 @@ func validateImageDependsOn(img manifest.Image, sidecars map[string]*manifest.Si } } - return validateNoCircularDependencies(sidecars, img, workloadName) + return validateNoCircularDependencies(s) } func isValidStatus(s string) bool { diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index aacb20980e6..172630ac553 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -246,7 +246,12 @@ func TestValidateSidecarDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateSidecarDependsOn(*tc.inSidecar, mockSidecarName, tc.allSidecars, mockWorkloadName) + s := convertSidecarOpts{ + sidecarConfig: tc.allSidecars, + imageConfig: &manifest.Image{}, + workloadName: mockWorkloadName, + } + gotErr := validateSidecarDependsOn(*tc.inSidecar, mockSidecarName, s) if tc.wantErr == nil { require.NoError(t, gotErr) } else { @@ -256,7 +261,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { } } -func TestValidateNoCircularDependency(t *testing.T) { +func TestValidateNoCircularDependencies(t *testing.T) { mockWorkloadName := "frontend" image := manifest.Image{} testCases := map[string]struct { @@ -364,7 +369,11 @@ func TestValidateNoCircularDependency(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateNoCircularDependencies(tc.allSidecars, image, mockWorkloadName) + gotErr := validateNoCircularDependencies(convertSidecarOpts{ + sidecarConfig: tc.allSidecars, + imageConfig: &image, + workloadName: mockWorkloadName, + }) if tc.wantErr == nil { require.NoError(t, gotErr) } else { @@ -443,7 +452,11 @@ func TestValidateImageDependsOn(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { - gotErr := validateImageDependsOn(*tc.inImage, tc.inSidecars, mockWorkloadName) + gotErr := validateImageDependsOn(convertSidecarOpts{ + sidecarConfig: tc.inSidecars, + imageConfig: tc.inImage, + workloadName: mockWorkloadName, + }) if tc.wantErr == nil { require.NoError(t, gotErr) } else { From 0bb561d404e6718e66eeeb76cea6738754ec42b5 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Tue, 8 Jun 2021 15:33:32 -0400 Subject: [PATCH 07/13] adjusta errors, modularity, statuses adjusts errors to state dependencies for circular dependencies, adds error for dependsny on itself adjusts comments adjusts status to allow healthy and adds function to ensure uppercase better modularizes cyclic dependency detection better modularizes essential status Please enter the commit message for your changes. Lines starting --- .../cloudformation/stack/backend_svc.go | 2 +- .../deploy/cloudformation/stack/lb_web_svc.go | 2 +- .../cloudformation/stack/scheduled_job.go | 2 +- .../cloudformation/stack/transformers.go | 33 ++++- .../cloudformation/stack/transformers_test.go | 48 ++++-- .../deploy/cloudformation/stack/validate.go | 138 ++++++++++-------- .../cloudformation/stack/validate_test.go | 66 +++++---- 7 files changed, 180 insertions(+), 111 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/backend_svc.go b/internal/pkg/deploy/cloudformation/stack/backend_svc.go index 57cc6ca26c9..895b1f4ba55 100644 --- a/internal/pkg/deploy/cloudformation/stack/backend_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/backend_svc.go @@ -84,7 +84,7 @@ func (s *BackendService) Template() (string, error) { convSidecarOpts := convertSidecarOpts{ sidecarConfig: s.manifest.Sidecars, imageConfig: &s.manifest.ImageConfig.Image, - workloadName: *s.manifest.Name, + workloadName: aws.StringValue(s.manifest.Name), } sidecars, err := convertSidecar(convSidecarOpts) if err != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go index 7e50854f3ea..32effa7756d 100644 --- a/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go +++ b/internal/pkg/deploy/cloudformation/stack/lb_web_svc.go @@ -109,7 +109,7 @@ func (s *LoadBalancedWebService) Template() (string, error) { convSidecarOpts := convertSidecarOpts{ sidecarConfig: s.manifest.Sidecars, imageConfig: &s.manifest.ImageConfig.Image, - workloadName: *s.manifest.Name, + workloadName: aws.StringValue(s.manifest.Name), } sidecars, err := convertSidecar(convSidecarOpts) if err != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go index ef37dca1ced..7a5d1ac8f67 100644 --- a/internal/pkg/deploy/cloudformation/stack/scheduled_job.go +++ b/internal/pkg/deploy/cloudformation/stack/scheduled_job.go @@ -127,7 +127,7 @@ func (j *ScheduledJob) Template() (string, error) { convSidecarOpts := convertSidecarOpts{ sidecarConfig: j.manifest.Sidecars, imageConfig: &j.manifest.ImageConfig, - workloadName: *j.manifest.Name, + workloadName: aws.StringValue(j.manifest.Name), } sidecars, err := convertSidecar(convSidecarOpts) if err != nil { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers.go b/internal/pkg/deploy/cloudformation/stack/transformers.go index 74b035335a3..95de8cc8dbf 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers.go @@ -50,6 +50,12 @@ var ( errInvalidSpotConfig = errors.New(`"count.spot" and "count.range" cannot be specified together`) ) +type convertSidecarOpts struct { + sidecarConfig map[string]*manifest.SidecarConfig + imageConfig *manifest.Image + workloadName string +} + // convertSidecar converts the manifest sidecar configuration into a format parsable by the templates pkg. func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { if s.sidecarConfig == nil { @@ -67,6 +73,7 @@ func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { if err := validateSidecarMountPoints(config.MountPoints); err != nil { return nil, err } + convertDependsOnStatus(&s) if err := validateSidecarDependsOn(*config, name, s); err != nil { return nil, err } @@ -89,23 +96,37 @@ func convertSidecar(s convertSidecarOpts) ([]*template.SidecarOpts, error) { return sidecars, nil } +// convertDependsOnStatus converts image and sidecar depends on fields to have upper case statuses +func convertDependsOnStatus(s *convertSidecarOpts) { + if s.sidecarConfig != nil { + for _, sidecar := range s.sidecarConfig { + if sidecar.DependsOn == nil { + continue + } + for name, status := range sidecar.DependsOn { + sidecar.DependsOn[name] = strings.ToUpper(status) + } + } + } + if s.imageConfig != nil && s.imageConfig.DependsOn != nil { + for name, status := range s.imageConfig.DependsOn { + s.imageConfig.DependsOn[name] = strings.ToUpper(status) + } + } +} + // convertDependsOn converts an Image DependsOn field to a template DependsOn version func convertImageDependsOn(s convertSidecarOpts) (map[string]string, error) { if s.imageConfig == nil || s.imageConfig.DependsOn == nil { return nil, nil } + convertDependsOnStatus(&s) if err := validateImageDependsOn(s); err != nil { return nil, err } return s.imageConfig.DependsOn, nil } -type convertSidecarOpts struct { - sidecarConfig map[string]*manifest.SidecarConfig - imageConfig *manifest.Image - workloadName string -} - // Valid sidecar portMapping example: 2000/udp, or 2000 (default to be tcp). func parsePortMapping(s *string) (port *string, protocol *string, err error) { if s == nil { diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index ad8f9d63802..fba40264d6a 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -19,12 +19,14 @@ func Test_convertSidecar(t *testing.T) { mockWorkloadName := "frontend" mockMap := map[string]string{"foo": "bar"} mockCredsParam := aws.String("mockCredsParam") + circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") testCases := map[string]struct { - inPort string - inEssential bool - inLabels map[string]string - inDependsOn map[string]string - inImg manifest.Image + inPort string + inEssential bool + inLabels map[string]string + inDependsOn map[string]string + inImg manifest.Image + circDepContainers []string wanted *template.SidecarOpts wantedErr error @@ -70,7 +72,7 @@ func Test_convertSidecar(t *testing.T) { "foo": "start", }, - wantedErr: errCircularDependency, + wantedErr: fmt.Errorf("circular container dependency chain present: container foo depends on itself"), }, "invalid container dependency due to circularly depending on another container": { inPort: "2000", @@ -83,7 +85,8 @@ func Test_convertSidecar(t *testing.T) { "foo": "start", }, }, - wantedErr: errCircularDependency, + wantedErr: circularDependencyErr, + circDepContainers: []string{"frontend", "foo"}, }, "invalid container dependency status": { inPort: "2000", @@ -117,7 +120,7 @@ func Test_convertSidecar(t *testing.T) { Variables: mockMap, Essential: aws.Bool(true), DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, }, @@ -137,7 +140,7 @@ func Test_convertSidecar(t *testing.T) { Variables: mockMap, Essential: aws.Bool(false), DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, }, @@ -182,7 +185,12 @@ func Test_convertSidecar(t *testing.T) { workloadName: mockWorkloadName, }) - if tc.wantedErr != nil { + if tc.wantedErr == circularDependencyErr { + require.Contains(t, err.Error(), circularDependencyErr.Error()) + for _, container := range tc.circDepContainers { + require.Contains(t, err.Error(), container) + } + } else if tc.wantedErr != nil { require.EqualError(t, err, tc.wantedErr.Error()) } else { require.NoError(t, err) @@ -1244,9 +1252,11 @@ func Test_convertEphemeral(t *testing.T) { func Test_convertImageDependsOn(t *testing.T) { mockWorkloadName := "frontend" + circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") testCases := map[string]struct { - inImage *manifest.Image - inSidecars map[string]*manifest.SidecarConfig + inImage *manifest.Image + inSidecars map[string]*manifest.SidecarConfig + circDepContainers []string wanted map[string]string wantedError error @@ -1261,7 +1271,7 @@ func Test_convertImageDependsOn(t *testing.T) { "frontend": "start", }, }, - wantedError: errCircularDependency, + wantedError: fmt.Errorf("circular container dependency chain present: container frontend depends on itself"), }, "invalid container dependency due to circular dependency on a sidecar": { inImage: &manifest.Image{ @@ -1281,7 +1291,8 @@ func Test_convertImageDependsOn(t *testing.T) { }, }, }, - wantedError: errCircularDependency, + wantedError: circularDependencyErr, + circDepContainers: []string{"frontend", "sidecar", "sidecar2"}, }, "invalid container dependency due to status": { inImage: &manifest.Image{ @@ -1330,7 +1341,7 @@ func Test_convertImageDependsOn(t *testing.T) { "sidecar": {}, }, wanted: map[string]string{ - "sidecar": "start", + "sidecar": "START", }, }, } @@ -1341,7 +1352,12 @@ func Test_convertImageDependsOn(t *testing.T) { imageConfig: tc.inImage, workloadName: mockWorkloadName, }) - if tc.wantedError != nil { + if tc.wantedError == circularDependencyErr { + require.Contains(t, err.Error(), circularDependencyErr.Error()) + for _, container := range tc.circDepContainers { + require.Contains(t, err.Error(), container) + } + } else if tc.wantedError != nil { require.EqualError(t, err, tc.wantedError.Error()) } else { require.Equal(t, got, tc.wanted) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 4a79822ea1f..1fb9d8a8e33 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -11,11 +11,12 @@ import ( "github.com/aws/copilot-cli/internal/pkg/manifest" ) -// container dependency status constants. +// Container dependency status constants. const ( - dependsOnStart = "start" - dependsOnComplete = "complete" - dependsOnSuccess = "success" + dependsOnStart = "START" + dependsOnComplete = "COMPLETE" + dependsOnSuccess = "SUCCESS" + dependsOnHealthy = "HEALTHY" ) // Empty field errors. @@ -34,15 +35,15 @@ var ( errInvalidUIDGIDConfig = errors.New("must specify both UID and GID, or neither") errInvalidEFSConfig = errors.New("bad EFS configuration: cannot specify both bool and config") errReservedUID = errors.New("UID must not be 0") - errCircularDependency = errors.New("bad dependency name: circular dependency present") errInvalidContainer = errors.New("container dependency does not exist") - errInvalidDependsOnStatus = errors.New("container dependency status must be one of < start | complete | success >") + errInvalidDependsOnStatus = fmt.Errorf("container dependency status must be one of < %s | %s | %s | %s >", dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy) errEssentialContainerStatus = errors.New("essential container dependencies can only have status 'start'") ) +// Container dependency status options var ( - // Container dependency status options - DependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} + essentialContainerStatus = []string{dependsOnStart, dependsOnHealthy} + dependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -120,49 +121,88 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c } for name, status := range in.DependsOn { - // status must be one of < start | complete | success > if !isValidStatus(status) { return errInvalidDependsOnStatus } - // essential containers must have 'start' as a status - if name == s.workloadName || s.sidecarConfig[name].Essential == nil || aws.BoolValue(s.sidecarConfig[name].Essential) { - if status != "start" { - return errEssentialContainerStatus - } + if isEssential(name, s) && !isEssentialStatus(status) { + return errEssentialContainerStatus } } return nil } +func isValidStatus(s string) bool { + for _, allowed := range dependsOnStatus { + if s == allowed { + return true + } + } + + return false +} + +func isEssential(name string, s convertSidecarOpts) bool { + if s.sidecarConfig == nil { + return false + } + if name == s.workloadName || s.sidecarConfig[name].Essential == nil || aws.BoolValue(s.sidecarConfig[name].Essential) { + return true + } + + return false +} + +func isEssentialStatus(s string) bool { + for _, allowed := range essentialContainerStatus { + if s == allowed { + return true + } + } + + return false +} + func validateNoCircularDependencies(s convertSidecarOpts) error { - used := make(map[string]bool) - path := make(map[string]bool) dependencies, err := buildDependencyGraph(s) - // don't let nonexistent containers pass if err != nil { return err } - // don't let circular dependencies happen - for node := range dependencies.nodes { - if !used[node] && hasCycles(dependencies, used, path, node) { - return errCircularDependency + if !dependencies.isAcyclic() { + if len(dependencies.cycle) == 1 { + return fmt.Errorf("circular container dependency chain present: container %s depends on itself", dependencies.cycle[0]) } + return fmt.Errorf("circular container dependency chain present: chain includes the following containers: %s", dependencies.cycle) } return nil } -func hasCycles(graph *Graph, used map[string]bool, path map[string]bool, currNode string) bool { +func (g *graph) isAcyclic() bool { + used := make(map[string]bool) + path := make(map[string]bool) + + for node := range g.nodes { + if !used[node] && g.hasCycles(used, path, node) { + return false + } + } + + return true +} + +func (g *graph) hasCycles(used map[string]bool, path map[string]bool, currNode string) bool { used[currNode] = true path[currNode] = true - for _, node := range graph.nodes[currNode] { - if !used[node] && hasCycles(graph, used, path, node) { + for _, node := range g.nodes[currNode] { + if !used[node] && g.hasCycles(used, path, node) { + g.cycle = append(g.cycle, node) return true } else if path[node] { + g.cycle = append(g.cycle, node) return true } } @@ -171,15 +211,13 @@ func hasCycles(graph *Graph, used map[string]bool, path map[string]bool, currNod return false } -func buildDependencyGraph(s convertSidecarOpts) (*Graph, error) { - dependencyGraph := Graph{nodes: make(map[string][]string)} +func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { + dependencyGraph := graph{nodes: make(map[string][]string), cycle: make([]string, 0)} - // sidecar dependencies + // Add any idecar dependencies. for name, sidecar := range s.sidecarConfig { - // add any existing dependency to the graph if len(sidecar.DependsOn) != 0 { for dep := range sidecar.DependsOn { - // containers being depended on must exist if s.sidecarConfig[dep] == nil && dep != s.workloadName { return nil, errInvalidContainer } @@ -189,10 +227,9 @@ func buildDependencyGraph(s convertSidecarOpts) (*Graph, error) { } } - // add any image dependencies to the graph + // Add any image dependencies. if len(s.imageConfig.DependsOn) != 0 { for dep := range s.imageConfig.DependsOn { - // containers being depended on must exist if s.sidecarConfig[dep] == nil && dep != s.workloadName { return nil, errInvalidContainer } @@ -204,28 +241,29 @@ func buildDependencyGraph(s convertSidecarOpts) (*Graph, error) { return &dependencyGraph, nil } -type Graph struct { +type graph struct { nodes map[string][]string + cycle []string } -func (graph *Graph) Add(fromNode, toNode string) { +func (g *graph) Add(fromNode, toNode string) { hasNode := false - // add origin node if doesn't exist - if graph.nodes[fromNode] == nil { - graph.nodes[fromNode] = []string{} + // Add origin node if doesn't exist. + if g.nodes[fromNode] == nil { + g.nodes[fromNode] = []string{} } - // check if edge exists between from and to nodes - for _, node := range graph.nodes[fromNode] { + // Check if edge exists between from and to nodes. + for _, node := range g.nodes[fromNode] { if node == toNode { hasNode = true } } - // add edge if not there already + // Add edge if not there already. if !hasNode { - graph.nodes[fromNode] = append(graph.nodes[fromNode], toNode) + g.nodes[fromNode] = append(g.nodes[fromNode], toNode) } } @@ -235,33 +273,17 @@ func validateImageDependsOn(s convertSidecarOpts) error { } for name, status := range s.imageConfig.DependsOn { - // status must be one of < start | complete | success > if !isValidStatus(status) { return errInvalidDependsOnStatus } - // essential containers must have 'start' as a status - if s.sidecarConfig != nil { - if s.sidecarConfig[name].Essential == nil || *s.sidecarConfig[name].Essential { - if status != "start" { - return errEssentialContainerStatus - } - } + if isEssential(name, s) && !isEssentialStatus(status) { + return errEssentialContainerStatus } } return validateNoCircularDependencies(s) } -func isValidStatus(s string) bool { - for _, allowed := range DependsOnStatus { - if s == allowed { - return true - } - } - - return false -} - func validateEFSConfig(in manifest.Volume) error { // EFS is implicitly disabled. We don't use the attached EmptyVolume function here // because it may hide invalid config. diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 172630ac553..ab8e2a27026 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -4,6 +4,7 @@ package stack import ( + "fmt" "testing" "github.com/aws/aws-sdk-go/aws" @@ -169,7 +170,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "working set essential sidecar with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "sidecar1": "start", + "sidecar1": "START", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -182,7 +183,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "working implied essential container with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -193,7 +194,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "working non-essential sidecar with container dependency": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "sidecar2": "complete", + "sidecar2": "COMPLETE", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -207,7 +208,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "error when sidecar container dependency status is invalid": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "sidecar2": "end", + "sidecar2": "END", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -221,7 +222,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "error when set essential sidecar has a status besides start": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "sidecar2": "complete", + "sidecar2": "COMPLETE", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -235,7 +236,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "error when implied essential sidecar has a status besides start": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ - "frontend": "complete", + "frontend": "COMPLETE", }, }, allSidecars: map[string]*manifest.SidecarConfig{ @@ -264,8 +265,10 @@ func TestValidateSidecarDependsOn(t *testing.T) { func TestValidateNoCircularDependencies(t *testing.T) { mockWorkloadName := "frontend" image := manifest.Image{} + circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") testCases := map[string]struct { - allSidecars map[string]*manifest.SidecarConfig + allSidecars map[string]*manifest.SidecarConfig + circDepContainers []string wantErr error }{ @@ -279,7 +282,7 @@ func TestValidateNoCircularDependencies(t *testing.T) { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, }, @@ -289,17 +292,17 @@ func TestValidateNoCircularDependencies(t *testing.T) { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "secondCar": "start", + "secondCar": "START", }, }, "secondCar": { DependsOn: map[string]string{ - "thirdCar": "start", + "thirdCar": "START", }, }, "thirdCar": { DependsOn: map[string]string{ - "fourthCar": "start", + "fourthCar": "START", }, }, "fourthCar": {}, @@ -310,57 +313,59 @@ func TestValidateNoCircularDependencies(t *testing.T) { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "sidecar": "start", + "sidecar": "START", }, }, }, - wantErr: errCircularDependency, + wantErr: fmt.Errorf("circular container dependency chain present: container sidecar depends on itself"), }, "error when sidecars circularly depend on each other": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, "frontend": { DependsOn: map[string]string{ - "sidecar": "start", + "sidecar": "START", }, }, }, - wantErr: errCircularDependency, + wantErr: circularDependencyErr, + circDepContainers: []string{"sidecar", "frontend"}, }, "error when sidecars inadvertently depend on each other": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "secondCar": "start", + "secondCar": "START", }, }, "secondCar": { DependsOn: map[string]string{ - "thirdCar": "start", + "thirdCar": "START", }, }, "thirdCar": { DependsOn: map[string]string{ - "fourthCar": "start", + "fourthCar": "START", }, }, "fourthCar": { DependsOn: map[string]string{ - "sidecar": "start", + "sidecar": "START", }, }, }, - wantErr: errCircularDependency, + wantErr: circularDependencyErr, + circDepContainers: []string{"sidecar", "secondCar", "thirdCar", "fourthCar"}, }, "error when container doesn't exist": { allSidecars: map[string]*manifest.SidecarConfig{ "sidecar": { DependsOn: map[string]string{ - "something": "start", + "something": "START", }, }, }, @@ -376,6 +381,11 @@ func TestValidateNoCircularDependencies(t *testing.T) { }) if tc.wantErr == nil { require.NoError(t, gotErr) + } else if tc.wantErr == circularDependencyErr { + require.Contains(t, gotErr.Error(), circularDependencyErr.Error()) + for _, container := range tc.circDepContainers { + require.Contains(t, gotErr.Error(), container) + } } else { require.EqualError(t, gotErr, tc.wantErr.Error()) } @@ -398,7 +408,7 @@ func TestValidateImageDependsOn(t *testing.T) { "working image with container dependency": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "sidecar": "start", + "sidecar": "START", }, }, inSidecars: map[string]*manifest.SidecarConfig{ @@ -409,15 +419,15 @@ func TestValidateImageDependsOn(t *testing.T) { "error when image depends on itself": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "frontend": "start", + "frontend": "START", }, }, - wantErr: errCircularDependency, + wantErr: fmt.Errorf("circular container dependency chain present: container frontend depends on itself"), }, "error when image container dependency status is invalid": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "sidecar": "end", + "sidecar": "END", }, }, inSidecars: map[string]*manifest.SidecarConfig{ @@ -428,7 +438,7 @@ func TestValidateImageDependsOn(t *testing.T) { "error when set essential container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "sidecar": "complete", + "sidecar": "COMPLETE", }, }, inSidecars: map[string]*manifest.SidecarConfig{ @@ -441,7 +451,7 @@ func TestValidateImageDependsOn(t *testing.T) { "error when implied essential container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ - "sidecar": "complete", + "sidecar": "COMPLETE", }, }, inSidecars: map[string]*manifest.SidecarConfig{ From b1245b6d9316d2921d5f6553bd7c312e9387b3e4 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Tue, 8 Jun 2021 16:56:38 -0400 Subject: [PATCH 08/13] adjusts essential container status error --- internal/pkg/deploy/cloudformation/stack/validate.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 1fb9d8a8e33..84263957fc5 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -37,7 +37,7 @@ var ( errReservedUID = errors.New("UID must not be 0") errInvalidContainer = errors.New("container dependency does not exist") errInvalidDependsOnStatus = fmt.Errorf("container dependency status must be one of < %s | %s | %s | %s >", dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy) - errEssentialContainerStatus = errors.New("essential container dependencies can only have status 'start'") + errEssentialContainerStatus = fmt.Errorf("essential container dependencies can only have status < %s | %s >", dependsOnStart, dependsOnHealthy) ) // Container dependency status options From 6966c948548a9667bba821fa249c2b220928782d Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Wed, 9 Jun 2021 15:53:15 -0400 Subject: [PATCH 09/13] removes uncessary check for dependsOn size Lines starting --- .../deploy/cloudformation/stack/validate.go | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 84263957fc5..181c4b2f223 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -214,28 +214,24 @@ func (g *graph) hasCycles(used map[string]bool, path map[string]bool, currNode s func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { dependencyGraph := graph{nodes: make(map[string][]string), cycle: make([]string, 0)} - // Add any idecar dependencies. + // Add any sidecar dependencies. for name, sidecar := range s.sidecarConfig { - if len(sidecar.DependsOn) != 0 { - for dep := range sidecar.DependsOn { - if s.sidecarConfig[dep] == nil && dep != s.workloadName { - return nil, errInvalidContainer - } - - dependencyGraph.Add(name, dep) + for dep := range sidecar.DependsOn { + if s.sidecarConfig[dep] == nil && dep != s.workloadName { + return nil, errInvalidContainer } + + dependencyGraph.Add(name, dep) } } // Add any image dependencies. - if len(s.imageConfig.DependsOn) != 0 { - for dep := range s.imageConfig.DependsOn { - if s.sidecarConfig[dep] == nil && dep != s.workloadName { - return nil, errInvalidContainer - } - - dependencyGraph.Add(s.workloadName, dep) + for dep := range s.imageConfig.DependsOn { + if s.sidecarConfig[dep] == nil && dep != s.workloadName { + return nil, errInvalidContainer } + + dependencyGraph.Add(s.workloadName, dep) } return &dependencyGraph, nil From a15557d1dd67bdcacd81eabdf97f84bc05c3d206 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 10 Jun 2021 11:26:19 -0400 Subject: [PATCH 10/13] adjusts status check, errors adjusts essential status check and valid status check to consider that sidecars can never be "HEALTHY" adds errors for the above for sidecars adjusts errors to be less deeply nested for circular dependencies makes graph function private refactors acyclic to return early --- .../cloudformation/stack/transformers_test.go | 14 +-- .../deploy/cloudformation/stack/validate.go | 91 +++++++++++-------- .../cloudformation/stack/validate_test.go | 41 +++++++-- 3 files changed, 92 insertions(+), 54 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index fba40264d6a..bedb18a9254 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -19,7 +19,7 @@ func Test_convertSidecar(t *testing.T) { mockWorkloadName := "frontend" mockMap := map[string]string{"foo": "bar"} mockCredsParam := aws.String("mockCredsParam") - circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") + circularDependencyErr := fmt.Errorf("circular container dependency chain includes the following containers: ") testCases := map[string]struct { inPort string inEssential bool @@ -72,7 +72,7 @@ func Test_convertSidecar(t *testing.T) { "foo": "start", }, - wantedErr: fmt.Errorf("circular container dependency chain present: container foo depends on itself"), + wantedErr: fmt.Errorf("container foo cannot depend on itself"), }, "invalid container dependency due to circularly depending on another container": { inPort: "2000", @@ -1252,7 +1252,7 @@ func Test_convertEphemeral(t *testing.T) { func Test_convertImageDependsOn(t *testing.T) { mockWorkloadName := "frontend" - circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") + circularDependencyErr := fmt.Errorf("circular container dependency chain includes the following containers: ") testCases := map[string]struct { inImage *manifest.Image inSidecars map[string]*manifest.SidecarConfig @@ -1271,7 +1271,7 @@ func Test_convertImageDependsOn(t *testing.T) { "frontend": "start", }, }, - wantedError: fmt.Errorf("circular container dependency chain present: container frontend depends on itself"), + wantedError: fmt.Errorf("container frontend cannot depend on itself"), }, "invalid container dependency due to circular dependency on a sidecar": { inImage: &manifest.Image{ @@ -1305,7 +1305,7 @@ func Test_convertImageDependsOn(t *testing.T) { Essential: aws.Bool(false), }, }, - wantedError: errInvalidDependsOnStatus, + wantedError: errInvalidSidecarDependsOnStatus, }, "invalid implied essential container depdendency": { inImage: &manifest.Image{ @@ -1316,7 +1316,7 @@ func Test_convertImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantedError: errEssentialContainerStatus, + wantedError: errEssentialSidecarStatus, }, "invalid set essential container depdendency": { inImage: &manifest.Image{ @@ -1329,7 +1329,7 @@ func Test_convertImageDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantedError: errEssentialContainerStatus, + wantedError: errEssentialSidecarStatus, }, "good essential container dependency": { inImage: &manifest.Image{ diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 181c4b2f223..f532562b6cb 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -29,21 +29,24 @@ var ( // Conditional errors. var ( - errAccessPointWithRootDirectory = errors.New("`root_directory` must be empty or \"/\" when `access_point` is specified") - errAccessPointWithoutIAM = errors.New("`iam` must be true when `access_point` is specified") - errUIDWithNonManagedFS = errors.New("UID and GID cannot be specified with non-managed EFS") - errInvalidUIDGIDConfig = errors.New("must specify both UID and GID, or neither") - errInvalidEFSConfig = errors.New("bad EFS configuration: cannot specify both bool and config") - errReservedUID = errors.New("UID must not be 0") - errInvalidContainer = errors.New("container dependency does not exist") - errInvalidDependsOnStatus = fmt.Errorf("container dependency status must be one of < %s | %s | %s | %s >", dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy) - errEssentialContainerStatus = fmt.Errorf("essential container dependencies can only have status < %s | %s >", dependsOnStart, dependsOnHealthy) + errAccessPointWithRootDirectory = errors.New("`root_directory` must be empty or \"/\" when `access_point` is specified") + errAccessPointWithoutIAM = errors.New("`iam` must be true when `access_point` is specified") + errUIDWithNonManagedFS = errors.New("UID and GID cannot be specified with non-managed EFS") + errInvalidUIDGIDConfig = errors.New("must specify both UID and GID, or neither") + errInvalidEFSConfig = errors.New("bad EFS configuration: cannot specify both bool and config") + errReservedUID = errors.New("UID must not be 0") + errInvalidContainer = errors.New("container dependency does not exist") + errInvalidDependsOnStatus = fmt.Errorf("container dependency status must be one of < %s | %s | %s | %s >", dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy) + errInvalidSidecarDependsOnStatus = fmt.Errorf("sidecar container dependency status must be one of < %s | %s | %s >", dependsOnStart, dependsOnComplete, dependsOnSuccess) + errEssentialContainerStatus = fmt.Errorf("essential container dependencies can only have status < %s | %s >", dependsOnStart, dependsOnHealthy) + errEssentialSidecarStatus = fmt.Errorf("essential sidecar container dependencies can only have status < %s >", dependsOnStart) ) // Container dependency status options var ( essentialContainerStatus = []string{dependsOnStart, dependsOnHealthy} dependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} + sidecarDependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -121,25 +124,33 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c } for name, status := range in.DependsOn { - if !isValidStatus(status) { - return errInvalidDependsOnStatus + if ok, err := isValidStatus(status, name, s); !ok { + return err } - if isEssential(name, s) && !isEssentialStatus(status) { - return errEssentialContainerStatus + if ok, err := isEssentialStatus(status, name, s); isEssential(name, s) && !ok { + return err } } return nil } -func isValidStatus(s string) bool { - for _, allowed := range dependsOnStatus { - if s == allowed { - return true +func isValidStatus(status string, container string, c convertSidecarOpts) (bool, error) { + if c.sidecarConfig[container] != nil { + for _, allowed := range sidecarDependsOnStatus { + if status == allowed { + return true, nil + } } + return false, errInvalidSidecarDependsOnStatus } - return false + for _, allowed := range dependsOnStatus { + if status == allowed { + return true, nil + } + } + return false, errInvalidDependsOnStatus } func isEssential(name string, s convertSidecarOpts) bool { @@ -153,14 +164,20 @@ func isEssential(name string, s convertSidecarOpts) bool { return false } -func isEssentialStatus(s string) bool { - for _, allowed := range essentialContainerStatus { - if s == allowed { - return true +func isEssentialStatus(status string, container string, c convertSidecarOpts) (bool, error) { + if c.sidecarConfig[container] != nil { + if status == dependsOnStart { + return true, nil } + return false, errEssentialSidecarStatus } - return false + for _, allowed := range essentialContainerStatus { + if status == allowed { + return true, nil + } + } + return false, errEssentialContainerStatus } func validateNoCircularDependencies(s convertSidecarOpts) error { @@ -169,15 +186,13 @@ func validateNoCircularDependencies(s convertSidecarOpts) error { if err != nil { return err } - - if !dependencies.isAcyclic() { - if len(dependencies.cycle) == 1 { - return fmt.Errorf("circular container dependency chain present: container %s depends on itself", dependencies.cycle[0]) - } - return fmt.Errorf("circular container dependency chain present: chain includes the following containers: %s", dependencies.cycle) + if dependencies.isAcyclic() { + return nil } - - return nil + if len(dependencies.cycle) == 1 { + return fmt.Errorf("container %s cannot depend on itself", dependencies.cycle[0]) + } + return fmt.Errorf("circular container dependency chain includes the following containers: %s", dependencies.cycle) } func (g *graph) isAcyclic() bool { @@ -221,7 +236,7 @@ func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { return nil, errInvalidContainer } - dependencyGraph.Add(name, dep) + dependencyGraph.add(name, dep) } } @@ -231,7 +246,7 @@ func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { return nil, errInvalidContainer } - dependencyGraph.Add(s.workloadName, dep) + dependencyGraph.add(s.workloadName, dep) } return &dependencyGraph, nil @@ -242,7 +257,7 @@ type graph struct { cycle []string } -func (g *graph) Add(fromNode, toNode string) { +func (g *graph) add(fromNode, toNode string) { hasNode := false // Add origin node if doesn't exist. @@ -269,11 +284,11 @@ func validateImageDependsOn(s convertSidecarOpts) error { } for name, status := range s.imageConfig.DependsOn { - if !isValidStatus(status) { - return errInvalidDependsOnStatus + if ok, err := isValidStatus(status, name, s); !ok { + return err } - if isEssential(name, s) && !isEssentialStatus(status) { - return errEssentialContainerStatus + if ok, err := isEssentialStatus(status, name, s); isEssential(name, s) && !ok { + return err } } diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index ab8e2a27026..d01e0502da5 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -217,6 +217,17 @@ func TestValidateSidecarDependsOn(t *testing.T) { Essential: aws.Bool(false), }, }, + wantErr: errInvalidSidecarDependsOnStatus, + }, + "error when container dependency status is invalid": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "frontend": "END", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + }, wantErr: errInvalidDependsOnStatus, }, "error when set essential sidecar has a status besides start": { @@ -231,9 +242,21 @@ func TestValidateSidecarDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantErr: errEssentialContainerStatus, + wantErr: errEssentialSidecarStatus, }, "error when implied essential sidecar has a status besides start": { + inSidecar: &manifest.SidecarConfig{ + DependsOn: map[string]string{ + "sidecar2": "COMPLETE", + }, + }, + allSidecars: map[string]*manifest.SidecarConfig{ + "sidecar": {}, + "sidecar2": {}, + }, + wantErr: errEssentialSidecarStatus, + }, + "error when essential container dependency status is invalid": { inSidecar: &manifest.SidecarConfig{ DependsOn: map[string]string{ "frontend": "COMPLETE", @@ -265,7 +288,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { func TestValidateNoCircularDependencies(t *testing.T) { mockWorkloadName := "frontend" image := manifest.Image{} - circularDependencyErr := fmt.Errorf("circular container dependency chain present: chain includes the following containers: ") + circularDependencyErr := fmt.Errorf("circular container dependency chain includes the following containers: ") testCases := map[string]struct { allSidecars map[string]*manifest.SidecarConfig circDepContainers []string @@ -317,7 +340,7 @@ func TestValidateNoCircularDependencies(t *testing.T) { }, }, }, - wantErr: fmt.Errorf("circular container dependency chain present: container sidecar depends on itself"), + wantErr: fmt.Errorf("container sidecar cannot depend on itself"), }, "error when sidecars circularly depend on each other": { allSidecars: map[string]*manifest.SidecarConfig{ @@ -422,7 +445,7 @@ func TestValidateImageDependsOn(t *testing.T) { "frontend": "START", }, }, - wantErr: fmt.Errorf("circular container dependency chain present: container frontend depends on itself"), + wantErr: fmt.Errorf("container frontend cannot depend on itself"), }, "error when image container dependency status is invalid": { inImage: &manifest.Image{ @@ -433,9 +456,9 @@ func TestValidateImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantErr: errInvalidDependsOnStatus, + wantErr: errInvalidSidecarDependsOnStatus, }, - "error when set essential container has a status besides start": { + "error when set essential sidecar container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "COMPLETE", @@ -446,9 +469,9 @@ func TestValidateImageDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantErr: errEssentialContainerStatus, + wantErr: errEssentialSidecarStatus, }, - "error when implied essential container has a status besides start": { + "error when implied essential sidecar container has a status besides start": { inImage: &manifest.Image{ DependsOn: map[string]string{ "sidecar": "COMPLETE", @@ -457,7 +480,7 @@ func TestValidateImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantErr: errEssentialContainerStatus, + wantErr: errEssentialSidecarStatus, }, } for name, tc := range testCases { From 07487db13c9769efdaf0e0d39c9b36a53912d17f Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 10 Jun 2021 11:53:09 -0400 Subject: [PATCH 11/13] adjusts nil format to comma ok format --- internal/pkg/deploy/cloudformation/stack/validate.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index f532562b6cb..42da4cca793 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -136,7 +136,7 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c } func isValidStatus(status string, container string, c convertSidecarOpts) (bool, error) { - if c.sidecarConfig[container] != nil { + if _, ok := c.sidecarConfig[container]; ok { for _, allowed := range sidecarDependsOnStatus { if status == allowed { return true, nil @@ -165,7 +165,7 @@ func isEssential(name string, s convertSidecarOpts) bool { } func isEssentialStatus(status string, container string, c convertSidecarOpts) (bool, error) { - if c.sidecarConfig[container] != nil { + if _, ok := c.sidecarConfig[container]; ok { if status == dependsOnStart { return true, nil } @@ -232,7 +232,7 @@ func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { // Add any sidecar dependencies. for name, sidecar := range s.sidecarConfig { for dep := range sidecar.DependsOn { - if s.sidecarConfig[dep] == nil && dep != s.workloadName { + if _, ok := s.sidecarConfig[dep]; !ok && dep != s.workloadName { return nil, errInvalidContainer } @@ -242,7 +242,7 @@ func buildDependencyGraph(s convertSidecarOpts) (*graph, error) { // Add any image dependencies. for dep := range s.imageConfig.DependsOn { - if s.sidecarConfig[dep] == nil && dep != s.workloadName { + if _, ok := s.sidecarConfig[dep]; !ok && dep != s.workloadName { return nil, errInvalidContainer } @@ -261,7 +261,7 @@ func (g *graph) add(fromNode, toNode string) { hasNode := false // Add origin node if doesn't exist. - if g.nodes[fromNode] == nil { + if _, ok := g.nodes[fromNode]; !ok { g.nodes[fromNode] = []string{} } From 59d909fad4b089957fad988e1a0e0c9d076b4d09 Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 10 Jun 2021 17:25:17 -0400 Subject: [PATCH 12/13] adjusts modularity of sidecar check, naming adjusts sidecar check to be its own function adjusts naming --- .../deploy/cloudformation/stack/validate.go | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 42da4cca793..1b7e91f74f6 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -44,9 +44,9 @@ var ( // Container dependency status options var ( - essentialContainerStatus = []string{dependsOnStart, dependsOnHealthy} - dependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} - sidecarDependsOnStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} + essentialContainerValidStatus = []string{dependsOnStart, dependsOnHealthy} + dependsOnValidStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} + sidecarDependsOnValidStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -124,10 +124,11 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c } for name, status := range in.DependsOn { - if ok, err := isValidStatus(status, name, s); !ok { + if ok, err := isValidStatusForContainer(status, name, s); !ok { return err } - if ok, err := isEssentialStatus(status, name, s); isEssential(name, s) && !ok { + // Container cannot depend on essential containers if status is 'COMPLETE' or 'SUCCESS' + if ok, err := isEssentialStatus(status, name, s); isEssentialContainer(name, s) && !ok { return err } } @@ -135,9 +136,16 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c return nil } -func isValidStatus(status string, container string, c convertSidecarOpts) (bool, error) { - if _, ok := c.sidecarConfig[container]; ok { - for _, allowed := range sidecarDependsOnStatus { +func isSidecar(container string, sidecars map[string]*manifest.SidecarConfig) bool { + if _, ok := sidecars[container]; ok { + return true + } + return false +} + +func isValidStatusForContainer(status string, container string, c convertSidecarOpts) (bool, error) { + if isSidecar(container, c.sidecarConfig) { + for _, allowed := range sidecarDependsOnValidStatus { if status == allowed { return true, nil } @@ -145,7 +153,7 @@ func isValidStatus(status string, container string, c convertSidecarOpts) (bool, return false, errInvalidSidecarDependsOnStatus } - for _, allowed := range dependsOnStatus { + for _, allowed := range dependsOnValidStatus { if status == allowed { return true, nil } @@ -153,7 +161,7 @@ func isValidStatus(status string, container string, c convertSidecarOpts) (bool, return false, errInvalidDependsOnStatus } -func isEssential(name string, s convertSidecarOpts) bool { +func isEssentialContainer(name string, s convertSidecarOpts) bool { if s.sidecarConfig == nil { return false } @@ -165,14 +173,14 @@ func isEssential(name string, s convertSidecarOpts) bool { } func isEssentialStatus(status string, container string, c convertSidecarOpts) (bool, error) { - if _, ok := c.sidecarConfig[container]; ok { + if isSidecar(container, c.sidecarConfig) { if status == dependsOnStart { return true, nil } return false, errEssentialSidecarStatus } - for _, allowed := range essentialContainerStatus { + for _, allowed := range essentialContainerValidStatus { if status == allowed { return true, nil } @@ -284,10 +292,10 @@ func validateImageDependsOn(s convertSidecarOpts) error { } for name, status := range s.imageConfig.DependsOn { - if ok, err := isValidStatus(status, name, s); !ok { + if ok, err := isValidStatusForContainer(status, name, s); !ok { return err } - if ok, err := isEssentialStatus(status, name, s); isEssential(name, s) && !ok { + if ok, err := isEssentialStatus(status, name, s); isEssentialContainer(name, s) && !ok { return err } } From 93f6fc6dc581f78570d8c2c1ee9f1cd2a25112be Mon Sep 17 00:00:00 2001 From: Alexandra French Date: Thu, 10 Jun 2021 17:30:23 -0400 Subject: [PATCH 13/13] adjusts status variable naming to statuses --- internal/pkg/deploy/cloudformation/stack/validate.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/pkg/deploy/cloudformation/stack/validate.go b/internal/pkg/deploy/cloudformation/stack/validate.go index 1b7e91f74f6..cace89b8d8d 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -44,9 +44,9 @@ var ( // Container dependency status options var ( - essentialContainerValidStatus = []string{dependsOnStart, dependsOnHealthy} - dependsOnValidStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} - sidecarDependsOnValidStatus = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} + essentialContainerValidStatuses = []string{dependsOnStart, dependsOnHealthy} + dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} + sidecarDependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) // Validate that paths contain only an approved set of characters to guard against command injection. @@ -145,7 +145,7 @@ func isSidecar(container string, sidecars map[string]*manifest.SidecarConfig) bo func isValidStatusForContainer(status string, container string, c convertSidecarOpts) (bool, error) { if isSidecar(container, c.sidecarConfig) { - for _, allowed := range sidecarDependsOnValidStatus { + for _, allowed := range sidecarDependsOnValidStatuses { if status == allowed { return true, nil } @@ -153,7 +153,7 @@ func isValidStatusForContainer(status string, container string, c convertSidecar return false, errInvalidSidecarDependsOnStatus } - for _, allowed := range dependsOnValidStatus { + for _, allowed := range dependsOnValidStatuses { if status == allowed { return true, nil } @@ -180,7 +180,7 @@ func isEssentialStatus(status string, container string, c convertSidecarOpts) (b return false, errEssentialSidecarStatus } - for _, allowed := range essentialContainerValidStatus { + for _, allowed := range essentialContainerValidStatuses { if status == allowed { return true, nil }