part of 'add the gcp platform to kfctl (golang)'

[kkasravi]

fixes #2370

this PR is based off of PR #2433

This implements a good part of the gcp platform option for kfctl.
Additional work remains and will be addressed by a follow-on PR.

---

This change is 

[kkasravi]

Reviewable status: 13 of 21 files reviewed, 19 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, and @kunmingg)

---

bootstrap/pkg/client/gcp/gcp.go, line 181 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

Unmarshal will not work here. YAML file is not in the format of Deployment. Instead, you should fill in the field Name with deployment name, Target.Config.Content to be the file content in string.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 202 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

this should be optional - noop if the file doesn't exist.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 206 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

same here, gcfs should be optional.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 210 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

IIUC, we should do the following after deployments above:

• Update iam policy
• Set credentials for kubectl context.
• Create a named context.
• Set user as cluster admin.
• Create namespace if not found.

That is given we are following shell script logic, right?

Gabriel, please pick up. Note that namespace is created in ksonnet apply

---

bootstrap/pkg/client/gcp/gcp.go, line 497 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

s/configFileData/storageFileData in this line.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 500 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

a space after ipName: is needed.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 502 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

replace to with correct path str.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 509 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

replace to with correct path str.

Gabriel, please pick up

---

bootstrap/pkg/client/gcp/gcp.go, line 614 at r10 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

email format is not the same as the ones in generateDMConfigs. might be better to have a helper function so that naming is unified?

Gabriel, please pick up

[kkasravi]

@jlewi @gabrielwen
interim PR - to be followed by Gabriel's work on the gcp platform (gcp.go).
Jeremy, please merge.

[kkasravi]

@gabrielwen i made a few minor changes on the branch. You should get them on your PR after this is merged to master by doing git merge upstream/master

@jlewi this PR should be merged before #2530. Also noted there.

[jlewi]

Reviewed 1 of 3 files at r7, 1 of 8 files at r9, 2 of 5 files at r10, 4 of 5 files at r11.
Reviewable status: 21 of 22 files reviewed, 23 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, @kkasravi, and @kunmingg)

---

bootstrap/Makefile, line 18 at r11 (raw file):

GOLANG_VERSION ?= 1.11.5
GOPATH ?= $(HOME)/go
GOOGLE_APPLICATION_CREDENTIALS ?= $(HOME)/auth.json

Why do you need to set GOOGLE_APPLICATION_CREDENTIALS? I would expect code that needs credentials would automatically check various locations for credentials if needed

1. i.e. Check if GOOGLE_APPLICATION_CREDENTIALS is set
2. Fall back to check for credentials created by gcloud which should be cached in $(HOME)

Why does the MakeFile need to be involved in passing GOOGLE_APPLICATION_CREDENTIALS

---

bootstrap/Makefile, line 90 at r11 (raw file):

test-known-platforms-init: install build-dockerfordesktop-plugin
	@rm -rf $(HOME)/dockerfordesktop && \

Why use $(HOME)? If you need a build directory why not just make it a subdirectory of the current directory and add it to the .gitingore file?

---

bootstrap/cmd/kfctl/cmd/init.go, line 81 at r11 (raw file):

	initCmd.Flags().StringP(string(kftypes.PLATFORM), "p", kftypes.DefaultPlatform,
		"one of 'gcp|minikube|ksonnet'")

ksonnet is not a platform. The platform is "vanilla K8s" in the past we used "none" to indicate that it wasn't a specific platform.

I'm not sure "none" is the best name but how about we use it for consistency with kfutil.sh and think about changing it later?

---

bootstrap/pkg/client/gcp/gcp.go, line 534 at r7 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

this has been changed and I'm using GOOGLE_APPLICATION_CREDENTIALS to point to a json file that holds the credentials for the serviceaccount

Won't google.DefaultClient automatically use GOOGLE_APPLICATION_CRENDETIALS if its defined?

Will users need to download a service account and set GOOGLE_APPLICATION_CREDENTIALS?

