Build IAM policy in code

Easier to get right than relying on string manipulation, but we're still
doing the same policies, with the improvements as done by @weargoogles.

upup/models/cloudup/_aws/resources/iam/kubernetes-master-policy.json.template

@@ -1,44 +1 @@
-{
+{{ IAMMasterPolicy }}
-  "Version": "2012-10-17",
-  "Statement": [
-    {
-      "Effect": "Allow",
-      "Action": ["ec2:*"],
-      "Resource": ["*"]
-    },
-    {
-      "Effect": "Allow",
-      "Action": ["route53:*"],
-      "Resource": ["*"]
-    },
-    {
-      "Effect": "Allow",
-      "Action": ["elasticloadbalancing:*"],
-      "Resource": ["*"]
-    }
-{{ if .MasterPermissions.S3Buckets }}
-    ,
-    {
-      "Effect": "Allow",
-      "Action": "s3:*",
-      "Resource": [
-        {{ range $i, $b := .MasterPermissions.S3Buckets }}
-        {{if $i}},{{end}}
-        "{{ IAMPrefix }}:s3:::{{ $b }}/{{ ClusterName }}",
-        "{{ IAMPrefix }}:s3:::{{ $b }}/{{ ClusterName }}/*"
-        {{ end }}
-      ]
-    },
-    {
-      "Effect": "Allow",
-      "Action": [ "s3:GetBucketLocation", "s3:ListBucket" ],
-      "Resource": [
-        {{ range $i, $b := .MasterPermissions.S3Buckets }}
-        {{if $i}},{{end}}
-        "{{ IAMPrefix }}:s3:::{{ $b }}"
-        {{ end }}
-      ]
-    }
-{{ end }}
-  ]
-}

upup/models/cloudup/_aws/resources/iam/kubernetes-node-policy.json.template

@@ -1,62 +1 @@
-{
+{{ IAMNodePolicy }}
-  "Version": "2012-10-17",
-  "Statement": [
-    {
-      "Effect": "Allow",
-      "Action": "ec2:Describe*",
-      "Resource": "*"
-    },
-    {
-      "Effect": "Allow",
-      "Action": "ec2:AttachVolume",
-      "Resource": "*"
-    },
-    {
-      "Effect": "Allow",
-      "Action": "ec2:DetachVolume",
-      "Resource": "*"
-    },
-    {
-      "Effect": "Allow",
-      "Action": ["route53:*"],
-      "Resource": ["*"]
-    },
-    {
-      "Effect": "Allow",
-      "Action": [
-        "ecr:GetAuthorizationToken",
-        "ecr:BatchCheckLayerAvailability",
-        "ecr:GetDownloadUrlForLayer",
-        "ecr:GetRepositoryPolicy",
-        "ecr:DescribeRepositories",
-        "ecr:ListImages",
-        "ecr:BatchGetImage"
-      ],
-      "Resource": "*"
-    }
-{{ if .NodePermissions.S3Buckets }}
-    ,
-    {
-      "Effect": "Allow",
-      "Action": "s3:*",
-      "Resource": [
-        {{ range $i, $b := .NodePermissions.S3Buckets }}
-        {{if $i}},{{end}}
-        "{{ IAMPrefix }}:s3:::{{ $b }}/{{ ClusterName }}",
-        "{{ IAMPrefix }}:s3:::{{ $b }}/{{ ClusterName }}/*"
-        {{ end }}
-      ]
-    },
-    {
-      "Effect": "Allow",
-      "Action": [ "s3:GetBucketLocation", "s3:ListBucket" ],
-      "Resource": [
-        {{ range $i, $b := .NodePermissions.S3Buckets }}
-        {{if $i}},{{end}}
-        "{{ IAMPrefix }}:s3:::{{ $b }}"
-        {{ end }}
-      ]
-    }
-{{ end }}
-  ]
-}

upup/pkg/api/cluster.go

