Drop GCP credentials env if user-gcp-sa secret is not present #2643

Bobgy · 2019-11-22T10:41:19Z

This is part of #2589

@IronPan @jingzhang36
Can you help me review?

Behavior summary:

When user's workflow uses user-gcp-sa secret
Backend checks if the secret exists
If the secret doesn't -> drop GOOGLE_APPLICATION_CREDENTIALS env and mark user-gcp-sa volume as optional for templates that mount user-gcp-sa.
If the secret does exist -> do nothing
If an error occurred when fetching the secret -> propagate the error above

Verified

When cluster's namespace doesn't have user-gcp-sa secret, this can drop env and mark the secret as optional. Therefore, tfx and xgboost samples run successfully.
When cluster's namespace have user-gcp-sa, this keeps env variables, and let pipelines run as normal.

~~TODO: add some unit tests~~
Done

TODO2: apply manifest changes here to Kubeflow too

/cc @rmgogogo
/assign @IronPan
/area backend

This change is

…ot present

k8s-ci-robot · 2019-11-22T11:20:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign ironpan
You can assign the PR to them by writing /assign @ironpan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

backend/src/apiserver/resource/resource_manager.go

rmgogogo · 2019-11-22T12:03:53Z

backend/src/apiserver/resource/resource_manager.go

+	}
+	if !someStepsHaveUserGcpSa {
+		// No steps used user-gcp-sa secret, no need to drop GOOGLE_APPLICATION_CREDENTIALS.
+		return


would that possible: no steps/components mounted secret but set the ADC env?

Let me explain. We only handle the default case of using use_gcp_secret with all default arguments.

If user doesn't have this secret, but have ADC env, they may have mounted secret as a different name. So we shouldn't break them

backend/src/apiserver/resource/resource_manager.go

rmgogogo · 2019-11-22T12:22:16Z

manifests/kustomize/base/pipeline/ml-pipeline-apiserver-role.yaml

@@ -13,6 +13,12 @@ rules:
  - get
  - list


https://github.com/kubeflow/pipelines/blob/master/manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/mysql.yaml#L46

would you help remove the line in one shot?

it's OK if no test for now.

so does MINIO.
I think MySQL and MINIO won't hit the createRun to allow you remove it.
https://github.com/kubeflow/pipelines/blob/master/manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/minio.yaml#L46

Thanks for pointing them out. I prefer a separate one because this is complex enough.
Will do that shortly after this.

OK, or feel free to leave it to me if multi-user is busy

backend/src/apiserver/resource/resource_manager.go

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/pipeline.yaml

…ding volume mount

Bobgy · 2019-11-25T04:41:11Z

/assign @jingzhang36
for early review

Bobgy · 2019-11-25T07:57:19Z

backend/src/apiserver/resource/resource_manager.go

@@ -281,6 +399,11 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) {
 		}
 	}

+	err = r.dropUserGcpSaIfNotConfigured(workflow)
+	if err != nil {
+		return nil, util.NewInternalServerError(err, "Failed to verify whether GCP secret is configured")


Ask for review. Call out that this is probably something that would be controversial. I discussed with @jingzhang36 about this. We concluded that because only users whose workflow contains use_gcp_secret can possibly hit this error. It's clearer if the error is returned to user, instead of silently swallowed (probably with a warning message that no user will ever see).

rmgogogo · 2019-11-25T09:58:17Z

backend/src/apiserver/resource/resource_manager.go

+				someStepHasGcpSecretVolume = true
+				break
+			}
+		}


nit: here also can break
if someStepHasGcpSecretVolume is true, it also can break in outer "for".

In fact, I would prefer writing no breaks here to keep it simple and easy to read. (because this cannot impact performance)

WDYT?

backend/src/apiserver/resource/resource_manager.go

rmgogogo · 2019-11-25T10:04:22Z

backend/src/apiserver/resource/resource_manager.go

+			*overrideOptional = true
+			volume.Secret.Optional = overrideOptional
+		}
+	}


FYI, only remove the ENV Var is enough, but OK here you also do these clean up.

ack, got your meaning to have more info for debugging

I prefer leaving the secret optional here to give them a clue what happened, just in case they may hit this unexepectedly.

rmgogogo · 2019-11-25T10:13:04Z

/lgtm

rmgogogo · 2019-11-26T13:43:01Z

/lgtm

not sure whether you tested this CL via CLI deployment. We can do the test via creating a full scope cluster with "gcloud".

IronPan · 2019-11-26T18:24:11Z

Why do we need this change? If you really just want to unblock the marketplace samples from being running, isn't it better just to fork the sample in mkp to not mounting the secret?

This change introduce "magical" behavior to the kfp backend. How do you plan to convey this behavior to the end user?

