Cleanup and fix outputs package

pkg/cfn/outputs/api.go

@@ -36,25 +36,6 @@ const (
 	NodeGroupFeatureLocalSecurityGroup  = "FeatureLocalSecurityGroup"
 )
 
-// MustCollect will use each of the keys and attempt to find an output in the given
-// stack, if any of the keys are not preset it will return an error
-func MustCollect(stack cfn.Stack, keys []string, results map[string]string) error {
-	for _, key := range keys {
-		var value *string
-		for _, x := range stack.Outputs {
-			if *x.OutputKey == key {
-				value = x.OutputValue
-				break
-			}
-		}
-		if value == nil {
-			return fmt.Errorf("no ouput %q in stack %q", key, *stack.StackName)
-		}
-		results[key] = *value
-	}
-	return nil
-}
-
 type (
 	// Collector is a callback function that takes an output value
 	// and may return an error
@@ -75,7 +56,7 @@ func NewCollectorSet(set map[string]Collector) *CollectorSet {
 }
 
 func (c *CollectorSet) doCollect(must bool, stack cfn.Stack) error {
-	for key, collect := range c.set {
+	for key, collector := range c.set {
 		var value *string
 		for _, x := range stack.Outputs {
 			if *x.OutputKey == key {
@@ -84,16 +65,16 @@ func (c *CollectorSet) doCollect(must bool, stack cfn.Stack) error {
 			}
 		}
 		if value == nil {
-			if !must {
-				return nil
-			}
-			err := fmt.Errorf("no ouput %q", key)
-			if stack.StackName != nil {
-				return fmt.Errorf("%s in stack %q", err.Error(), *stack.StackName)
+			if must {
+				err := fmt.Errorf("no ouput %q", key)
+				if stack.StackName != nil {
+					return fmt.Errorf("%s in stack %q", err.Error(), *stack.StackName)
+				}
+				return err
 			}
-			return err
+			continue
 		}
-		if err := collect(*value); err != nil {
+		if err := collector(*value); err != nil {
 			return err
 		}
 	}

pkg/cfn/outputs/api_test.go

@@ -157,27 +157,28 @@ var _ = Describe("CloudFormation stack outputs API", func() {
 			}
 
 			{
-				appendOutput(&stack, "test2", "")
+				appendOutput(&stack, "test3", "")
+				appendOutput(&stack, "test4", "")
 
-				test1 := false
-				test2 := false
+				test3 := false
+				test4 := false
 
 				optionalCollectors := map[string]Collector{
-					"test1": func(_ string) error {
-						test1 = true
+					"test3": func(_ string) error {
+						test3 = true
 						return nil
 					},
-					"test2": func(_ string) error {
-						test2 = true
+					"test4": func(_ string) error {
+						test4 = true
 						return nil
 					},
 				}
 
 				err := Collect(stack, nil, optionalCollectors)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				Expect(test1).To(BeTrue())
-				Expect(test2).To(BeTrue())
+				Expect(test3).To(BeTrue())
+				Expect(test4).To(BeTrue())
 			}
 		}
 	})

pkg/eks/compatibility.go

@@ -20,14 +20,18 @@ func (c *ClusterProvider) ValidateClusterForCompatibility(cfg *api.ClusterConfig
 		return errors.Wrap(err, "getting cluster stacks")
 	}
 
-	sharedClusterNodeSG := ""
-	for _, x := range cluster.Outputs {
-		if *x.OutputKey == outputs.ClusterSharedNodeSecurityGroup {
-			sharedClusterNodeSG = *x.OutputValue
-		}
-	}
+	err = outputs.Collect(*cluster,
+		map[string]outputs.Collector{
+			outputs.ClusterSharedNodeSecurityGroup: func(v string) error {
+				logger.Debug("ClusterSharedNodeSecurityGroup = %s", v)
+				return nil
+			},
+		},
+		nil,
+	)
 
-	if sharedClusterNodeSG == "" {
+	if err != nil {
+		logger.Debug("err = %s", err.Error())
 		return fmt.Errorf(
 			"shared node security group missing, to fix this run 'eksctl utils update-cluster-stack --name=%s --region=%s'",
 			cfg.Metadata.Name,
@@ -43,37 +47,41 @@ func isNodeGroupCompatible(name string, info manager.StackInfo) bool {
 	usesSharedSecurityGroup := false
 	hasLocalSecurityGroupFlag := false
 
-	for _, x := range info.Stack.Outputs {
-		if *x.OutputKey == outputs.NodeGroupFeatureSharedSecurityGroup {
-			hasSharedSecurityGroupFlag = true
-			switch *x.OutputValue {
-			case "true":
-				usesSharedSecurityGroup = true
-			case "false":
-				usesSharedSecurityGroup = false
-			}
-		}
-		if *x.OutputKey == outputs.NodeGroupFeatureLocalSecurityGroup {
-			hasLocalSecurityGroupFlag = true
-		}
-	}
-
-	if !hasSharedSecurityGroupFlag && !hasLocalSecurityGroupFlag {
-		// has none of the feture flags makes it incompatible
+	_ = outputs.Collect(*info.Stack,
+		nil,
+		map[string]outputs.Collector{
+			outputs.NodeGroupFeatureSharedSecurityGroup: func(v string) error {
+				hasSharedSecurityGroupFlag = true
+				switch v {
+				case "true":
+					usesSharedSecurityGroup = true
+				case "false":
+					usesSharedSecurityGroup = false
+				}
+				return nil
+			},
+			outputs.NodeGroupFeatureLocalSecurityGroup: func(v string) error {
+				hasLocalSecurityGroupFlag = true
+				return nil
+			},
+		},
+	)
+
+	if !hasSharedSecurityGroupFlag {
+		// if it doesn't have `outputs.NodeGroupFeatureSharedSecurityGroup` flags at all,
+		// it must be incompatible
 		return false
 	}
 
-	if hasSharedSecurityGroupFlag {
-		if !hasLocalSecurityGroupFlag && !usesSharedSecurityGroup {
-			// when `outputs.NodeGroupFeatureSharedSecurityGroup` was added in 0.1.19, v1alpha3 didn't set
-			// `ng.SharedSecurityGroup=true` by default, and (technically) it implies the nodegroup maybe compatible,
-			// however users are unaware of that API v1alpha3 was broken this way, so we need this warning;
-			// as `outputs.NodeGroupFeatureLocalSecurityGroup` was added in 0.1.20/v1alpha4, it can be used to
-			// infer use of v1alpha3 and thereby warn the user that their cluster may had been misconfigured
-			logger.Warning("looks like nodegroup %q was created using v1alpha3 API and is not using shared SG", name)
-			logger.Warning("if you didn't disable shared SG and expect that pods running on %q should have access to all other pods", name)
-			logger.Warning("then you should replace nodegroup %q or patch the configuration", name)
-		}
+	if !hasLocalSecurityGroupFlag && !usesSharedSecurityGroup {
+		// when `outputs.NodeGroupFeatureSharedSecurityGroup` was added in 0.1.19, v1alpha3 didn't set
+		// `ng.SharedSecurityGroup=true` by default, and (technically) it implies the nodegroup maybe compatible,
+		// however users are unaware of that API v1alpha3 was broken this way, so we need this warning;
+		// as `outputs.NodeGroupFeatureLocalSecurityGroup` was added in 0.1.20/v1alpha4, it can be used to
+		// infer use of v1alpha3 and thereby warn the user that their cluster may had been misconfigured
+		logger.Warning("looks like nodegroup %q was created using v1alpha3 API and is not using shared SG", name)
+		logger.Warning("if you didn't disable shared SG and expect that pods running on %q should have access to all other pods", name)
+		logger.Warning("then you should replace nodegroup %q or patch the configuration", name)
 	}
 
 	return true