From dc42b97b8a6f3a68e95ee58695095a0d5440e316 Mon Sep 17 00:00:00 2001 From: Penghao He Date: Mon, 27 Sep 2021 15:03:46 -0700 Subject: [PATCH] chore(manifest): move platform validation and container dependencies (#2869) part of #2818 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License. --- .../cloudformation/stack/transformers_test.go | 6 +- .../deploy/cloudformation/stack/validate.go | 40 +- .../cloudformation/stack/validate_test.go | 12 +- internal/pkg/graph/graph.go | 97 ++++ internal/pkg/graph/graph_test.go | 93 ++++ internal/pkg/manifest/backend_svc.go | 1 + internal/pkg/manifest/job.go | 1 + internal/pkg/manifest/lb_web_svc.go | 1 + internal/pkg/manifest/validate.go | 195 +++++++- internal/pkg/manifest/validate_test.go | 431 ++++++++++++++++-- internal/pkg/manifest/worker_svc.go | 1 + internal/pkg/manifest/workload.go | 11 +- site/content/docs/developing/sidecars.en.md | 30 -- .../content/docs/include/sidecar-config.en.md | 3 + 14 files changed, 805 insertions(+), 117 deletions(-) create mode 100644 internal/pkg/graph/graph.go create mode 100644 internal/pkg/graph/graph_test.go diff --git a/internal/pkg/deploy/cloudformation/stack/transformers_test.go b/internal/pkg/deploy/cloudformation/stack/transformers_test.go index 64f04e44e1a..1417f74c793 100644 --- a/internal/pkg/deploy/cloudformation/stack/transformers_test.go +++ b/internal/pkg/deploy/cloudformation/stack/transformers_test.go @@ -1490,7 +1490,7 @@ func Test_convertImageDependsOn(t *testing.T) { Essential: aws.Bool(false), }, }, - wantedError: errInvalidSidecarDependsOnStatus, + wantedError: errInvalidDependsOnStatus, }, "invalid implied essential container depdendency": { inImage: &manifest.Image{ @@ -1501,7 +1501,7 @@ func Test_convertImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantedError: errEssentialSidecarStatus, + wantedError: errEssentialContainerStatus, }, "invalid set essential container depdendency": { inImage: &manifest.Image{ @@ -1514,7 +1514,7 @@ func Test_convertImageDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantedError: errEssentialSidecarStatus, + wantedError: errEssentialContainerStatus, }, "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 e17a9a491f8..1a211084def 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate.go +++ b/internal/pkg/deploy/cloudformation/stack/validate.go @@ -32,28 +32,25 @@ 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) - 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) - errInvalidPubSubTopicName = errors.New("topic names can only contain letters, numbers, underscores, and hypthens") - errInvalidSvcName = errors.New("service names cannot be empty") - errSvcNameTooLong = errors.New("service names must not exceed 255 characters") - errSvcNameBadFormat = errors.New("service names must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") + 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) + errInvalidPubSubTopicName = errors.New("topic names can only contain letters, numbers, underscores, and hypthens") + errInvalidSvcName = errors.New("service names cannot be empty") + errSvcNameTooLong = errors.New("service names must not exceed 255 characters") + errSvcNameBadFormat = errors.New("service names must start with a letter, contain only lower-case letters, numbers, and hyphens, and have no consecutive or trailing hyphen") ) // Container dependency status options. var ( essentialContainerValidStatuses = []string{dependsOnStart, dependsOnHealthy} dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} - sidecarDependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess} ) // Regex options. @@ -167,15 +164,6 @@ 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 sidecarDependsOnValidStatuses { - if status == allowed { - return true, nil - } - } - return false, errInvalidSidecarDependsOnStatus - } - for _, allowed := range dependsOnValidStatuses { if status == allowed { return true, nil @@ -200,7 +188,7 @@ func isEssentialStatus(status string, container string, c convertSidecarOpts) (b if status == dependsOnStart { return true, nil } - return false, errEssentialSidecarStatus + return false, errEssentialContainerStatus } for _, allowed := range essentialContainerValidStatuses { diff --git a/internal/pkg/deploy/cloudformation/stack/validate_test.go b/internal/pkg/deploy/cloudformation/stack/validate_test.go index 81489f9d492..3903396c6f0 100644 --- a/internal/pkg/deploy/cloudformation/stack/validate_test.go +++ b/internal/pkg/deploy/cloudformation/stack/validate_test.go @@ -213,7 +213,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { Essential: aws.Bool(false), }, }, - wantErr: errInvalidSidecarDependsOnStatus, + wantErr: errInvalidDependsOnStatus, }, "error when container dependency status is invalid": { inSidecar: &manifest.SidecarConfig{ @@ -238,7 +238,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantErr: errEssentialSidecarStatus, + wantErr: errEssentialContainerStatus, }, "error when implied essential sidecar has a status besides start": { inSidecar: &manifest.SidecarConfig{ @@ -250,7 +250,7 @@ func TestValidateSidecarDependsOn(t *testing.T) { "sidecar": {}, "sidecar2": {}, }, - wantErr: errEssentialSidecarStatus, + wantErr: errEssentialContainerStatus, }, "error when essential container dependency status is invalid": { inSidecar: &manifest.SidecarConfig{ @@ -452,7 +452,7 @@ func TestValidateImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantErr: errInvalidSidecarDependsOnStatus, + wantErr: errInvalidDependsOnStatus, }, "error when set essential sidecar container has a status besides start": { inImage: &manifest.Image{ @@ -465,7 +465,7 @@ func TestValidateImageDependsOn(t *testing.T) { Essential: aws.Bool(true), }, }, - wantErr: errEssentialSidecarStatus, + wantErr: errEssentialContainerStatus, }, "error when implied essential sidecar container has a status besides start": { inImage: &manifest.Image{ @@ -476,7 +476,7 @@ func TestValidateImageDependsOn(t *testing.T) { inSidecars: map[string]*manifest.SidecarConfig{ "sidecar": {}, }, - wantErr: errEssentialSidecarStatus, + wantErr: errEssentialContainerStatus, }, } for name, tc := range testCases { diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go new file mode 100644 index 00000000000..073286e5f07 --- /dev/null +++ b/internal/pkg/graph/graph.go @@ -0,0 +1,97 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package graph provides functionality for directed graphs. +package graph + +// nodeStatus denotes the visiting status of a node when running DFS in a graph. +type nodeStatus int + +const ( + unvisited nodeStatus = iota + 1 + visiting + visited +) + +// Graph represents a directed graph. +type Graph struct { + nodes map[string]neighbors +} + +// Edge represents one edge of a directed graph. +type Edge struct { + From string + To string +} + +type neighbors map[string]bool + +// New initiates a new Graph. +func New() *Graph { + return &Graph{ + nodes: make(map[string]neighbors), + } +} + +// Add adds a connection between two Nodes. +func (g *Graph) Add(edge Edge) { + fromNode, toNode := edge.From, edge.To + // Add origin node if doesn't exist. + if _, ok := g.nodes[fromNode]; !ok { + g.nodes[fromNode] = make(neighbors) + } + // Add edge. + g.nodes[fromNode][toNode] = true +} + +type findCycleTempVars struct { + status map[string]nodeStatus + nodeParent map[string]string + cycleStart string + cycleEnd string +} + +// IsAcyclic checks if the graph is acyclic. If not, return the first detected cycle. +func (g *Graph) IsAcyclic() ([]string, bool) { + var cycle []string + status := make(map[string]nodeStatus) + for node := range g.nodes { + status[node] = unvisited + } + temp := findCycleTempVars{ + status: status, + nodeParent: make(map[string]string), + } + // We will run a series of DFS in the graph. Initially all vertices are marked unvisited. + // From each unvisited node, start the DFS, mark it visiting while entering and mark it visited on exit. + // If DFS moves to a visiting node, then we have found a cycle. The cycle itself can be reconstructed using parent map. + // See https://cp-algorithms.com/graph/finding-cycle.html + for node := range g.nodes { + if status[node] == unvisited && g.hasCycles(&temp, node) { + for n := temp.cycleStart; n != temp.cycleEnd; n = temp.nodeParent[n] { + cycle = append(cycle, n) + } + cycle = append(cycle, temp.cycleEnd) + return cycle, false + } + } + return nil, true +} + +func (g *Graph) hasCycles(temp *findCycleTempVars, currNode string) bool { + temp.status[currNode] = visiting + for node := range g.nodes[currNode] { + if temp.status[node] == unvisited { + temp.nodeParent[node] = currNode + if g.hasCycles(temp, node) { + return true + } + } else if temp.status[node] == visiting { + temp.cycleStart = currNode + temp.cycleEnd = node + return true + } + } + temp.status[currNode] = visited + return false +} diff --git a/internal/pkg/graph/graph_test.go b/internal/pkg/graph/graph_test.go new file mode 100644 index 00000000000..4f595ef872d --- /dev/null +++ b/internal/pkg/graph/graph_test.go @@ -0,0 +1,93 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package graph + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestGraph_Add(t *testing.T) { + t.Run("success", func(t *testing.T) { + // GIVEN + graph := New() + + // WHEN + graph.Add(Edge{ + From: "A", + To: "B", + }) + graph.Add(Edge{ + From: "B", + To: "A", + }) + graph.Add(Edge{ + From: "A", + To: "C", + }) + + // THEN + require.Equal(t, graph.nodes["A"], neighbors{"B": true, "C": true}) + require.Equal(t, graph.nodes["B"], neighbors{"A": true}) + }) +} + +func TestGraph_IsAcyclic(t *testing.T) { + testCases := map[string]struct { + graph Graph + + isAcyclic bool + cycle []string + }{ + "small non acyclic graph": { + graph: Graph{ + nodes: map[string]neighbors{ + "A": {"B": true, "C": true}, + "B": {"A": true}, + }, + }, + + isAcyclic: false, + cycle: []string{"A", "B"}, + }, + "non acyclic": { + graph: Graph{ + nodes: map[string]neighbors{ + "K": {"F": true}, + "A": {"B": true, "C": true}, + "B": {"D": true, "E": true}, + "E": {"G": true}, + "F": {"G": true}, + "G": {"A": true}, + }, + }, + + isAcyclic: false, + cycle: []string{"A", "G", "E", "B"}, + }, + "acyclic": { + graph: Graph{ + nodes: map[string]neighbors{ + "A": {"B": true, "C": true}, + "B": {"D": true}, + "E": {"G": true}, + "F": {"G": true}, + }, + }, + + isAcyclic: true, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + // WHEN + gotCycle, gotAcyclic := tc.graph.IsAcyclic() + + // THEN + require.Equal(t, tc.isAcyclic, gotAcyclic) + require.ElementsMatch(t, tc.cycle, gotCycle) + }) + } +} diff --git a/internal/pkg/manifest/backend_svc.go b/internal/pkg/manifest/backend_svc.go index e67f5171c4a..88e74f81192 100644 --- a/internal/pkg/manifest/backend_svc.go +++ b/internal/pkg/manifest/backend_svc.go @@ -25,6 +25,7 @@ type BackendService struct { // BackendServiceConfig holds the configuration that can be overridden per environments. type BackendServiceConfig struct { + name string ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"` ImageOverride `yaml:",inline"` TaskConfig `yaml:",inline"` diff --git a/internal/pkg/manifest/job.go b/internal/pkg/manifest/job.go index b54f32a4f4d..f7881f601b0 100644 --- a/internal/pkg/manifest/job.go +++ b/internal/pkg/manifest/job.go @@ -36,6 +36,7 @@ type ScheduledJob struct { // ScheduledJobConfig holds the configuration for a scheduled job type ScheduledJobConfig struct { + name string ImageConfig ImageWithHealthcheck `yaml:"image,flow"` ImageOverride `yaml:",inline"` TaskConfig `yaml:",inline"` diff --git a/internal/pkg/manifest/lb_web_svc.go b/internal/pkg/manifest/lb_web_svc.go index 503b8cdbb70..9139871c192 100644 --- a/internal/pkg/manifest/lb_web_svc.go +++ b/internal/pkg/manifest/lb_web_svc.go @@ -46,6 +46,7 @@ type LoadBalancedWebService struct { // LoadBalancedWebServiceConfig holds the configuration for a load balanced web service. type LoadBalancedWebServiceConfig struct { + name string ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"` ImageOverride `yaml:",inline"` RoutingRule `yaml:"http,flow"` diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index d621e0658e4..ffb91b3cebc 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -8,14 +8,28 @@ import ( "fmt" "net" "regexp" + "sort" "strconv" "strings" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/copilot-cli/internal/pkg/graph" + "github.com/dustin/go-humanize/english" +) + +// Container dependency status constants. +const ( + dependsOnStart = "START" + dependsOnComplete = "COMPLETE" + dependsOnSuccess = "SUCCESS" + dependsOnHealthy = "HEALTHY" ) var ( intRangeBandRegexp = regexp.MustCompile(`^(\d+)-(\d+)$`) + + essentialContainerDependsOnValidStatuses = []string{dependsOnStart, dependsOnHealthy} + dependsOnValidStatuses = []string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy} ) // Validate returns nil if LoadBalancedWebServiceConfig is configured correctly. @@ -38,7 +52,7 @@ func (l *LoadBalancedWebServiceConfig) Validate() error { } for k, v := range l.Sidecars { if err = v.Validate(); err != nil { - return fmt.Errorf(`validate sidecars[%s]: %w`, k, err) + return fmt.Errorf(`validate "sidecars[%s]": %w`, k, err) } } if err = l.Network.Validate(); err != nil { @@ -49,9 +63,16 @@ func (l *LoadBalancedWebServiceConfig) Validate() error { } for ind, taskDefOverride := range l.TaskDefOverrides { if err = taskDefOverride.Validate(); err != nil { - return fmt.Errorf(`validate taskdef_overrides[%d]: %w`, ind, err) + return fmt.Errorf(`validate "taskdef_overrides[%d]": %w`, ind, err) } } + if err = validateContainerDeps(validateDependenciesOpts{ + sidecarConfig: l.Sidecars, + imageConfig: l.ImageConfig.Image, + mainContainerName: l.name, + }); err != nil { + return fmt.Errorf("validate container dependencies: %w", err) + } return nil } @@ -72,7 +93,7 @@ func (b *BackendServiceConfig) Validate() error { } for k, v := range b.Sidecars { if err = v.Validate(); err != nil { - return fmt.Errorf(`validate sidecars[%s]: %w`, k, err) + return fmt.Errorf(`validate "sidecars[%s]": %w`, k, err) } } if err = b.Network.Validate(); err != nil { @@ -83,9 +104,16 @@ func (b *BackendServiceConfig) Validate() error { } for ind, taskDefOverride := range b.TaskDefOverrides { if err = taskDefOverride.Validate(); err != nil { - return fmt.Errorf(`validate taskdef_overrides[%d]: %w`, ind, err) + return fmt.Errorf(`validate "taskdef_overrides[%d]": %w`, ind, err) } } + if err = validateContainerDeps(validateDependenciesOpts{ + sidecarConfig: b.Sidecars, + imageConfig: b.ImageConfig.Image, + mainContainerName: b.name, + }); err != nil { + return fmt.Errorf("validate container dependencies: %w", err) + } return nil } @@ -121,7 +149,7 @@ func (w *WorkerServiceConfig) Validate() error { } for k, v := range w.Sidecars { if err = v.Validate(); err != nil { - return fmt.Errorf(`validate sidecars[%s]: %w`, k, err) + return fmt.Errorf(`validate "sidecars[%s]": %w`, k, err) } } if err = w.Network.Validate(); err != nil { @@ -132,9 +160,16 @@ func (w *WorkerServiceConfig) Validate() error { } for ind, taskDefOverride := range w.TaskDefOverrides { if err = taskDefOverride.Validate(); err != nil { - return fmt.Errorf(`validate taskdef_overrides[%d]: %w`, ind, err) + return fmt.Errorf(`validate "taskdef_overrides[%d]": %w`, ind, err) } } + if err = validateContainerDeps(validateDependenciesOpts{ + sidecarConfig: w.Sidecars, + imageConfig: w.ImageConfig.Image, + mainContainerName: w.name, + }); err != nil { + return fmt.Errorf("validate container dependencies: %w", err) + } return nil } @@ -155,7 +190,7 @@ func (s *ScheduledJobConfig) Validate() error { } for k, v := range s.Sidecars { if err = v.Validate(); err != nil { - return fmt.Errorf(`validate sidecars[%s]: %w`, k, err) + return fmt.Errorf(`validate "sidecars[%s]": %w`, k, err) } } if err = s.Network.Validate(); err != nil { @@ -172,9 +207,16 @@ func (s *ScheduledJobConfig) Validate() error { } for ind, taskDefOverride := range s.TaskDefOverrides { if err = taskDefOverride.Validate(); err != nil { - return fmt.Errorf(`validate taskdef_overrides[%d]: %w`, ind, err) + return fmt.Errorf(`validate "taskdef_overrides[%d]": %w`, ind, err) } } + if err = validateContainerDeps(validateDependenciesOpts{ + sidecarConfig: s.Sidecars, + imageConfig: s.ImageConfig.Image, + mainContainerName: s.name, + }); err != nil { + return fmt.Errorf("validate container dependencies: %w", err) + } return nil } @@ -224,6 +266,30 @@ func (i *Image) Validate() error { mustExist: true, } } + if err = i.DependsOn.Validate(); err != nil { + return fmt.Errorf(`validate "depends_on": %w`, err) + } + return nil +} + +// Validate returns nil if DependsOn is configured correctly. +func (d *DependsOn) Validate() error { + if d == nil { + return nil + } + for _, v := range *d { + status := strings.ToUpper(v) + var isValid bool + for _, allowed := range dependsOnValidStatuses { + if status == allowed { + isValid = true + break + } + } + if !isValid { + return fmt.Errorf("container dependency status must be one of %s", english.WordSeries([]string{dependsOnStart, dependsOnComplete, dependsOnSuccess, dependsOnHealthy}, "or")) + } + } return nil } @@ -357,16 +423,26 @@ func (p *PlatformString) Validate() error { if p == nil { return nil } - // TODO: Consolidate with "UnmarshalYAML". + if err := validatePlatform(p); err != nil { + return err + } return nil } // Validate returns nil if PlatformArgsOrString is configured correctly. -// TODO: add validation once "feat/pencere" is merged. func (p *PlatformArgs) Validate() error { if p.isEmpty() { return nil } + if !p.bothSpecified() { + return errors.New(`fields "osfamily" and "architecture" must either both be specified or both be empty`) + } + if err := validateOS(p.OSFamily); err != nil { + return err + } + if err := validateArch(p.Arch); err != nil { + return err + } return nil } @@ -623,6 +699,9 @@ func (s *SidecarConfig) Validate() error { if err := s.HealthCheck.Validate(); err != nil { return fmt.Errorf(`validate "healthcheck": %w`, err) } + if err := s.DependsOn.Validate(); err != nil { + return fmt.Errorf(`validate "depends_on": %w`, err) + } return s.ImageOverride.Validate() } @@ -734,3 +813,99 @@ func (d *DeadLetterQueue) Validate() error { func (*OverrideRule) Validate() error { return nil } + +type validateDependenciesOpts struct { + mainContainerName string + sidecarConfig map[string]*SidecarConfig + imageConfig Image +} + +type containerDependency struct { + dependsOn DependsOn + isEssential bool +} + +func validateContainerDeps(opts validateDependenciesOpts) error { + containerDependencies := make(map[string]containerDependency) + containerDependencies[opts.mainContainerName] = containerDependency{ + dependsOn: opts.imageConfig.DependsOn, + isEssential: true, + } + for name, config := range opts.sidecarConfig { + containerDependencies[name] = containerDependency{ + dependsOn: config.DependsOn, + isEssential: config.Essential == nil || aws.BoolValue(config.Essential), + } + } + if err := validateDepsForEssentialContainers(containerDependencies); err != nil { + return err + } + return validateNoCircularDependencies(opts) +} + +func validateDepsForEssentialContainers(deps map[string]containerDependency) error { + for name, containerDep := range deps { + for dep, status := range containerDep.dependsOn { + if !deps[dep].isEssential { + continue + } + if err := validateEssentialContainerDependency(dep, strings.ToUpper(status)); err != nil { + return fmt.Errorf("validate %s container dependencies status: %w", name, err) + } + } + } + return nil +} + +func validateEssentialContainerDependency(name, status string) error { + for _, allowed := range essentialContainerDependsOnValidStatuses { + if status == allowed { + return nil + } + } + return fmt.Errorf("essential container %s can only have status %s", name, english.WordSeries([]string{dependsOnStart, dependsOnHealthy}, "or")) +} + +func validateNoCircularDependencies(opts validateDependenciesOpts) error { + dependencies, err := buildDependencyGraph(opts) + if err != nil { + return err + } + cycle, ok := dependencies.IsAcyclic() + if ok { + return nil + } + if len(cycle) == 1 { + return fmt.Errorf("container %s cannot depend on itself", cycle[0]) + } + // Stablize unit tests. + sort.SliceStable(cycle, func(i, j int) bool { return cycle[i] < cycle[j] }) + return fmt.Errorf("circular container dependency chain includes the following containers: %s", cycle) +} + +func buildDependencyGraph(opts validateDependenciesOpts) (*graph.Graph, error) { + dependencyGraph := graph.New() + // Add any sidecar dependencies. + for name, sidecar := range opts.sidecarConfig { + for dep := range sidecar.DependsOn { + if _, ok := opts.sidecarConfig[dep]; !ok && dep != opts.mainContainerName { + return nil, fmt.Errorf("container %s does not exist", dep) + } + dependencyGraph.Add(graph.Edge{ + From: name, + To: dep, + }) + } + } + // Add any image dependencies. + for dep := range opts.imageConfig.DependsOn { + if _, ok := opts.sidecarConfig[dep]; !ok && dep != opts.mainContainerName { + return nil, fmt.Errorf("container %s does not exist", dep) + } + dependencyGraph.Add(graph.Edge{ + From: opts.mainContainerName, + To: dep, + }) + } + return dependencyGraph, nil +} diff --git a/internal/pkg/manifest/validate_test.go b/internal/pkg/manifest/validate_test.go index 05c614e2708..1c3dfa0d9fe 100644 --- a/internal/pkg/manifest/validate_test.go +++ b/internal/pkg/manifest/validate_test.go @@ -25,7 +25,8 @@ func TestLoadBalancedWebServiceConfig_Validate(t *testing.T) { testCases := map[string]struct { lbConfig LoadBalancedWebServiceConfig - wantedErrorPrefix string + wantedError error + wantedErrorMsgPrefix string }{ "error if fail to validate image": { lbConfig: LoadBalancedWebServiceConfig{ @@ -38,7 +39,7 @@ func TestLoadBalancedWebServiceConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "image": `, + wantedErrorMsgPrefix: `validate "image": `, }, "error if fail to validate http": { lbConfig: LoadBalancedWebServiceConfig{ @@ -48,32 +49,63 @@ func TestLoadBalancedWebServiceConfig_Validate(t *testing.T) { TargetContainerCamelCase: aws.String("mockTargetContainer"), }, }, - wantedErrorPrefix: `validate "http": `, + wantedErrorMsgPrefix: `validate "http": `, }, - "error if fail to validate network": { + "error if fail to validate sidecars": { lbConfig: LoadBalancedWebServiceConfig{ ImageConfig: testImageConfig, - RoutingRule: RoutingRule{ - TargetContainer: aws.String("mockTargetContainer"), + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "foo": "bar", + }, + }, }, + }, + wantedErrorMsgPrefix: `validate "sidecars[foo]": `, + }, + "error if fail to validate network": { + lbConfig: LoadBalancedWebServiceConfig{ + ImageConfig: testImageConfig, Network: NetworkConfig{ vpcConfig{ SecurityGroups: []string{}, }, }, }, - wantedErrorPrefix: `validate "network": `, + wantedErrorMsgPrefix: `validate "network": `, + }, + "error if fail to validate dependencies": { + lbConfig: LoadBalancedWebServiceConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: map[string]string{"bar": "healthy"}, + Essential: aws.Bool(false), + }, + "bar": { + DependsOn: map[string]string{"foo": "healthy"}, + Essential: aws.Bool(false), + }, + }, + }, + wantedErrorMsgPrefix: `validate container dependencies: `, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { gotErr := tc.lbConfig.Validate() - if tc.wantedErrorPrefix != "" { - require.Contains(t, gotErr.Error(), tc.wantedErrorPrefix) - } else { - require.NoError(t, gotErr) + if tc.wantedError != nil { + require.EqualError(t, gotErr, tc.wantedError.Error()) + return + } + if tc.wantedErrorMsgPrefix != "" { + require.Error(t, gotErr) + require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + return } + require.NoError(t, gotErr) }) } } @@ -90,7 +122,8 @@ func TestBackendServiceConfig_Validate(t *testing.T) { testCases := map[string]struct { config BackendServiceConfig - wantedErrorPrefix string + wantedErrorMsgPrefix string + wantedError error }{ "error if fail to validate image": { config: BackendServiceConfig{ @@ -103,7 +136,20 @@ func TestBackendServiceConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "image": `, + wantedErrorMsgPrefix: `validate "image": `, + }, + "error if fail to validate sidecars": { + config: BackendServiceConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "foo": "bar", + }, + }, + }, + }, + wantedErrorMsgPrefix: `validate "sidecars[foo]": `, }, "error if fail to validate network": { config: BackendServiceConfig{ @@ -114,18 +160,37 @@ func TestBackendServiceConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "network": `, + wantedErrorMsgPrefix: `validate "network": `, + }, + "error if fail to validate dependencies": { + config: BackendServiceConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: map[string]string{"bar": "start"}, + }, + "bar": { + DependsOn: map[string]string{"foo": "start"}, + }, + }, + }, + wantedErrorMsgPrefix: `validate container dependencies: `, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { gotErr := tc.config.Validate() - if tc.wantedErrorPrefix != "" { - require.Contains(t, gotErr.Error(), tc.wantedErrorPrefix) - } else { - require.NoError(t, gotErr) + if tc.wantedError != nil { + require.EqualError(t, gotErr, tc.wantedError.Error()) + return + } + if tc.wantedErrorMsgPrefix != "" { + require.Error(t, gotErr) + require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + return } + require.NoError(t, gotErr) }) } } @@ -170,7 +235,8 @@ func TestWorkerServiceConfig_Validate(t *testing.T) { testCases := map[string]struct { config WorkerServiceConfig - wantedErrorPrefix string + wantedError error + wantedErrorMsgPrefix string }{ "error if fail to validate image": { config: WorkerServiceConfig{ @@ -181,7 +247,20 @@ func TestWorkerServiceConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "image": `, + wantedErrorMsgPrefix: `validate "image": `, + }, + "error if fail to validate sidecars": { + config: WorkerServiceConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "foo": "bar", + }, + }, + }, + }, + wantedErrorMsgPrefix: `validate "sidecars[foo]": `, }, "error if fail to validate network": { config: WorkerServiceConfig{ @@ -192,18 +271,37 @@ func TestWorkerServiceConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "network": `, + wantedErrorMsgPrefix: `validate "network": `, + }, + "error if fail to validate dependencies": { + config: WorkerServiceConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: map[string]string{"bar": "start"}, + }, + "bar": { + DependsOn: map[string]string{"foo": "start"}, + }, + }, + }, + wantedErrorMsgPrefix: `validate container dependencies: `, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { gotErr := tc.config.Validate() - if tc.wantedErrorPrefix != "" { - require.Contains(t, gotErr.Error(), tc.wantedErrorPrefix) - } else { - require.NoError(t, gotErr) + if tc.wantedError != nil { + require.EqualError(t, gotErr, tc.wantedError.Error()) + return + } + if tc.wantedErrorMsgPrefix != "" { + require.Error(t, gotErr) + require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + return } + require.NoError(t, gotErr) }) } } @@ -217,7 +315,8 @@ func TestScheduledJobConfig_Validate(t *testing.T) { testCases := map[string]struct { config ScheduledJobConfig - wantedErrorPrefix string + wantedError error + wantedErrorMsgPrefix string }{ "error if fail to validate image": { config: ScheduledJobConfig{ @@ -228,7 +327,20 @@ func TestScheduledJobConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "image": `, + wantedErrorMsgPrefix: `validate "image": `, + }, + "error if fail to validate sidecars": { + config: ScheduledJobConfig{ + ImageConfig: testImageConfig, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "foo": "bar", + }, + }, + }, + }, + wantedErrorMsgPrefix: `validate "sidecars[foo]": `, }, "error if fail to validate network": { config: ScheduledJobConfig{ @@ -239,25 +351,47 @@ func TestScheduledJobConfig_Validate(t *testing.T) { }, }, }, - wantedErrorPrefix: `validate "network": `, + wantedErrorMsgPrefix: `validate "network": `, }, "error if fail to validate on": { config: ScheduledJobConfig{ ImageConfig: testImageConfig, On: JobTriggerConfig{}, }, - wantedErrorPrefix: `validate "on": `, + wantedErrorMsgPrefix: `validate "on": `, + }, + "error if fail to validate dependencies": { + config: ScheduledJobConfig{ + ImageConfig: testImageConfig, + On: JobTriggerConfig{ + Schedule: aws.String("mockSchedule"), + }, + Sidecars: map[string]*SidecarConfig{ + "foo": { + DependsOn: map[string]string{"bar": "start"}, + }, + "bar": { + DependsOn: map[string]string{"foo": "start"}, + }, + }, + }, + wantedErrorMsgPrefix: `validate container dependencies: `, }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { gotErr := tc.config.Validate() - if tc.wantedErrorPrefix != "" { - require.Contains(t, gotErr.Error(), tc.wantedErrorPrefix) - } else { - require.NoError(t, gotErr) + if tc.wantedError != nil { + require.EqualError(t, gotErr, tc.wantedError.Error()) + return + } + if tc.wantedErrorMsgPrefix != "" { + require.Error(t, gotErr) + require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + return } + require.NoError(t, gotErr) }) } } @@ -294,7 +428,8 @@ func TestImage_Validate(t *testing.T) { testCases := map[string]struct { Image Image - wantedError error + wantedError error + wantedErrorMsgPrefix string }{ "error if build and location both specified": { Image: Image{ @@ -309,6 +444,15 @@ func TestImage_Validate(t *testing.T) { Image: Image{}, wantedError: fmt.Errorf(`must specify one of "build" and "location"`), }, + "error if fail to validate depends_on": { + Image: Image{ + Location: aws.String("mockLocation"), + DependsOn: DependsOn{ + "foo": "bar", + }, + }, + wantedErrorMsgPrefix: `validate "depends_on":`, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { @@ -316,13 +460,41 @@ func TestImage_Validate(t *testing.T) { if tc.wantedError != nil { require.EqualError(t, gotErr, tc.wantedError.Error()) - } else { - require.NoError(t, gotErr) + return + } + if tc.wantedErrorMsgPrefix != "" { + require.Error(t, gotErr) + require.Contains(t, gotErr.Error(), tc.wantedErrorMsgPrefix) + return } + require.NoError(t, gotErr) }) } } +func TestDependsOn_Validate(t *testing.T) { + testCases := map[string]struct { + in DependsOn + wanted error + }{ + "should return an error if dependency status is invalid": { + in: DependsOn{ + "foo": "bar", + }, + wanted: errors.New("container dependency status must be one of START, COMPLETE, SUCCESS or HEALTHY"), + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := tc.in.Validate() + if tc.wanted != nil { + require.EqualError(t, err, tc.wanted.Error()) + } else { + require.NoError(t, err) + } + }) + } +} func TestRoutingRule_Validate(t *testing.T) { testCases := map[string]struct { RoutingRule RoutingRule @@ -396,6 +568,14 @@ func TestTaskConfig_Validate(t *testing.T) { wantedErrorPrefix string }{ + "error if fail to validate platform": { + TaskConfig: TaskConfig{ + Platform: PlatformArgsOrString{ + PlatformString: (*PlatformString)(aws.String("foobar")), + }, + }, + wantedErrorPrefix: `validate "platform": `, + }, "error if fail to validate count": { TaskConfig: TaskConfig{ Count: Count{ @@ -437,12 +617,16 @@ func TestTaskConfig_Validate(t *testing.T) { }) } } - func TestPlatformString_Validate(t *testing.T) { testCases := map[string]struct { in PlatformString wanted error - }{} + }{ + "error if platform string is invalid": { + in: PlatformString("foobar"), + wanted: fmt.Errorf("platform foobar is invalid; the valid platform is: linux/amd64"), + }, + } for name, tc := range testCases { t.Run(name, func(t *testing.T) { err := tc.in.Validate() @@ -455,7 +639,44 @@ func TestPlatformString_Validate(t *testing.T) { }) } } +func TestPlatformArgs_Validate(t *testing.T) { + testCases := map[string]struct { + in PlatformArgs + wanted error + }{ + "error if only osfamily is specified": { + in: PlatformArgs{ + OSFamily: aws.String("linux"), + }, + wanted: fmt.Errorf(`fields "osfamily" and "architecture" must either both be specified or both be empty`), + }, + "error if osfamily is invalid": { + in: PlatformArgs{ + OSFamily: aws.String("foo"), + Arch: aws.String("amd64"), + }, + wanted: fmt.Errorf("OS foo is invalid; the valid operating system is: linux"), + }, + "error if arch is invalid": { + in: PlatformArgs{ + OSFamily: aws.String("linux"), + Arch: aws.String("bar"), + }, + wanted: fmt.Errorf("architecture bar is invalid; the valid architecture is: amd64"), + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := tc.in.Validate() + if tc.wanted != nil { + require.EqualError(t, err, tc.wanted.Error()) + } else { + require.NoError(t, err) + } + }) + } +} func TestAdvancedCount_Validate(t *testing.T) { var ( mockPerc = Percentage(70) @@ -862,6 +1083,34 @@ func TestEFSVolumeConfiguration_Validate(t *testing.T) { } } +func TestSidecarConfig_Validate(t *testing.T) { + testCases := map[string]struct { + config SidecarConfig + + wantedErrorPrefix string + }{ + "error if fail to validate depends_on": { + config: SidecarConfig{ + DependsOn: DependsOn{ + "foo": "bar", + }, + }, + wantedErrorPrefix: `validate "depends_on": `, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + gotErr := tc.config.Validate() + + if tc.wantedErrorPrefix != "" { + require.Contains(t, gotErr.Error(), tc.wantedErrorPrefix) + } else { + require.NoError(t, gotErr) + } + }) + } +} + func TestNetworkConfig_Validate(t *testing.T) { testCases := map[string]struct { config NetworkConfig @@ -965,3 +1214,109 @@ func TestJobTriggerConfig_Validate(t *testing.T) { }) } } + +func TestValidateContainerDeps(t *testing.T) { + testCases := map[string]struct { + in validateDependenciesOpts + wanted error + }{ + "should return an error if main container dependencies status is invalid": { + in: validateDependenciesOpts{ + mainContainerName: "mockMainContainer", + imageConfig: Image{ + DependsOn: DependsOn{ + "mockMainContainer": "complete", + }, + }, + }, + wanted: fmt.Errorf("validate mockMainContainer container dependencies status: essential container mockMainContainer can only have status START or HEALTHY"), + }, + "should return an error if sidecar container dependencies status is invalid": { + in: validateDependenciesOpts{ + mainContainerName: "mockMainContainer", + sidecarConfig: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "mockMainContainer": "success", + }, + }, + }, + }, + wanted: fmt.Errorf("validate foo container dependencies status: essential container mockMainContainer can only have status START or HEALTHY"), + }, + "should return an error if a main container dependency does not exist": { + in: validateDependenciesOpts{ + mainContainerName: "mockMainContainer", + imageConfig: Image{ + DependsOn: DependsOn{ + "foo": "healthy", + }, + }, + }, + wanted: fmt.Errorf("container foo does not exist"), + }, + "should return an error if a sidecar container dependency does not exist": { + in: validateDependenciesOpts{ + mainContainerName: "mockMainContainer", + sidecarConfig: map[string]*SidecarConfig{ + "foo": { + DependsOn: DependsOn{ + "bar": "healthy", + }, + }, + }, + }, + wanted: fmt.Errorf("container bar does not exist"), + }, + "should return an error if container depends on itself": { + in: validateDependenciesOpts{ + mainContainerName: "mockMainContainer", + imageConfig: Image{ + DependsOn: DependsOn{ + "mockMainContainer": "healthy", + }, + }, + }, + wanted: fmt.Errorf("container mockMainContainer cannot depend on itself"), + }, + "should return an error if container dependencies graph is cyclic": { + in: validateDependenciesOpts{ + mainContainerName: "alpha", + imageConfig: Image{ + DependsOn: DependsOn{ + "beta": "healthy", + }, + }, + sidecarConfig: map[string]*SidecarConfig{ + "beta": { + DependsOn: DependsOn{ + "gamma": "healthy", + }, + }, + "gamma": { + DependsOn: DependsOn{ + "alpha": "healthy", + }, + }, + "zeta": { + DependsOn: DependsOn{ + "alpha": "healthy", + }, + }, + }, + }, + wanted: fmt.Errorf("circular container dependency chain includes the following containers: [alpha beta gamma]"), + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + err := validateContainerDeps(tc.in) + + if tc.wanted != nil { + require.EqualError(t, err, tc.wanted.Error()) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index c2020356189..c8c94e3a54f 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -27,6 +27,7 @@ type WorkerService struct { // WorkerServiceConfig holds the configuration that can be overridden per environments. type WorkerServiceConfig struct { + name string ImageConfig ImageWithHealthcheck `yaml:"image,flow"` ImageOverride `yaml:",inline"` TaskConfig `yaml:",inline"` diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index 269416a0f93..66580144877 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -75,21 +75,24 @@ type OverrideRule struct { Value yaml.Node `yaml:"value"` } +// DependsOn represents container dependency for a container. +type DependsOn map[string]string + // Image represents the workload's container image. type Image struct { Build BuildArgsOrString `yaml:"build"` // Build an image from a Dockerfile. Location *string `yaml:"location"` // Use an existing image instead. Credentials *string `yaml:"credentials"` // ARN of the secret containing the private repository credentials. DockerLabels map[string]string `yaml:"labels,flow"` // Apply Docker labels to the container at runtime. - DependsOn map[string]string `yaml:"depends_on,flow"` // Add any sidecar dependencies. + DependsOn DependsOn `yaml:"depends_on,flow"` // Add any sidecar dependencies. } // UnmarshalYAML overrides the default YAML unmarshaling logic for the Image // struct, allowing it to perform more complex unmarshaling behavior. // This method implements the yaml.Unmarshaler (v3) interface. -func (i *Image) UnmarshalYAML(node *yaml.Node) error { +func (i *Image) UnmarshalYAML(value *yaml.Node) error { type image Image - if err := node.Decode((*image)(i)); err != nil { + if err := value.Decode((*image)(i)); err != nil { return err } if !i.Build.isEmpty() && i.Location != nil { @@ -430,7 +433,7 @@ type SidecarConfig struct { Secrets map[string]string `yaml:"secrets"` MountPoints []SidecarMountPoint `yaml:"mount_points"` DockerLabels map[string]string `yaml:"labels"` - DependsOn map[string]string `yaml:"depends_on"` + DependsOn DependsOn `yaml:"depends_on"` HealthCheck ContainerHealthCheck `yaml:"healthcheck"` ImageOverride `yaml:",inline"` } diff --git a/site/content/docs/developing/sidecars.en.md b/site/content/docs/developing/sidecars.en.md index f788d1735d4..08085a3a164 100644 --- a/site/content/docs/developing/sidecars.en.md +++ b/site/content/docs/developing/sidecars.en.md @@ -14,36 +14,6 @@ There are two ways of adding sidecars using the Copilot manifest: by specifying ### General sidecars You'll need to provide the URL for the sidecar image. Optionally, you can specify the port you'd like to expose and the credential parameter for [private registry](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/private-auth.html). -``` yaml -sidecars: - : - # Port of the container to expose. (Optional) - port: - # Image URL for the sidecar container. (Required) - image: - # ARN of the secret containing the private repository credentials. (Optional) - credentialsParameter: - # Environment variables for the sidecar container. - variables: - # Secrets to expose to the sidecar container. - secrets: - # Mount paths for EFS volumes specified at the service level. (Optional) - mount_points: - - # Source volume to mount in this sidecar. (Required) - source_volume: - # The path inside the sidecar container at which to mount the volume. (Required) - path: - # Whether to allow the sidecar read-only access to the volume. (Default true) - read_only: - # Optional Docker labels to apply to this container. - labels: - {label key} :