Skip to content

Commit

Permalink
chore(manifest): move platform validation and container dependencies (a…
Browse files Browse the repository at this point in the history
…ws#2869)

<!-- Provide summary of changes -->
part of aws#2818
<!-- Issue number, if available. E.g. "Fixes aws#31", "Addresses aws#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.
  • Loading branch information
iamhopaul123 authored and thrau committed Dec 9, 2022
1 parent 994245a commit dc42b97
Show file tree
Hide file tree
Showing 14 changed files with 805 additions and 117 deletions.
6 changes: 3 additions & 3 deletions internal/pkg/deploy/cloudformation/stack/transformers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down
40 changes: 14 additions & 26 deletions internal/pkg/deploy/cloudformation/stack/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
12 changes: 6 additions & 6 deletions internal/pkg/deploy/cloudformation/stack/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -476,7 +476,7 @@ func TestValidateImageDependsOn(t *testing.T) {
inSidecars: map[string]*manifest.SidecarConfig{
"sidecar": {},
},
wantErr: errEssentialSidecarStatus,
wantErr: errEssentialContainerStatus,
},
}
for name, tc := range testCases {
Expand Down
97 changes: 97 additions & 0 deletions internal/pkg/graph/graph.go
Original file line number Diff line number Diff line change
@@ -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
}
93 changes: 93 additions & 0 deletions internal/pkg/graph/graph_test.go
Original file line number Diff line number Diff line change
@@ -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)
})
}
}
1 change: 1 addition & 0 deletions internal/pkg/manifest/backend_svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
1 change: 1 addition & 0 deletions internal/pkg/manifest/job.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
1 change: 1 addition & 0 deletions internal/pkg/manifest/lb_web_svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
Loading

0 comments on commit dc42b97

Please sign in to comment.