IronPan · 2019-11-26T18:24:15Z

/hold

Bobgy · 2019-11-27T01:28:54Z

Synced with @IronPan, because I don't have bandwidth on finishing workload identity documentation for samples and standalone deployment. He prefers finding someone else to finish those, so that we don't need this PR.

Bobgy · 2019-12-18T00:58:16Z

/close
in favor of #2619

k8s-ci-robot · 2019-12-18T00:58:18Z

@Bobgy: Closed this PR.

In response to this:

/close
in favor of #2619

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Bobgy added 4 commits November 22, 2019 17:28

Drop GOOGLE_APPLICATION_CREDENTIALS env unconditionally

37f68a2

Add secret k8s client

4331260

Refactor drop secret transformation to a separate function

1447d63

Drop GOOGLE_APPLICATION_CREDENTIALS conditionally if user-gcp-sa is n…

23215e8

…ot present

k8s-ci-robot assigned IronPan Nov 22, 2019

k8s-ci-robot added the area/backend label Nov 22, 2019

k8s-ci-robot requested a review from rmgogogo November 22, 2019 10:41

k8s-ci-robot added the size/L label Nov 22, 2019

Bobgy mentioned this pull request Nov 22, 2019

Make user-gcp-sa secret optional on GCP #2589

Closed

5 tasks

Bobgy added 2 commits November 22, 2019 18:50

Update BUILD.bazel

81a8e67

Update pipeline role permission

93c5785

rmgogogo reviewed Nov 22, 2019

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

rmgogogo reviewed Nov 22, 2019

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

rmgogogo reviewed Nov 22, 2019

View reviewed changes

Ark-kun reviewed Nov 22, 2019

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

Ark-kun reviewed Nov 22, 2019

View reviewed changes

manifests/gcp_marketplace/chart/kubeflow-pipelines/templates/pipeline.yaml Show resolved Hide resolved

Bobgy added 4 commits November 25, 2019 10:22

Improved the implementation by dropping env on steps with a correspon…

902b291

…ding volume mount

Add unit tests

7e55142

Reduced test code duplication

8e398b4

Simplify implementation

a1c3a54

k8s-ci-robot assigned jingzhang36 Nov 25, 2019

Bobgy added 4 commits November 25, 2019 14:47

Skip errors other than NotFound error

1495395

Error out if failed to verify secret is configured correctly or not

04bf7ca

Fix bazel BUILD file

0568ba0

Update tests

5e678f5

Bobgy commented Nov 25, 2019

View reviewed changes

rmgogogo reviewed Nov 25, 2019

View reviewed changes

backend/src/apiserver/resource/resource_manager.go Outdated Show resolved Hide resolved

rmgogogo reviewed Nov 25, 2019

View reviewed changes

k8s-ci-robot assigned rmgogogo Nov 25, 2019

k8s-ci-robot added the lgtm label Nov 25, 2019

Format according to comments

4fef8ab

k8s-ci-robot removed the lgtm label Nov 25, 2019

k8s-ci-robot added the lgtm label Nov 26, 2019

k8s-ci-robot added the do-not-merge/hold label Nov 26, 2019

k8s-ci-robot closed this Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop GCP credentials env if user-gcp-sa secret is not present #2643

Drop GCP credentials env if user-gcp-sa secret is not present #2643

Bobgy commented Nov 22, 2019 •

edited

Loading

k8s-ci-robot commented Nov 22, 2019

rmgogogo Nov 22, 2019

Bobgy Nov 23, 2019

rmgogogo Nov 22, 2019

rmgogogo Nov 22, 2019

rmgogogo Nov 22, 2019

Bobgy Nov 23, 2019

rmgogogo Nov 25, 2019

Bobgy commented Nov 25, 2019

Bobgy Nov 25, 2019

rmgogogo Nov 25, 2019

Bobgy Nov 25, 2019

rmgogogo Nov 26, 2019

rmgogogo Nov 25, 2019

rmgogogo Nov 25, 2019

Bobgy Nov 25, 2019

rmgogogo commented Nov 25, 2019

rmgogogo commented Nov 26, 2019

IronPan commented Nov 26, 2019

IronPan commented Nov 26, 2019

Bobgy commented Nov 27, 2019

Bobgy commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

Drop GCP credentials env if user-gcp-sa secret is not present #2643

Drop GCP credentials env if user-gcp-sa secret is not present #2643

Conversation

Bobgy commented Nov 22, 2019 • edited Loading

k8s-ci-robot commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmgogogo commented Nov 25, 2019

rmgogogo commented Nov 26, 2019

IronPan commented Nov 26, 2019

IronPan commented Nov 26, 2019

Bobgy commented Nov 27, 2019

Bobgy commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

Bobgy commented Nov 22, 2019 •

edited

Loading