Semi-working kfctl go binary

[gabrielwen]

Works related to this PR:

• Fixes kfctl go binary should have an option to skip initi project #2491
• Fixes kfctl: support deploy with basic auth #2472
• Fixes add additional config files to kfctl for iap, basic_auth, default components #2514
• Making add the gcp platform to kfctl (golang) #2370 ~80% finished.

Known WIPs:

• [kfctl] Consolidate kubeconfig and kfctl API calls #2586
• [kfctl] Implements 3 way merge to CreateResourceFromFile or shell out to kubectl #2585
• kfctl installs istio #2507
• kfctl (golang) Remove hard dependency on GOOGLE_APPLICATION_CREDENTIALS #2535
• kfctl should try and apply all components rather than just the application component #2391
• [GCP] kfctl create named context and set namespace #2604
• When creating resources, some of resources has error message roles.rbac.authorization.k8s.io [resource] is forbidden: attempt to grant extra privileges
• Error message Resource creation for metricscollector-template is failed at 1 attempt: ConfigMap "" is invalid: metadata.name: Required value: name or generateName is required
• [GCP] kfctl iap setup permissions error #2605 pod iap doesn't seem to be working - Having error message like [gcp] backend services can't be listed insufficient IAM permissions #953

---

This change is 

[gabrielwen]

Reviewable status: 14 of 15 files reviewed, 15 unresolved discussions (waiting on @ellis-bigelow, @jlewi, @kunmingg, and @lluunn)

---

bootstrap/config/kfctl_iap.yaml, line 37 at r6 (raw file):

notebook-controller
Done

---

bootstrap/config/kfctl_iap.yaml, line 57 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Should this be renamed?
protoParams -> componentParam
compParams -> ksInitParams

What parameters are getting set with ks init?

Do you need to rebase?

Looking at
https://github.com/kubeflow/kubeflow/blob/master/bootstrap/config/kfctl_iap.yaml

It looks like this called parameters at master?

it is because in #2478 Pan removed a lot of parameters in prototypes. Therefore we couldn't pass those parameters during ks init. In fact, in kfctl.sh we do call ks init pipeline pipeline and later call ks param set <...> <...>.

---

bootstrap/config/types.go, line 26 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Can you explain? Are we not using the same go struct and YAML config file as the Click To Deploy App is currently using?

I didn't check how click to deploy app does, but we need a way to differentiate parameters available during ks init and ks param set.

---

bootstrap/pkg/apis/apps/group.go, line 56 at r10 (raw file):

ksInitParams
we do set it in kfctl.sh:

kubeflow/scripts/gke/util.sh

Line 60 in 922382e

sed -i.bak "s/gkeApiVersion: SET_GKE_API_VERSION/gkeApiVersion: ${GKE_API_VERSION}/" "${KUBEFLOW_DM_DIR}/${CONFIG_FILE}"

---

bootstrap/pkg/client/gcp/gcp.go, line 649 at r6 (raw file):

2515
Done

---

bootstrap/pkg/client/gcp/gcp.go, line 676 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Why do we have to replace GKE_API_VERSION? The idea here is that whatever the desired value should be set directly in the DM config file. So maybe you need to update the YAML file and then delete this call?

we do have it in

kubeflow/scripts/gke/util.sh

Line 60 in 922382e

sed -i.bak "s/gkeApiVersion: SET_GKE_API_VERSION/gkeApiVersion: ${GKE_API_VERSION}/" "${KUBEFLOW_DM_DIR}/${CONFIG_FILE}"

---

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

Previously, jlewi (Jeremy Lewi) wrote…

I wonder why we are doing impersonate here?
I don't believe we do that with kfctl.sh.

No action needed but something to think about in terms of potential RBAC errors.

we do:

kubeflow/scripts/gke/util.sh

Line 197 in 922382e

kubectl apply -f ${KUBEFLOW_K8S_MANIFESTS_DIR}/rbac-setup.yaml --as=admin --as-group=system:masters

---