The desired behavior is that it automatically uses the credentials cached in the home directory created by gcloud. If a user sets GOOGLE_APPLICATION_CREDENTIALS then those should be used.

My expectation is that the go client libraries will automatically try a variety of methods (including the 2 I listed above) to get credentials. So our code

---

bootstrap/pkg/client/gcp/gcp.go, line 401 at r9 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

yes, good point, see #2515

Can you add a TODO(#2515)

---

bootstrap/pkg/client/gcp/gcp.go, line 181 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

I take that to mean @gabrielwen will fix in a follow on PR? Can we make a note of that perhaps in the related issue #2370 so we can keep track of remaining work? And put a TODO here?

---

bootstrap/pkg/client/gcp/gcp.go, line 202 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

Please add a TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 206 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

Please add a TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 210 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up. Note that namespace is created in ksonnet apply

Please add a TODO

Some of the above steps aren't GCP specific

• Creating a named context; we should probably always do that.

---

bootstrap/pkg/client/gcp/gcp.go, line 500 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

Please add a TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 502 at r10 (raw file):

Please add a TODO
Please add a TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 509 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

Please add a TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 614 at r10 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

Gabriel, please pick up

Please add a TODO.

---

bootstrap/pkg/client/gcp/gcp.go, line 222 at r11 (raw file):

TODO

Can you explain this TODO? Per @gabrielwen's comment above if we make the current user a cluster admin is this still necessary?

---

bootstrap/pkg/client/gcp/gcp.go, line 232 at r11 (raw file):

/

---

bootstrap/pkg/client/gcp/gcp.go, line 246 at r11 (raw file):

		return fmt.Errorf("gcp apply could not create secrets Error %v", secretsErr)
	}
	ks := gcp.Children[kftypes.KSONNET]

So the architecture is the platform in this case is GCP owns children and is responsible for applying the children?

I was envisioning platform and K8s having more of a sibling relationship. I don't know which is better or that we should try to resolve it in this PR but I think its something we might want to think about.

---

bootstrap/pkg/client/minikube/minikube.go, line 68 at r11 (raw file):

