fix make build; set IAM policy following read/modify/write (kubeflow#…

…2751)

* fix make build; set IAM policy following read/modify/write

* address comments

bootstrap/Dockerfile

@@ -5,6 +5,7 @@ ARG GOLANG_VERSION=1.11.5
 FROM golang:$GOLANG_VERSION as bootstrap_base
 
 RUN apt-get update
+RUN apt-get install -y unzip
 
 # We need gcloud to get gke credentials.
 RUN \
@@ -23,6 +24,7 @@ WORKDIR ${GOPATH}/src/github.com/kubeflow/kubeflow/bootstrap
 ENV GO111MODULE=on
 
 COPY . .
+RUN unzip -q -d /tmp hack/v2.zip
 RUN go mod download
 RUN go build -gcflags 'all=-N -l' -o bin/bootstrapper cmd/bootstrap/main.go
 

bootstrap/Makefile

@@ -76,7 +76,7 @@ v2/pkg/apis/apps/kfdef/v1alpha1/zz_generated.deepcopy.go: v2/pkg/apis/apps/kfdef
 
 deepcopy: $(GOPATH)/bin/deepcopy-gen config/zz_generated.deepcopy.go pkg/apis/apps/kfdef/v1alpha1/zz_generated.deepcopy.go v2/pkg/apis/apps/kfdef/v1alpha1/zz_generated.deepcopy.go 
 
-build-bootstrap: generate fmt vet
+build-bootstrap: /tmp/v2 deepcopy generate fmt vet
 	$(GO) build -gcflags 'all=-N -l' -o bin/bootstrapper cmd/bootstrap/main.go
 
 build-kfctl: /tmp/v2 deepcopy generate fmt vet

bootstrap/cmd/bootstrap/app/gcpUtils.go

