From 52b37619f2e16c7d25ed16a0bfd2d5dfa90f0d4f Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Wed, 11 Nov 2020 09:03:56 +0200 Subject: [PATCH 1/3] Validate external IAM policies --- pkg/apis/kops/validation/instancegroup.go | 2 +- pkg/apis/kops/validation/validation.go | 48 ++++++++++++++----- .../externalpolicies/in-v1alpha2.yaml | 6 +-- .../externalpolicies/kubernetes.tf | 8 ++-- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/pkg/apis/kops/validation/instancegroup.go b/pkg/apis/kops/validation/instancegroup.go index 148cc96770eb3..ecdf50629857e 100644 --- a/pkg/apis/kops/validation/instancegroup.go +++ b/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")) } diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 353ce098b1956..2c13bde20afb6 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/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 { @@ -798,31 +804,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 { diff --git a/tests/integration/update_cluster/externalpolicies/in-v1alpha2.yaml b/tests/integration/update_cluster/externalpolicies/in-v1alpha2.yaml index 915feceec9565..fda0e8351feb2 100644 --- a/tests/integration/update_cluster/externalpolicies/in-v1alpha2.yaml +++ b/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 diff --git a/tests/integration/update_cluster/externalpolicies/kubernetes.tf b/tests/integration/update_cluster/externalpolicies/kubernetes.tf index 03ddf6067dd05..720e2a26afe9c 100644 --- a/tests/integration/update_cluster/externalpolicies/kubernetes.tf +++ b/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 } From d5e3726a7b1abba4a982852dc89e6eb677260e17 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Wed, 11 Nov 2020 11:12:18 +0200 Subject: [PATCH 2/3] Print changes also for consistency errors --- upup/pkg/fi/dryrun_target.go | 1 + 1 file changed, 1 insertion(+) diff --git a/upup/pkg/fi/dryrun_target.go b/upup/pkg/fi/dryrun_target.go index 89f9b18a64f96..79ec1d9c71ecc 100644 --- a/upup/pkg/fi/dryrun_target.go +++ b/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 } From 025d099fcc25cfd69f8a48a196a6137ded4ab9a9 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Wed, 11 Nov 2020 11:19:13 +0200 Subject: [PATCH 3/3] Handle cluster cleanup more gracefully --- pkg/resources/aws/aws.go | 10 +++++----- pkg/resources/aws/errors.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/resources/aws/aws.go b/pkg/resources/aws/aws.go index 00c1b28dd4c21..e1b9ddb02e7a8 100644 --- a/pkg/resources/aws/aws.go +++ b/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 @@ -1744,7 +1744,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 } diff --git a/pkg/resources/aws/errors.go b/pkg/resources/aws/errors.go index bb7cf6ed9d9b7..533a506c1f324 100644 --- a/pkg/resources/aws/errors.go +++ b/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)