Merge pull request #10217 from hakman/nits

Fix various nits

pkg/apis/kops/validation/instancegroup.go

@@ -248,7 +248,7 @@ func validateInstanceProfile(v *kops.IAMProfileSpec, fldPath *field.Path) field.
 	if v != nil && v.Profile != nil {
 		instanceProfileARN := *v.Profile
 		parsedARN, err := arn.Parse(instanceProfileARN)
-		if err != nil || !strings.HasPrefix(parsedARN.Resource, "instance-profile") {
+		if err != nil || !strings.HasPrefix(parsedARN.Resource, "instance-profile/") {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("profile"), instanceProfileARN,
 				"Instance Group IAM Instance Profile must be a valid aws arn such as arn:aws:iam::123456789012:instance-profile/KopsExampleRole"))
 		}

pkg/apis/kops/validation/validation.go

@@ -23,21 +23,21 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/blang/semver/v4"
 	"golang.org/x/net/ipv4"
 	"golang.org/x/net/ipv6"
-	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/kops/pkg/featureflag"
-	"k8s.io/kops/upup/pkg/fi"
-
 	"k8s.io/apimachinery/pkg/api/validation"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	utilnet "k8s.io/apimachinery/pkg/util/net"
 	"k8s.io/apimachinery/pkg/util/sets"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/pkg/featureflag"
 	"k8s.io/kops/pkg/model/components"
 	"k8s.io/kops/pkg/model/iam"
+	"k8s.io/kops/upup/pkg/fi"
 )
 
 func newValidateCluster(cluster *kops.Cluster) field.ErrorList {
@@ -158,12 +158,18 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 		allErrs = append(allErrs, validateNodeTerminationHandler(c, spec.NodeTerminationHandler, fieldPath.Child("nodeTerminationHandler"))...)
 	}
 
-	// IAM additionalPolicies
+	// IAM additional policies
 	if spec.AdditionalPolicies != nil {
 		for k, v := range *spec.AdditionalPolicies {
 			allErrs = append(allErrs, validateAdditionalPolicy(k, v, fieldPath.Child("additionalPolicies"))...)
 		}
 	}
+	// IAM external policies
+	if spec.ExternalPolicies != nil {
+		for k, v := range *spec.ExternalPolicies {
+			allErrs = append(allErrs, validateExternalPolicies(k, v, fieldPath.Child("externalPolicies"))...)
+		}
+	}
 
 	// EtcdClusters
 	{
@@ -795,31 +801,51 @@ func validateNetworkingGCE(c *kops.ClusterSpec, v *kops.GCENetworkingSpec, fldPa
 }
 
 func validateAdditionalPolicy(role string, policy string, fldPath *field.Path) field.ErrorList {
-	errs := field.ErrorList{}
+	allErrs := field.ErrorList{}
 
 	var valid []string
 	for _, r := range kops.AllInstanceGroupRoles {
 		valid = append(valid, strings.ToLower(string(r)))
 	}
-	errs = append(errs, IsValidValue(fldPath, &role, valid)...)
+	allErrs = append(allErrs, IsValidValue(fldPath, &role, valid)...)
 
 	statements, err := iam.ParseStatements(policy)
 	if err != nil {
-		errs = append(errs, field.Invalid(fldPath.Key(role), policy, "policy was not valid JSON: "+err.Error()))
+		allErrs = append(allErrs, field.Invalid(fldPath.Key(role), policy, "policy was not valid JSON: "+err.Error()))
 	}
 
 	// Trivial validation of policy, mostly to make sure it isn't some other random object
 	for i, statement := range statements {
 		fldEffect := fldPath.Key(role).Index(i).Child("Effect")
 		if statement.Effect == "" {
-			errs = append(errs, field.Required(fldEffect, "Effect must be specified for IAM policy"))
+			allErrs = append(allErrs, field.Required(fldEffect, "Effect must be specified for IAM policy"))
 		} else {
 			value := string(statement.Effect)
-			errs = append(errs, IsValidValue(fldEffect, &value, []string{"Allow", "Deny"})...)
+			allErrs = append(allErrs, IsValidValue(fldEffect, &value, []string{"Allow", "Deny"})...)
+		}
+	}
+
+	return allErrs
+}
+
+func validateExternalPolicies(role string, policies []string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	var valid []string
+	for _, r := range kops.AllInstanceGroupRoles {
+		valid = append(valid, strings.ToLower(string(r)))
+	}
+	allErrs = append(allErrs, IsValidValue(fldPath, &role, valid)...)
+
+	for _, policy := range policies {
+		parsedARN, err := arn.Parse(policy)
+		if err != nil || !strings.HasPrefix(parsedARN.Resource, "policy/") {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child(role), policy,
+				"Policy must be a valid AWS ARN such as arn:aws:iam::123456789012:policy/KopsExamplePolicy"))
 		}
 	}
 
