Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Adds DependsOn converters and logic #2421

Merged
merged 14 commits into from
Jun 10, 2021
Merged

chore: Adds DependsOn converters and logic #2421

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cc58eae
adjusts sidecar parameters due to adjusted conversion logic
xar-tol Jun 4, 2021
c3e3f25
adds dependency validation logic
xar-tol Jun 4, 2021
a6567ce
simplifies booleans
xar-tol Jun 4, 2021
ed913b2
adjusts formatting
xar-tol Jun 4, 2021
f20095d
adjusts graph, addreses comments
xar-tol Jun 7, 2021
2850882
adjusts functions for use of convertSidecarOpts
xar-tol Jun 7, 2021
0bb561d
adjusta errors, modularity, statuses
xar-tol Jun 8, 2021
b1245b6
adjusts essential container status error
xar-tol Jun 8, 2021
6966c94
removes uncessary check for dependsOn size
xar-tol Jun 9, 2021
a15557d
adjusts status check, errors
xar-tol Jun 10, 2021
07487db
adjusts nil format to comma ok format
xar-tol Jun 10, 2021
59d909f
adjusts modularity of sidecar check, naming
xar-tol Jun 10, 2021
93f6fc6
adjusts status variable naming to statuses
xar-tol Jun 10, 2021
258d91e
Merge branch 'mainline' into xar-tol/sidecar-dependency-converters
mergify[bot] Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions internal/pkg/deploy/cloudformation/stack/transformers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down
97 changes: 56 additions & 41 deletions internal/pkg/deploy/cloudformation/stack/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a quick comment here explaining this bit of logic?

// Container cannot depend on essential containers having status `complete` or `success`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a clarifying comment.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to isValidStatusForContainer? It might be a bit more transparent what we're checking that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted naming.

if _, ok := c.sidecarConfig[container]; ok {
for _, allowed := range sidecarDependsOnStatus {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rename this const to sidecarDependsOnValidStatuses or sth similar for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted.

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 {
Expand All @@ -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 _, ok := c.sidecarConfig[container]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this bit of code is used pretty often, we might want to refactor it into a function:

func isSidecar(container string, sidecarConfig map[string]*manifest.SidecarConfig) bool

and this line can be replaced by:

Suggested change
if _, ok := c.sidecarConfig[container]; ok {
if isSidecar(container, c.sidecarConfig) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better modularized.

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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -217,21 +232,21 @@ 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
}

dependencyGraph.Add(name, dep)
dependencyGraph.add(name, dep)
}
}

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

dependencyGraph.Add(s.workloadName, dep)
dependencyGraph.add(s.workloadName, dep)
}

return &dependencyGraph, nil
Expand All @@ -242,11 +257,11 @@ 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.
if g.nodes[fromNode] == nil {
if _, ok := g.nodes[fromNode]; !ok {
g.nodes[fromNode] = []string{}
}

Expand All @@ -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
}
}

Expand Down
41 changes: 32 additions & 9 deletions internal/pkg/deploy/cloudformation/stack/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -457,7 +480,7 @@ func TestValidateImageDependsOn(t *testing.T) {
inSidecars: map[string]*manifest.SidecarConfig{
"sidecar": {},
},
wantErr: errEssentialContainerStatus,
wantErr: errEssentialSidecarStatus,
},
}
for name, tc := range testCases {
Expand Down