From 446f330927e926fd6e4a596a1332240d59096e17 Mon Sep 17 00:00:00 2001 From: Kunming Qu <37601826+kunmingg@users.noreply.github.com> Date: Wed, 20 Mar 2019 17:43:38 -0700 Subject: [PATCH] fix make build; set IAM policy following read/modify/write (#2751) * fix make build; set IAM policy following read/modify/write * address comments --- bootstrap/Dockerfile | 2 + bootstrap/Makefile | 2 +- bootstrap/cmd/bootstrap/app/gcpUtils.go | 44 +++++++++++++------- bootstrap/cmd/bootstrap/app/gcpUtils_test.go | 13 ++++-- bootstrap/cmd/bootstrap/app/ksServer.go | 2 +- bootstrap/cmd/bootstrap/app/server.go | 2 +- bootstrap/go.mod | 1 + bootstrap/go.sum | 2 + bootstrap/pkg/kfapp/gcp/gcp.go | 12 ++++-- bootstrap/pkg/utils/iamutils.go | 22 ++++------ bootstrap/pkg/utils/iamutils_test.go | 8 ++-- 11 files changed, 69 insertions(+), 41 deletions(-) diff --git a/bootstrap/Dockerfile b/bootstrap/Dockerfile index 94484b78f42..a5b9d43b21f 100644 --- a/bootstrap/Dockerfile +++ b/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 diff --git a/bootstrap/Makefile b/bootstrap/Makefile index 7cc3d2245db..8bf0127483a 100755 --- a/bootstrap/Makefile +++ b/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 diff --git a/bootstrap/cmd/bootstrap/app/gcpUtils.go b/bootstrap/cmd/bootstrap/app/gcpUtils.go index 46b58681113..360647988ae 100644 --- a/bootstrap/cmd/bootstrap/app/gcpUtils.go +++ b/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,23 +266,39 @@ 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) @@ -289,8 +306,5 @@ func (s *ksServer) ApplyIamPolicy(ctx context.Context, req ApplyIamRequest) erro } return nil }, exp) - if err != nil { - return err - } return nil } diff --git a/bootstrap/cmd/bootstrap/app/gcpUtils_test.go b/bootstrap/cmd/bootstrap/app/gcpUtils_test.go index b1bc997f3ae..2f5350de758 100644 --- a/bootstrap/cmd/bootstrap/app/gcpUtils_test.go +++ b/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) } }) diff --git a/bootstrap/cmd/bootstrap/app/ksServer.go b/bootstrap/cmd/bootstrap/app/ksServer.go index c313a7c3783..35fde76b6ed 100644 --- a/bootstrap/cmd/bootstrap/app/ksServer.go +++ b/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" diff --git a/bootstrap/cmd/bootstrap/app/server.go b/bootstrap/cmd/bootstrap/app/server.go index 9215697fc94..0e15a2a3b4a 100644 --- a/bootstrap/cmd/bootstrap/app/server.go +++ b/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" diff --git a/bootstrap/go.mod b/bootstrap/go.mod index 4a9153a95a4..1da1c22aa52 100644 --- a/bootstrap/go.mod +++ b/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 diff --git a/bootstrap/go.sum b/bootstrap/go.sum index b72100c85b6..b0e51bac48a 100644 --- a/bootstrap/go.sum +++ b/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= diff --git a/bootstrap/pkg/kfapp/gcp/gcp.go b/bootstrap/pkg/kfapp/gcp/gcp.go index fa9893a52de..1f61f99188a 100644 --- a/bootstrap/pkg/kfapp/gcp/gcp.go +++ b/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) } diff --git a/bootstrap/pkg/utils/iamutils.go b/bootstrap/pkg/utils/iamutils.go index 1e19179f397..2b50197d93e 100644 --- a/bootstrap/pkg/utils/iamutils.go +++ b/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. diff --git a/bootstrap/pkg/utils/iamutils_test.go b/bootstrap/pkg/utils/iamutils_test.go index a8325eadb51..ab77e4c7582 100644 --- a/bootstrap/pkg/utils/iamutils_test.go +++ b/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)) } } }