@@ -37,11 +37,6 @@ type ClusterSpec struct {
 	// Project is the cloud project we should use, required on GCE
 	Project string `json:"project,omitempty"`
 
-	// MasterPermissions contains the IAM permissions for the masters
-	MasterPermissions *CloudPermissions `json:"masterPermissions,omitempty"`
-	// NodePermissions contains the IAM permissions for the nodes
-	NodePermissions *CloudPermissions `json:"nodePermissions,omitempty"`
-
 	// MasterPublicName is the external DNS name for the master nodes
 	MasterPublicName string `json:"masterPublicName,omitempty"`
 	// MasterInternalName is the internal DNS name for the master nodes
@@ -429,59 +424,3 @@ func (z *ClusterZoneSpec) assignCIDR(c *Cluster) (string, error) {
 func (c *Cluster) SharedVPC() bool {
 	return c.Spec.NetworkID != ""
 }
-
-// CloudPermissions holds IAM-style permissions
-type CloudPermissions struct {
-	Permissions []*CloudPermission `json:"permissions,omitempty"`
-}
-
-// CloudPermission holds a single IAM-style permission
-type CloudPermission struct {
-	Resource string `json:"resource,omitempty"`
-}
-
-// AddS3Bucket adds a bucket if it does not already exist
-func (p *CloudPermissions) AddS3Bucket(bucket string) {
-	for _, p := range p.Permissions {
-		if p.Resource == "s3://"+bucket {
-			return
-		}
-	}
-
-	p.Permissions = append(p.Permissions, &CloudPermission{
-		Resource: "s3://" + bucket,
-	})
-}
-
-// S3Buckets returns each of the S3 buckets in the permission
-// TODO: Replace with something generic (probably we should just generate the permission)
-func (p *CloudPermissions) S3Buckets() []string {
-	var buckets []string
-	for _, p := range p.Permissions {
-		if strings.HasPrefix(p.Resource, "s3://") {
-			buckets = append(buckets, strings.TrimPrefix(p.Resource, "s3://"))
-		}
-	}
-
-	return buckets
-}
-
-//
-//// findImage finds the default image
-//func (c*NodeSetConfig) resolveImage() error {
-//	cloud.(*awsup.AWSCloud).ResolveImage()
-//
-//	if n.Image == "" {
-//		if defaultImage == "" {
-//			image, err := c.determineImage()
-//			if err != nil {
-//				return err
-//			}
-//			defaultImage = image
-//		}
-//		n.Image = defaultImage
-//	}
-//
-//
-//	return nil
-//}

upup/pkg/api/registry.go

@@ -110,12 +110,20 @@ func (r *ClusterRegistry) ReadCompletedConfig(clusterName string) (*Cluster, err
 }
 
 func (r *ClusterRegistry) ConfigurationPath(clusterName string) (vfs.Path, error) {
+	basePath, err := r.ClusterBase(clusterName)
+	if err != nil {
+		return nil, err
+	}
+	return basePath.Join(PathClusterCompleted), nil
+}
+
+func (r *ClusterRegistry) ClusterBase(clusterName string) (vfs.Path, error) {
 	if clusterName == "" {
 		return nil, fmt.Errorf("clusterName is required")
 	}
 	stateStore := r.stateStore(clusterName)
 
-	return stateStore.VFSPath().Join(PathClusterCompleted), nil
+	return stateStore.VFSPath(), nil
 }
 
 func (r *ClusterRegistry) Create(g *Cluster) error {

upup/pkg/fi/cloudup/iam_builder.go

@@ -0,0 +1,194 @@
+package cloudup
+
+import (
+	"encoding/json"
+	"fmt"
+	"github.com/golang/glog"
+	"k8s.io/kops/upup/pkg/api"
+	"k8s.io/kops/upup/pkg/fi/vfs"
+	"strings"
+)
+
+const IAMPolicyDefaultVersion = "2012-10-17"
+
+type IAMPolicy struct {
+	Version   string
+	Statement []*IAMStatement
+}
+
+func (p *IAMPolicy) AsJSON() (string, error) {
+	j, err := json.MarshalIndent(p, "", "  ")
+	if err != nil {
+		return "", fmt.Errorf("error marshaling policy to JSON: %v", err)
+	}
+	return string(j), nil
+}
+
+type IAMStatementEffect string
+
+const IAMStatementEffectAllow IAMStatementEffect = "Allow"
+
+type IAMStatement struct {
+	Effect   IAMStatementEffect
+	Action   []string
+	Resource []string
+}
+
+type IAMPolicyBuilder struct {
+	Cluster *api.Cluster
+	Role    api.InstanceGroupRole
+	Region  string
+}
+
+func (b *IAMPolicyBuilder) BuildAWSIAMPolicy() (*IAMPolicy, error) {
+	iamPrefix := b.IAMPrefix()
+
+	p := &IAMPolicy{
+		Version: IAMPolicyDefaultVersion,
+	}
+
+	if b.Role == api.InstanceGroupRoleNode {
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect:   IAMStatementEffectAllow,
+			Action:   []string{"ec2:Describe*"},
+			Resource: []string{"*"},
+		})
+
+		// No longer needed in 1.3
+		//p.Statement = append(p.Statement, &IAMStatement{
+		//	Effect: IAMStatementEffectAllow,
+		//	Action: []string{ "ec2:AttachVolume" },
+		//	Resource: []string{"*"},
+		//})
+		//p.Statement = append(p.Statement, &IAMStatement{
+		//	Effect: IAMStatementEffectAllow,
+		//	Action: []string{ "ec2:DetachVolume" },
+		//	Resource: []string{"*"},
+		//})
+
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect:   IAMStatementEffectAllow,
+			Action:   []string{"route53:*"},
+			Resource: []string{"*"},
+		})
+
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect: IAMStatementEffectAllow,
+			Action: []string{
+				"ecr:GetAuthorizationToken",
+				"ecr:BatchCheckLayerAvailability",
+				"ecr:GetDownloadUrlForLayer",
+				"ecr:GetRepositoryPolicy",
+				"ecr:DescribeRepositories",
+				"ecr:ListImages",
+				"ecr:BatchGetImage",
+			},
+			Resource: []string{"*"},
+		})
+	}
+
+	if b.Role == api.InstanceGroupRoleMaster {
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect:   IAMStatementEffectAllow,
+			Action:   []string{"ec2:*"},
+			Resource: []string{"*"},
+		})
+
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect:   IAMStatementEffectAllow,
+			Action:   []string{"route53:*"},
+			Resource: []string{"*"},
+		})
+
+		p.Statement = append(p.Statement, &IAMStatement{
+			Effect:   IAMStatementEffectAllow,
+			Action:   []string{"elasticloadbalancing:*"},
+			Resource: []string{"*"},
+		})
+	}
+
+	// For S3 IAM permissions, we grant permissions to subtrees.  So find the parents;
+	// we don't need to grant mypath and mypath/child.
+	var roots []string
+	{
+		var locations []string
+
+		for _, p := range []string{
+			b.Cluster.Spec.KeyStore,
+			b.Cluster.Spec.SecretStore,
+			b.Cluster.Spec.ConfigStore,
+		} {
+			if p == "" {
+				continue
+			}
+
+			if !strings.HasSuffix(p, "/") {
+				p = p + "/"
+			}
+			locations = append(locations, p)
+		}
+
+		for i, l := range locations {
+			isTopLevel := true
+			for j := range locations {
+				if i == j {
+					continue
+				}
+				if strings.HasPrefix(l, locations[j]) {
+					glog.V(4).Infof("Ignoring location %q because found parent %q", l, locations[j])
+					isTopLevel = false
+				}
+			}
+			if isTopLevel {
+				glog.V(4).Infof("Found root location %q", l)
+				roots = append(roots, l)
+			}
+		}
+	}
+
+	for _, root := range roots {
+		vfsPath, err := vfs.Context.BuildVfsPath(root)
+		if err != nil {
+			return nil, fmt.Errorf("cannot parse VFS path %q: %v", root, err)
+		}
+
+		if s3Path, ok := vfsPath.(*vfs.S3Path); ok {
+			// Note that the config store may itself be a subdirectory of a bucket
+			iamS3Path := s3Path.Bucket() + "/" + s3Path.Key()
+			iamS3Path = strings.TrimSuffix(iamS3Path, "/")
+
+			p.Statement = append(p.Statement, &IAMStatement{
+				Effect: IAMStatementEffectAllow,
+				Action: []string{"s3:*"},
+				Resource: []string{
+					iamPrefix + ":s3:::" + iamS3Path,
+					iamPrefix + ":s3:::" + iamS3Path + "/*",
+				},
+			})
+
+			p.Statement = append(p.Statement, &IAMStatement{
+				Effect: IAMStatementEffectAllow,
+				Action: []string{"s3:GetBucketLocation", "s3:ListBucket"},
+				Resource: []string{
+					iamPrefix + ":s3:::" + s3Path.Bucket(),
+				},
+			})
+		} else {
+			// We could implement this approach, but it seems better to get all clouds using cluster-readable storage
+			return nil, fmt.Errorf("path is not cluster readable: %v", root)
+		}
+	}
+
+	return p, nil
+}
+
+// IAMPrefix returns the prefix for AWS ARNs in the current region, for use with IAM
+// it is arn:aws everywhere but in cn-north, where it is arn:aws-cn
+func (b *IAMPolicyBuilder) IAMPrefix() string {
+	switch b.Region {
+	case "cn-north-1":
+		return "arn:aws-cn"
+	default:
+		return "arn:aws"
+	}
+}

