diff --git a/pkg/model/iam/BUILD.bazel b/pkg/model/iam/BUILD.bazel index 1b536eb634..96e657ca95 100644 --- a/pkg/model/iam/BUILD.bazel +++ b/pkg/model/iam/BUILD.bazel @@ -26,7 +26,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/apis/kops:go_default_library", - "//pkg/diff:go_default_library", + "//pkg/testutils/golden:go_default_library", "//pkg/util/stringorslice:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", ], diff --git a/pkg/model/iam/iam_builder.go b/pkg/model/iam/iam_builder.go index fc5ebab2e0..3d5e8c5932 100644 --- a/pkg/model/iam/iam_builder.go +++ b/pkg/model/iam/iam_builder.go @@ -312,6 +312,8 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) { sort.Strings(roots) + s3Buckets := sets.NewString() + for _, root := range roots { vfsPath, err := vfs.Context.BuildVfsPath(root) if err != nil { @@ -322,18 +324,7 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) { iamS3Path := s3Path.Bucket() + "/" + s3Path.Key() iamS3Path = strings.TrimSuffix(iamS3Path, "/") - p.Statement = append(p.Statement, &Statement{ - Effect: StatementEffectAllow, - Action: stringorslice.Of( - "s3:GetBucketLocation", - "s3:GetEncryptionConfiguration", - "s3:ListBucket", - "s3:ListBucketVersions", - ), - Resource: stringorslice.Slice([]string{ - strings.Join([]string{b.IAMPrefix(), ":s3:::", s3Path.Bucket()}, ""), - }), - }) + s3Buckets.Insert(s3Path.Bucket()) if b.Cluster.Spec.IAM.Legacy { p.Statement = append(p.Statement, &Statement{ @@ -449,11 +440,29 @@ func (b *PolicyBuilder) AddS3Permissions(p *Policy) (*Policy, error) { strings.Join([]string{b.IAMPrefix(), ":s3:::", iamS3Path, "/*"}, ""), ), }) + + s3Buckets.Insert(s3Path.Bucket()) } else { klog.Warningf("unknown writeable path, can't apply IAM policy: %q", vfsPath) } } + // We need some permissions on the buckets themselves + for _, s3Bucket := range s3Buckets.List() { + p.Statement = append(p.Statement, &Statement{ + Effect: StatementEffectAllow, + Action: stringorslice.Of( + "s3:GetBucketLocation", + "s3:GetEncryptionConfiguration", + "s3:ListBucket", + "s3:ListBucketVersions", + ), + Resource: stringorslice.Slice([]string{ + strings.Join([]string{b.IAMPrefix(), ":s3:::", s3Bucket}, ""), + }), + }) + } + return p, nil } diff --git a/pkg/model/iam/iam_builder_test.go b/pkg/model/iam/iam_builder_test.go index 02dadc47a8..2ea84dc2b7 100644 --- a/pkg/model/iam/iam_builder_test.go +++ b/pkg/model/iam/iam_builder_test.go @@ -18,14 +18,12 @@ package iam import ( "encoding/json" - "io/ioutil" - "strings" "testing" "github.com/aws/aws-sdk-go/aws" "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/diff" + "k8s.io/kops/pkg/testutils/golden" "k8s.io/kops/pkg/util/stringorslice" ) @@ -185,19 +183,7 @@ func TestPolicyGeneration(t *testing.T) { t.Errorf("case %d failed to convert generated IAM Policy to JSON. Error: %v", i, err) continue } - actualPolicy = strings.TrimSpace(actualPolicy) - expectedPolicyBytes, err := ioutil.ReadFile(x.Policy) - if err != nil { - t.Fatalf("unexpected error reading IAM Policy from file %q: %v", x.Policy, err) - } - expectedPolicy := strings.TrimSpace(string(expectedPolicyBytes)) - - if expectedPolicy != actualPolicy { - diffString := diff.FormatDiff(expectedPolicy, actualPolicy) - t.Logf("diff:\n%s\n", diffString) - t.Errorf("case %d failed, policy output differed from expected (%s).", i, x.Policy) - continue - } + golden.AssertMatchesFile(t, actualPolicy, x.Policy) } } diff --git a/pkg/model/iam/tests/iam_builder_master_legacy.json b/pkg/model/iam/tests/iam_builder_master_legacy.json index 67d3bf5631..ac383c1488 100644 --- a/pkg/model/iam/tests/iam_builder_master_legacy.json +++ b/pkg/model/iam/tests/iam_builder_master_legacy.json @@ -45,6 +45,13 @@ "*" ] }, + { + "Effect": "Allow", + "Action": [ + "s3:*" + ], + "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" + }, { "Effect": "Allow", "Action": [ @@ -57,13 +64,6 @@ "arn:aws:s3:::kops-tests" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:*" - ], - "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" - }, { "Effect": "Allow", "Action": [ diff --git a/pkg/model/iam/tests/iam_builder_master_strict.json b/pkg/model/iam/tests/iam_builder_master_strict.json index ad048a8ced..11db1135eb 100644 --- a/pkg/model/iam/tests/iam_builder_master_strict.json +++ b/pkg/model/iam/tests/iam_builder_master_strict.json @@ -137,6 +137,13 @@ "*" ] }, + { + "Effect": "Allow", + "Action": [ + "s3:Get*" + ], + "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" + }, { "Effect": "Allow", "Action": [ @@ -149,13 +156,6 @@ "arn:aws:s3:::kops-tests" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:Get*" - ], - "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" - }, { "Effect": "Allow", "Action": [ diff --git a/pkg/model/iam/tests/iam_builder_master_strict_ecr.json b/pkg/model/iam/tests/iam_builder_master_strict_ecr.json index cb9b48a0f4..ecd65ad5cb 100644 --- a/pkg/model/iam/tests/iam_builder_master_strict_ecr.json +++ b/pkg/model/iam/tests/iam_builder_master_strict_ecr.json @@ -1,4 +1,4 @@ - { +{ "Version": "2012-10-17", "Statement": [ { @@ -137,6 +137,13 @@ "*" ] }, + { + "Effect": "Allow", + "Action": [ + "s3:Get*" + ], + "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" + }, { "Effect": "Allow", "Action": [ @@ -149,13 +156,6 @@ "arn:aws:s3:::kops-tests" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:Get*" - ], - "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" - }, { "Effect": "Allow", "Action": [ diff --git a/pkg/model/iam/tests/iam_builder_node_legacy.json b/pkg/model/iam/tests/iam_builder_node_legacy.json index a2b135f92e..6f08265ea2 100644 --- a/pkg/model/iam/tests/iam_builder_node_legacy.json +++ b/pkg/model/iam/tests/iam_builder_node_legacy.json @@ -11,6 +11,13 @@ "*" ] }, + { + "Effect": "Allow", + "Action": [ + "s3:*" + ], + "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" + }, { "Effect": "Allow", "Action": [ @@ -23,13 +30,6 @@ "arn:aws:s3:::kops-tests" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:*" - ], - "Resource": "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/*" - }, { "Effect": "Allow", "Action": [ diff --git a/pkg/model/iam/tests/iam_builder_node_strict.json b/pkg/model/iam/tests/iam_builder_node_strict.json index ba6a526c7e..98576e6e62 100644 --- a/pkg/model/iam/tests/iam_builder_node_strict.json +++ b/pkg/model/iam/tests/iam_builder_node_strict.json @@ -11,18 +11,6 @@ "*" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:GetBucketLocation", - "s3:GetEncryptionConfiguration", - "s3:ListBucket", - "s3:ListBucketVersions" - ], - "Resource": [ - "arn:aws:s3:::kops-tests" - ] - }, { "Effect": "Allow", "Action": [ @@ -39,6 +27,18 @@ "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/pki/ssh/*", "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/secrets/dockerconfig" ] + }, + { + "Effect": "Allow", + "Action": [ + "s3:GetBucketLocation", + "s3:GetEncryptionConfiguration", + "s3:ListBucket", + "s3:ListBucketVersions" + ], + "Resource": [ + "arn:aws:s3:::kops-tests" + ] } ] } diff --git a/pkg/model/iam/tests/iam_builder_node_strict_ecr.json b/pkg/model/iam/tests/iam_builder_node_strict_ecr.json index a7243e73ee..85c74aeb83 100644 --- a/pkg/model/iam/tests/iam_builder_node_strict_ecr.json +++ b/pkg/model/iam/tests/iam_builder_node_strict_ecr.json @@ -11,18 +11,6 @@ "*" ] }, - { - "Effect": "Allow", - "Action": [ - "s3:GetBucketLocation", - "s3:GetEncryptionConfiguration", - "s3:ListBucket", - "s3:ListBucketVersions" - ], - "Resource": [ - "arn:aws:s3:::kops-tests" - ] - }, { "Effect": "Allow", "Action": [ @@ -40,6 +28,18 @@ "arn:aws:s3:::kops-tests/iam-builder-test.k8s.local/secrets/dockerconfig" ] }, + { + "Effect": "Allow", + "Action": [ + "s3:GetBucketLocation", + "s3:GetEncryptionConfiguration", + "s3:ListBucket", + "s3:ListBucketVersions" + ], + "Resource": [ + "arn:aws:s3:::kops-tests" + ] + }, { "Effect": "Allow", "Action": [