func (minikube *Minikube) Delete(resources kftypes.ResourceEnum, options map[string]interface{}) error {
	ks := minikube.Children[kftypes.KSONNET]
	if ks != nil {

Per comment above about parent vs child relationships. By making K8s a child of the platform KFApp we force every platform app to write logic to sequence the creation of the platform and the K8s resources.

I think a better approach would be to create an UberKFApp which would have a K8s KFApp and a Platform KFApp and know how to sequence the two.

Should we file an issue for that

/cc @gabrielwen

[jlewi]

@kkasravi How about updating the PR description and title to reflect the fact that this is a big chunk of work needed to support GCP but there's still more work needed?

[gabrielwen]

+1 lgtm once description is updated.

[kkasravi]

updated

Reviewable status: 20 of 22 files reviewed, 23 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, and @kunmingg)

---

bootstrap/Makefile, line 18 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Why do you need to set GOOGLE_APPLICATION_CREDENTIALS? I would expect code that needs credentials would automatically check various locations for credentials if needed

1. i.e. Check if GOOGLE_APPLICATION_CREDENTIALS is set
2. Fall back to check for credentials created by gcloud which should be cached in $(HOME)

Why does the MakeFile need to be involved in passing GOOGLE_APPLICATION_CREDENTIALS

i agree, the credentials should be searched in various areas and the Makefile doesn't need to set this. Can we handle this as a follow-up task that Gabriel or I could address? I've opened #2535

---

bootstrap/Makefile, line 90 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Why use $(HOME)? If you need a build directory why not just make it a subdirectory of the current directory and add it to the .gitingore file?

sure, better suggestion, thanks see #2537

---

bootstrap/cmd/kfctl/cmd/init.go, line 81 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

ksonnet is not a platform. The platform is "vanilla K8s" in the past we used "none" to indicate that it wasn't a specific platform.

I'm not sure "none" is the best name but how about we use it for consistency with kfutil.sh and think about changing it later?

ok, so in effect just k8s resources are deployed if no platform is specified, although some platforms may add or subtract to these resources (gcp, minikube). Maybe this can be done as part of PR #2530?

---

bootstrap/pkg/client/gcp/gcp.go, line 534 at r7 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Won't google.DefaultClient automatically use GOOGLE_APPLICATION_CRENDETIALS if its defined?

Will users need to download a service account and set GOOGLE_APPLICATION_CREDENTIALS?

The desired behavior is that it automatically uses the credentials cached in the home directory created by gcloud. If a user sets GOOGLE_APPLICATION_CREDENTIALS then those should be used.

My expectation is that the go client libraries will automatically try a variety of methods (including the 2 I listed above) to get credentials. So our code

yep, noted earlier #2535

---

bootstrap/pkg/client/gcp/gcp.go, line 401 at r9 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Can you add a TODO(#2515)

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 181 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

I take that to mean @gabrielwen will fix in a follow on PR? Can we make a note of that perhaps in the related issue #2370 so we can keep track of remaining work? And put a TODO here?

yes

---

bootstrap/pkg/client/gcp/gcp.go, line 202 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 206 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 210 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO

Some of the above steps aren't GCP specific

• Creating a named context; we should probably always do that.

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 500 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 502 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO
Please add a TODO

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 509 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 614 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a TODO.

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 222 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

TODO

Can you explain this TODO? Per @gabrielwen's comment above if we make the current user a cluster admin is this still necessary?

i don't think it is, I was just referencing what scripts/gke/util.sh did. I've updated the TODO

---

bootstrap/pkg/client/gcp/gcp.go, line 232 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

/

Done.

---

bootstrap/pkg/client/gcp/gcp.go, line 246 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

So the architecture is the platform in this case is GCP owns children and is responsible for applying the children?

I was envisioning platform and K8s having more of a sibling relationship. I don't know which is better or that we should try to resolve it in this PR but I think its something we might want to think about.

yes, right now KSONNET is sort of being treated as a platform which you've noted it isn't. Although it implements the KfApi interface, it's really just handling K8 resources. I think we need a better model when we add kustomize - perhaps it can be revisited in that issue.

---

bootstrap/pkg/client/minikube/minikube.go, line 68 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Per comment above about parent vs child relationships. By making K8s a child of the platform KFApp we force every platform app to write logic to sequence the creation of the platform and the K8s resources.

I think a better approach would be to create an UberKFApp which would have a K8s KFApp and a Platform KFApp and know how to sequence the two.

Should we file an issue for that

/cc @gabrielwen

yes #2536

[kkasravi]

Reviewable status: 20 of 22 files reviewed, 23 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, and @kunmingg)

---

bootstrap/Makefile, line 90 at r11 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

sure, better suggestion, thanks see #2537

Done.

---

bootstrap/cmd/kfctl/cmd/init.go, line 81 at r11 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

ok, so in effect just k8s resources are deployed if no platform is specified, although some platforms may add or subtract to these resources (gcp, minikube). Maybe this can be done as part of PR #2530?

making this update as part of #2530

[jlewi]

Reviewed 1 of 5 files at r11, 1 of 1 files at r12.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, @kkasravi, and @kunmingg)

---

bootstrap/Makefile, line 18 at r11 (raw file):

Previously, kkasravi (Kam Kasravi) wrote…

i agree, the credentials should be searched in various areas and the Makefile doesn't need to set this. Can we handle this as a follow-up task that Gabriel or I could address? I've opened #2535

SGTM.

[jlewi]

/lgtm
/approve
/hold for @gabrielwen

@gabrielwen WDYT shall we go ahead and merge this and continue to iterate in follow on PRs?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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

The pull request process is described here

Needs approval from an approver in each of these files:

• bootstrap/OWNERS [jlewi]

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