@@ -142,13 +142,13 @@ func (s *ksServer) GetDeploymentStatus(ctx context.Context, req CreateRequest, d
 
 // Clear existing bindings for auto-generated service accounts of current deployment.
 // Those bindings could be leftover from previous actions.
-func GetClearServiceAccountpolicy(currentPolicy *cloudresourcemanager.Policy, req ApplyIamRequest) cloudresourcemanager.Policy {
+func ClearServiceAccountPolicy(currentPolicy *cloudresourcemanager.Policy, req ApplyIamRequest) {
 	serviceAccounts := map[string]bool{
 		fmt.Sprintf("serviceAccount:%v-admin@%v.iam.gserviceaccount.com", req.Cluster, req.Project): true,
 		fmt.Sprintf("serviceAccount:%v-user@%v.iam.gserviceaccount.com", req.Cluster, req.Project):  true,
 		fmt.Sprintf("serviceAccount:%v-vm@%v.iam.gserviceaccount.com", req.Cluster, req.Project):    true,
 	}
-	newPolicy := cloudresourcemanager.Policy{}
+	var newBindings []*cloudresourcemanager.Binding
 	for _, binding := range currentPolicy.Bindings {
 		newBinding := cloudresourcemanager.Binding{
 			Role: binding.Role,
@@ -160,9 +160,9 @@ func GetClearServiceAccountpolicy(currentPolicy *cloudresourcemanager.Policy, re
 				newBinding.Members = append(newBinding.Members, member)
 			}
 		}
-		newPolicy.Bindings = append(newPolicy.Bindings, &newBinding)
+		newBindings = append(newBindings, &newBinding)
 	}
-	return newPolicy
+	currentPolicy.Bindings = newBindings
 }
 
 func PrepareAccount(account string) string {
@@ -176,7 +176,7 @@ func PrepareAccount(account string) string {
 	}
 }
 
-func GetUpdatedPolicy(currentPolicy *cloudresourcemanager.Policy, iamConf *IamConf, req ApplyIamRequest) cloudresourcemanager.Policy {
+func UpdatePolicy(currentPolicy *cloudresourcemanager.Policy, iamConf *IamConf, req ApplyIamRequest){
 	// map from role to members.
 	policyMap := map[string]map[string]bool{}
 	for _, binding := range currentPolicy.Bindings {
@@ -212,7 +212,7 @@ func GetUpdatedPolicy(currentPolicy *cloudresourcemanager.Policy, iamConf *IamCo
 			}
 		}
 	}
-	newPolicy := cloudresourcemanager.Policy{}
+	var newBindings []*cloudresourcemanager.Binding
 	for role, memberSet := range policyMap {
 		binding := cloudresourcemanager.Binding{}
 		binding.Role = role
@@ -221,9 +221,9 @@ func GetUpdatedPolicy(currentPolicy *cloudresourcemanager.Policy, iamConf *IamCo
 				binding.Members = append(binding.Members, member)
 			}
 		}
-		newPolicy.Bindings = append(newPolicy.Bindings, &binding)
+		newBindings = append(newBindings, &binding)
 	}
-	return newPolicy
+	currentPolicy.Bindings = newBindings
 }
 
 func (s *ksServer) ApplyIamPolicy(ctx context.Context, req ApplyIamRequest) error {
@@ -254,6 +254,7 @@ func (s *ksServer) ApplyIamPolicy(ctx context.Context, req ApplyIamRequest) erro
 	exp.MaxInterval = 5 * time.Second
 	exp.MaxElapsedTime = time.Minute
 	exp.Reset()
+	// Remove bindings of target service accounts
 	err = backoff.Retry(func() error {
 		// Get current policy
 		saPolicy, err := resourceManager.Projects.GetIamPolicy(
@@ -265,32 +266,45 @@ func (s *ksServer) ApplyIamPolicy(ctx context.Context, req ApplyIamRequest) erro
 		}
 
 		// Force update iam bindings of service accounts
-		clearedPolicy := GetClearServiceAccountpolicy(saPolicy, req)
+		ClearServiceAccountPolicy(saPolicy, req)
 		_, err = resourceManager.Projects.SetIamPolicy(
 			req.Project,
 			&cloudresourcemanager.SetIamPolicyRequest{
-				Policy: &clearedPolicy,
+				Policy: saPolicy,
 			}).Do()
 		if err != nil {
 			log.Warningf("Cannot set refresh policy: %v", err)
 			return fmt.Errorf("Cannot set refresh policy: %v", err)
 		}
+		return nil
+	}, exp)
+	if err != nil {
+		return err
+	}
+	// Add new bindings to target service accounts
+	exp.Reset()
+	err = backoff.Retry(func() error {
+		// Get current policy
+		saPolicy, err := resourceManager.Projects.GetIamPolicy(
+			req.Project,
+			&cloudresourcemanager.GetIamPolicyRequest{}).Do()
+		if err != nil {
+			log.Warningf("Cannot get current policy: %v", err)
+			return fmt.Errorf("Cannot get current policy: %v", err)
+		}
 
 		// Get the updated policy and apply it.
-		newPolicy := GetUpdatedPolicy(saPolicy, &iamConf, req)
+		UpdatePolicy(saPolicy, &iamConf, req)
 		_, err = resourceManager.Projects.SetIamPolicy(
 			req.Project,
 			&cloudresourcemanager.SetIamPolicyRequest{
-				Policy: &newPolicy,
+				Policy: saPolicy,
 			}).Do()
 		if err != nil {
 			log.Warningf("Cannot set new policy: %v", err)
 			return fmt.Errorf("Cannot set new policy: %v", err)
 		}
 		return nil
 	}, exp)
-	if err != nil {
-		return err
-	}
 	return nil
 }

bootstrap/cmd/bootstrap/app/gcpUtils_test.go

@@ -22,16 +22,17 @@ import (
 )
 
 func TestGetUpdatedPolicy(t *testing.T) {
+	etags := "etagValueNeedPreserve"
 	tests := []struct {
 		// Name of the test.
 		name string
 
-		// Arguments for GetUpdatedPolicy function.
+		// Arguments for UpdatePolicy function.
 		currentPolicy *cloudresourcemanager.Policy
 		iamConf       *IamConf
 		req           ApplyIamRequest
 
-		// Check function that checks if the result from GetUpdatedPolicy is valid.
+		// Check function that checks if the result from UpdatePolicy is valid.
 		check func(cloudresourcemanager.Policy) error
 	}{
 		{
@@ -42,6 +43,7 @@ func TestGetUpdatedPolicy(t *testing.T) {
 						Role: "admin",
 					},
 				},
+				Etag: etags,
 			},
 			iamConf: &IamConf{},
 			req:     ApplyIamRequest{},
@@ -52,6 +54,9 @@ func TestGetUpdatedPolicy(t *testing.T) {
 				if policy.Bindings[0].Role != "admin" {
 					return fmt.Errorf("'admin' role wasn't added")
 				}
+				if policy.Etag != etags {
+					return fmt.Errorf("'Etag' role was deleted")
+				}
 				return nil
 			},
 		},
@@ -165,12 +170,12 @@ func TestGetUpdatedPolicy(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			policy := GetUpdatedPolicy(test.currentPolicy, test.iamConf, test.req)
+			UpdatePolicy(test.currentPolicy, test.iamConf, test.req)
 			if test.check == nil {
 				t.Errorf("no check implemented")
 				return
 			}
-			if err := test.check(policy); err != nil {
+			if err := test.check(*test.currentPolicy); err != nil {
 				t.Error(err)
 			}
 		})

bootstrap/cmd/bootstrap/app/ksServer.go

@@ -24,7 +24,7 @@ import (
 	"github.com/ksonnet/ksonnet/pkg/actions"
 	kApp "github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/ksonnet/ksonnet/pkg/client"
-	kstypes "github.com/kubeflow/kubeflow/bootstrap/pkg/apis/apps/ksonnet/v1alpha1"
+	kstypes "github.com/kubeflow/kubeflow/bootstrap/pkg/apis/apps/kfdef/v1alpha1"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
 	log "github.com/sirupsen/logrus"

bootstrap/cmd/bootstrap/app/server.go

@@ -26,7 +26,7 @@ import (
 
 	"github.com/ghodss/yaml"
 	"github.com/kubeflow/kubeflow/bootstrap/cmd/bootstrap/app/options"
-	kstypes "github.com/kubeflow/kubeflow/bootstrap/pkg/apis/apps/ksonnet/v1alpha1"
+	kstypes "github.com/kubeflow/kubeflow/bootstrap/pkg/apis/apps/kfdef/v1alpha1"
 	"github.com/kubeflow/kubeflow/bootstrap/version"
 	log "github.com/sirupsen/logrus"
 	"k8s.io/api/storage/v1"

bootstrap/go.mod

@@ -28,6 +28,7 @@ require (
 	github.com/fatih/camelcase v1.0.0 // indirect
 	github.com/ghodss/yaml v1.0.0
 	github.com/go-kit/kit v0.8.0
+	github.com/go-logfmt/logfmt v0.4.0 // indirect
 	github.com/go-openapi/jsonpointer v0.18.0
 	github.com/go-openapi/jsonreference v0.18.0
 	github.com/go-openapi/spec v0.18.0 // indirect

bootstrap/go.sum

@@ -127,7 +127,9 @@ github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeME
 github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q=
 github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8 h1:DujepqpGd1hyOd7aW59XpK7Qymp8iy83xq74fLr21is=
 github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q=
+github.com/go-kit/kit v0.8.0 h1:Wz+5lgoB0kkuqLEc6NVmwRknTKP6dTGbSqvhZtBI/j0=
 github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
+github.com/go-logfmt/logfmt v0.4.0 h1:MP4Eh7ZCb31lleYCFuwm0oe4/YGak+5l1vA2NOE80nA=
 github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V4qmtdjCk=
 github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas=
 github.com/go-logr/zapr v0.1.0/go.mod h1:tabnROwaDl0UNxkVeFRbY8bwB37GwRv0P8lg6aAiEnk=

bootstrap/pkg/kfapp/gcp/gcp.go

@@ -386,11 +386,17 @@ func (gcp *Gcp) updateDM(resources kftypes.ResourceEnum) error {
 	if iamPolicyErr != nil {
 		return fmt.Errorf("Read IAM policy YAML error: %v", iamPolicyErr)
 	}
-	clearedPolicy := utils.GetClearedIamPolicy(policy, iamPolicy)
-	if err := utils.SetIamPolicy(gcp.Spec.Project, clearedPolicy); err != nil {
+	utils.ClearIamPolicy(policy, iamPolicy)
+	if err := utils.SetIamPolicy(gcp.Spec.Project, policy); err != nil {
 		return fmt.Errorf("Set Cleared IamPolicy error: %v", err)
 	}
-	newPolicy, err := utils.RewriteIamPolicy(policy, iamPolicy)
+
+	// Need to read policy again as latest Etag changed.
+	newPolicy, policyErr := utils.GetIamPolicy(gcp.Spec.Project)
+	if policyErr != nil {
+		return fmt.Errorf("GetIamPolicy error: %v", policyErr)
+	}
+	utils.RewriteIamPolicy(newPolicy, iamPolicy)
 	if err := utils.SetIamPolicy(gcp.Spec.Project, newPolicy); err != nil {
 		return fmt.Errorf("Set New IamPolicy error: %v", err)
 	}

bootstrap/pkg/utils/iamutils.go

@@ -17,7 +17,6 @@ limitations under the License.
 package utils
 
 import (
-	"fmt"
 	"github.com/deckarep/golang-set"
 	"github.com/ghodss/yaml"
 	log "github.com/sirupsen/logrus"
@@ -70,8 +69,8 @@ func GetIamPolicy(project string) (*cloudresourcemanager.Policy, error) {
 	return service.Projects.GetIamPolicy(project, req).Context(ctx).Do()
 }
 
-// Remove existing bindings associated with service accounts of current deployment, and return the new policy
-func GetClearedIamPolicy(currentPolicy *cloudresourcemanager.Policy, pendingPolicy *cloudresourcemanager.Policy) *cloudresourcemanager.Policy {
+// Modify currentPolicy: Remove existing bindings associated with service accounts of current deployment
+func ClearIamPolicy(currentPolicy *cloudresourcemanager.Policy, pendingPolicy *cloudresourcemanager.Policy) {
 	serviceAccounts := make(map[string]bool)
 	for _, binding := range pendingPolicy.Bindings {
 		for _, member := range binding.Members {
@@ -80,7 +79,7 @@ func GetClearedIamPolicy(currentPolicy *cloudresourcemanager.Policy, pendingPoli
 			}
 		}
 	}
-	clearedPolicy := cloudresourcemanager.Policy{}
+	var newBindings []*cloudresourcemanager.Binding
 	for _, binding := range currentPolicy.Bindings {
 		newBinding := cloudresourcemanager.Binding{
 			Role: binding.Role,
@@ -92,9 +91,9 @@ func GetClearedIamPolicy(currentPolicy *cloudresourcemanager.Policy, pendingPoli
 				newBinding.Members = append(newBinding.Members, member)
 			}
 		}
-		clearedPolicy.Bindings = append(clearedPolicy.Bindings, &newBinding)
+		newBindings = append(newBindings, &newBinding)
 	}
-	return &clearedPolicy
+	currentPolicy.Bindings = newBindings
 }
 
 // TODO: Move type definitions to appropriate place.
@@ -146,10 +145,7 @@ func ReadIamBindingsYAML(filename string) (*cloudresourcemanager.Policy, error)
 }
 
 // Either patch or remove role bindings from `src` policy.
-func RewriteIamPolicy(currentPolicy *cloudresourcemanager.Policy, adding *cloudresourcemanager.Policy) (*cloudresourcemanager.Policy, error) {
-	if currentPolicy == nil {
-		return nil, fmt.Errorf("Source IAM policy is nil.")
-	}
+func RewriteIamPolicy(currentPolicy *cloudresourcemanager.Policy, adding *cloudresourcemanager.Policy) {
 	policyMap := map[string]map[string]bool{}
 	for _, binding := range currentPolicy.Bindings {
 		policyMap[binding.Role] = make(map[string]bool)
@@ -166,7 +162,7 @@ func RewriteIamPolicy(currentPolicy *cloudresourcemanager.Policy, adding *cloudr
 			policyMap[binding.Role][member] = true
 		}
 	}
-	newPolicy := cloudresourcemanager.Policy{}
+	var newBindings []*cloudresourcemanager.Binding
 	for role, memberSet := range policyMap {
 		binding := cloudresourcemanager.Binding{}
 		binding.Role = role
@@ -175,9 +171,9 @@ func RewriteIamPolicy(currentPolicy *cloudresourcemanager.Policy, adding *cloudr
 				binding.Members = append(binding.Members, member)
 			}
 		}
-		newPolicy.Bindings = append(newPolicy.Bindings, &binding)
+		newBindings = append(newBindings, &binding)
 	}
-	return &newPolicy, nil
+	currentPolicy.Bindings = newBindings
 }
 
 // "Override" project's IAM policy with given config.

bootstrap/pkg/utils/iamutils_test.go

@@ -51,6 +51,7 @@ func Test(t *testing.T) {
 						},
 					},
 				},
+				Etag: "ShouldKeep",
 			},
 			saPolicy: &cloudresourcemanager.Policy{
 				Bindings: []*cloudresourcemanager.Binding{
@@ -87,14 +88,15 @@ func Test(t *testing.T) {
 						},
 					},
 				},
+				Etag: "ShouldKeep",
 			},
 		},
 	}
 	for _, test := range tests {
-		clearedPolicy := GetClearedIamPolicy(test.currentPolicy, test.saPolicy)
-		if !reflect.DeepEqual(&clearedPolicy, &(test.expectedPolicy)) {
+		ClearIamPolicy(test.currentPolicy, test.saPolicy)
+		if !reflect.DeepEqual(test.currentPolicy, test.expectedPolicy) {
 			t.Errorf("Expect:\n%v; Output:\n%v", PolicyToString(test.expectedPolicy),
-				PolicyToString(clearedPolicy))
+				PolicyToString(test.currentPolicy))
 		}
 	}
 }