Ensure we have IAM bucket permissions to other S3 buckets

If we are expected to write to other buckets, we need to have suitable
permissions to e.g. determine their location.

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",
     ],

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{
+			s3Buckets.Insert(s3Path.Bucket())
-				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()}, ""),
-				}),
-			})
 
 			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
 }
 

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)
+		golden.AssertMatchesFile(t, actualPolicy, 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
-		}
 	}
 }

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": [

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": [

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": [

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": [

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"
+      ]
     }
   ]
 }

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": [