upup/pkg/fi/cloudup/populate_cluster_spec.go

@@ -173,16 +173,6 @@ func (c *populateClusterSpec) run() error {
 	if vfs.IsClusterReadable(secretStore.VFSPath()) {
 		vfsPath := secretStore.VFSPath()
 		cluster.Spec.SecretStore = vfsPath.Path()
-		if s3Path, ok := vfsPath.(*vfs.S3Path); ok {
-			if cluster.Spec.MasterPermissions == nil {
-				cluster.Spec.MasterPermissions = &api.CloudPermissions{}
-			}
-			cluster.Spec.MasterPermissions.AddS3Bucket(s3Path.Bucket())
-			if cluster.Spec.NodePermissions == nil {
-				cluster.Spec.NodePermissions = &api.CloudPermissions{}
-			}
-			cluster.Spec.NodePermissions.AddS3Bucket(s3Path.Bucket())
-		}
 	} else {
 		// We could implement this approach, but it seems better to get all clouds using cluster-readable storage
 		return fmt.Errorf("secrets path is not cluster readable: %v", secretStore.VFSPath())
@@ -191,29 +181,20 @@ func (c *populateClusterSpec) run() error {
 	if vfs.IsClusterReadable(keyStore.VFSPath()) {
 		vfsPath := keyStore.VFSPath()
 		cluster.Spec.KeyStore = vfsPath.Path()
-		if s3Path, ok := vfsPath.(*vfs.S3Path); ok {
-			if cluster.Spec.MasterPermissions == nil {
-				cluster.Spec.MasterPermissions = &api.CloudPermissions{}
-			}
-			cluster.Spec.MasterPermissions.AddS3Bucket(s3Path.Bucket())
-			if cluster.Spec.NodePermissions == nil {
-				cluster.Spec.NodePermissions = &api.CloudPermissions{}
-			}
-			cluster.Spec.NodePermissions.AddS3Bucket(s3Path.Bucket())
-		}
 	} else {
 		// We could implement this approach, but it seems better to get all clouds using cluster-readable storage
 		return fmt.Errorf("keyStore path is not cluster readable: %v", keyStore.VFSPath())
 	}
 
-	configPath, err := c.ClusterRegistry.ConfigurationPath(cluster.Name)
+	clusterBasePath, err := c.ClusterRegistry.ClusterBase(cluster.Name)
 	if err != nil {
 		return err
 	}
-	if vfs.IsClusterReadable(configPath) {
+	if vfs.IsClusterReadable(clusterBasePath) {
-		cluster.Spec.ConfigStore = configPath.Path()
+		cluster.Spec.ConfigStore = clusterBasePath.Path()
 	} else {
-		// We do support this...
+		// We could implement this approach, but it seems better to get all clouds using cluster-readable storage
+		return fmt.Errorf("ClusterBase path is not cluster readable: %v", clusterBasePath)
 	}
 
 	// Normalize k8s version

upup/pkg/fi/cloudup/template_functions.go

@@ -72,15 +72,18 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap) {
 		return tf.cluster.Name
 	}
 
-	dest["HasTag"] = func(tag string) bool {
+	dest["HasTag"] = tf.HasTag
-		_, found := tf.tags[tag]
-		return found
-	}
 
-	dest["IAMPrefix"] = tf.IAMPrefix
 	dest["IAMServiceEC2"] = tf.IAMServiceEC2
 
 	dest["Image"] = tf.Image
+
+	dest["IAMMasterPolicy"] = func() (string, error) {
+		return tf.buildAWSIAMPolicy(api.InstanceGroupRoleMaster)
+	}
+	dest["IAMNodePolicy"] = func() (string, error) {
+		return tf.buildAWSIAMPolicy(api.InstanceGroupRoleNode)
+	}
 }
 
 func (tf *TemplateFunctions) EtcdClusterMemberTags(etcd *api.EtcdClusterSpec, m *api.EtcdMemberSpec) map[string]string {
@@ -130,17 +133,6 @@ func (tf *TemplateFunctions) IAMServiceEC2() string {
 	}
 }
 
-// IAMPrefix returns the prefix for AWS ARNs in the current region, for use with IAM
-// it is arn:aws everywhere but in cn-north, where it is arn:aws-cn
-func (tf *TemplateFunctions) IAMPrefix() string {
-	switch tf.region {
-	case "cn-north-1":
-		return "arn:aws-cn"
-	default:
-		return "arn:aws"
-	}
-}
-
 // Image returns the docker image name for the specified component
 func (tf *TemplateFunctions) Image(component string) (string, error) {
 	if component == "kube-dns" {
@@ -167,3 +159,28 @@ func (tf *TemplateFunctions) Image(component string) (string, error) {
 
 	return "gcr.io/google_containers/" + component + ":" + tag, nil
 }
+
+// HasTag returns true if the specified tag is set
+func (tf *TemplateFunctions) HasTag(tag string) bool {
+	_, found := tf.tags[tag]
+	return found
+}
+
+// buildAWSIAMPolicy produces the AWS IAM policy for the given role
+func (tf *TemplateFunctions) buildAWSIAMPolicy(role api.InstanceGroupRole) (string, error) {
+	b := &IAMPolicyBuilder{
+		Cluster: tf.cluster,
+		Role:    role,
+		Region:  tf.region,
+	}
+
+	policy, err := b.BuildAWSIAMPolicy()
+	if err != nil {
+		return "", fmt.Errorf("error building IAM policy: %v", err)
+	}
+	json, err := policy.AsJSON()
+	if err != nil {
+		return "", fmt.Errorf("error building IAM policy: %v", err)
+	}
+	return json, nil
+}

upup/pkg/fi/vfs/s3fs.go

@@ -45,6 +45,10 @@ func (p *S3Path) Bucket() string {
 	return p.bucket
 }
 
+func (p *S3Path) Key() string {
+	return p.key
+}
+
 func (p *S3Path) String() string {
 	return p.Path()
 }