-	return errs
+	return allErrs
 }
 
 func validateEtcdClusterSpec(spec kops.EtcdClusterSpec, c *kops.Cluster, fieldPath *field.Path) field.ErrorList {

pkg/resources/aws/aws.go

@@ -575,13 +575,13 @@ func DeleteVolume(cloud fi.Cloud, r *resources.Resource) error {
 	}
 	_, err := c.EC2().DeleteVolume(request)
 	if err != nil {
-		if IsDependencyViolation(err) {
-			return err
-		}
 		if awsup.AWSErrorCode(err) == "InvalidVolume.NotFound" {
-			// Concurrently deleted
+			klog.V(2).Infof("Got InvalidVolume.NotFound error deleting Volume %q; will treat as already-deleted", id)
 			return nil
 		}
+		if IsDependencyViolation(err) {
+			return err
+		}
 		return fmt.Errorf("error deleting Volume %q: %v", id, err)
 	}
 	return nil
@@ -1732,7 +1732,7 @@ func DeleteElasticIP(cloud fi.Cloud, t *resources.Resource) error {
 	_, err := c.EC2().ReleaseAddress(request)
 	if err != nil {
 		if awsup.AWSErrorCode(err) == "InvalidAllocationID.NotFound" {
-			klog.V(2).Infof("Got InvalidAllocationID.NotFound error describing ElasticIP %q; will treat as already-deleted", id)
+			klog.V(2).Infof("Got InvalidAllocationID.NotFound error deleting ElasticIP %q; will treat as already-deleted", id)
 			return nil
 		}
 

pkg/resources/aws/errors.go

@@ -26,7 +26,7 @@ func IsDependencyViolation(err error) bool {
 	switch code {
 	case "":
 		return false
-	case "DependencyViolation", "VolumeInUse", "InvalidIPAddress.InUse":
+	case "AuthFailure", "DependencyViolation", "InvalidIPAddress.InUse", "VolumeInUse", "ResourceInUse":
 		return true
 	default:
 		klog.Infof("unexpected aws error code: %q", code)

tests/integration/update_cluster/externalpolicies/in-v1alpha2.yaml

@@ -50,11 +50,11 @@ spec:
     nodes: public
   externalPolicies:
     node:
-      - aws:arn:iam:123456789000:policy:test-policy
+      - arn:aws:iam::123456789000:policy/test-policy
     master:
-      - aws:arn:iam:123456789000:policy:test-policy
+      - arn:aws:iam::123456789000:policy/test-policy
     bastion:
-      - aws:arn:iam:123456789000:policy:test-policy
+      - arn:aws:iam::123456789000:policy/test-policy
   subnets:
   - cidr: 172.20.32.0/19
     name: us-test-1a

tests/integration/update_cluster/externalpolicies/kubernetes.tf

@@ -268,13 +268,13 @@ resource "aws_iam_instance_profile" "nodes-externalpolicies-example-com" {
   role = aws_iam_role.nodes-externalpolicies-example-com.name
 }
 
-resource "aws_iam_role_policy_attachment" "master-policyoverride-1178482798" {
-  policy_arn = "aws:arn:iam:123456789000:policy:test-policy"
+resource "aws_iam_role_policy_attachment" "master-policyoverride-1242070525" {
+  policy_arn = "arn:aws:iam::123456789000:policy/test-policy"
   role       = aws_iam_role.masters-externalpolicies-example-com.name
 }
 
-resource "aws_iam_role_policy_attachment" "node-policyoverride-1178482798" {
-  policy_arn = "aws:arn:iam:123456789000:policy:test-policy"
+resource "aws_iam_role_policy_attachment" "node-policyoverride-1242070525" {
+  policy_arn = "arn:aws:iam::123456789000:policy/test-policy"
   role       = aws_iam_role.nodes-externalpolicies-example-com.name
 }
 

upup/pkg/fi/dryrun_target.go

@@ -222,6 +222,7 @@ func (t *DryRunTarget) PrintReport(taskMap map[string]Task, out io.Writer) error
 					fmt.Fprintf(b, "   internal consistency error!\n")
 					fmt.Fprintf(b, "    actual: %+v\n", r.a)
 					fmt.Fprintf(b, "    expect: %+v\n", r.e)
+					fmt.Fprintf(b, "    change: %+v\n", r.changes)
 					continue
 				}