bootstrap/pkg/client/ksonnet/ksonnet.go, line 184 at r10 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Could you please a TODO(#2391) to replace this with ksApply

Done

---

bootstrap/pkg/utils/k8utils.go, line 85 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Makes sense. Could you add a TODO explaining why we aren't using kubectl as a library here and why we might want to consider doing so in the future.

Added.

[jlewi]

Reviewed 4 of 6 files at r11.
Reviewable status: 13 of 15 files reviewed, 12 unresolved discussions (waiting on @ellis-bigelow, @gabrielwen, @jlewi, @kunmingg, and @lluunn)

---

bootstrap/config/kfctl_iap.yaml, line 57 at r6 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

it is because in #2478 Pan removed a lot of parameters in prototypes. Therefore we couldn't pass those parameters during ks init. In fact, in kfctl.sh we do call ks init pipeline pipeline and later call ks param set <...> <...>.

Do we need to set any component parameters during ks init?

I would expect ks init only sets ks app related parameters.

I would expect all component parameters to be set consistently.

---

bootstrap/config/kfctl_iap.yaml, line 40 at r11 (raw file):

  - tensorboard
  - tf-job-operator
ksInitParams:

What's the difference between ksInitParams and componentParams?

I would expect that on ks init we only set parameters related to the ksonnet application; e.g. namespace.

cert-manager and iap-ingress are component parameters so why are these set differently then the other component parameters as listed below?

---

bootstrap/config/types.go, line 26 at r6 (raw file):

Previously, gabrielwen (Hung-Ting Wen) wrote…

I didn't check how click to deploy app does, but we need a way to differentiate parameters available during ks init and ks param set.

See comment below about KsInitParams

---

bootstrap/config/types.go, line 37 at r11 (raw file):

	ComponentParams Parameters `json:"componentParams,omitempty"`
	// Parameters to be passed into component prototypes.
	KsInitParams Parameters `json:"ksInitParams,omityempty"`

Ok I get it some parameters need to be specified during component initialization. These will be parameters that are required. Furthermore, only parameters that are explicitly defined in the prototype can be passed during ks init which is why Yang's recent change #2478 creates an issue. In #2478 we no longer explicitly define a bunch of parameters in the prototype so they can no longer be set during ksInitParam.

Instead of having a separate structure of Parameters; can we just add a field to ComponentParams?

Perhaps we can add a field indicating whether its a definedParameter in the prototype and default it to true?

Or alternatively add a field indicating whether its a required parameter and default it to false?

I think we only need to set required parameters during init of the component; all other parameters could be set consistently using the equivalent of ks param set.

---

bootstrap/pkg/client/ksonnet/ksonnet.go, line 451 at r11 (raw file):

		}
		parameterArgs := []string{}
		if val, ok := config.KsInitParams[compName]; ok {

Ok I get it now. There are some required parameters like name that need to be specified during KsInit.

[gabrielwen]

Reviewable status: 11 of 15 files reviewed, 12 unresolved discussions (waiting on @ellis-bigelow, @jlewi, @kunmingg, and @lluunn)

---

bootstrap/config/kfctl_iap.yaml, line 57 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Do we need to set any component parameters during ks init?

I would expect ks init only sets ks app related parameters.

I would expect all component parameters to be set consistently.

added InitRequired

---

bootstrap/config/kfctl_iap.yaml, line 40 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

What's the difference between ksInitParams and componentParams?

I would expect that on ks init we only set parameters related to the ksonnet application; e.g. namespace.

cert-manager and iap-ingress are component parameters so why are these set differently then the other component parameters as listed below?

added a new field in NameValue

---

bootstrap/config/types.go, line 26 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

See comment below about KsInitParams

OK

---

bootstrap/config/types.go, line 37 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Ok I get it some parameters need to be specified during component initialization. These will be parameters that are required. Furthermore, only parameters that are explicitly defined in the prototype can be passed during ks init which is why Yang's recent change #2478 creates an issue. In #2478 we no longer explicitly define a bunch of parameters in the prototype so they can no longer be set during ksInitParam.

Instead of having a separate structure of Parameters; can we just add a field to ComponentParams?

Perhaps we can add a field indicating whether its a definedParameter in the prototype and default it to true?

Or alternatively add a field indicating whether its a required parameter and default it to false?

I think we only need to set required parameters during init of the component; all other parameters could be set consistently using the equivalent of ks param set.

new field added.

---

bootstrap/pkg/client/ksonnet/ksonnet.go, line 451 at r11 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Ok I get it now. There are some required parameters like name that need to be specified during KsInit.

OK

[jlewi]

Reviewed 4 of 4 files at r12.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ellis-bigelow, @kunmingg, and @lluunn)

[jlewi]

/lgtm
/approve

[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