chore(manifest): move platform validation and container dependencies (#2869)

<!-- Provide summary of changes -->
part of #2818
<!-- Issue number, if available. E.g. "Fixes #31", "Addresses #42, 77" -->

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

Author: iamhopaul123

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{

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 {

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 {

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
+}

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)
+		})
+	}
+}

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"`

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"`

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"`