kubeflow / kfctl

kfctl is a CLI for deploying and managing Kubeflow
Apache License 2.0
181 stars 137 forks source link

add kustomize fn to merge configmap for application #364

Closed adrian555 closed 4 years ago

adrian555 commented 4 years ago

For certain applications, users need to provide custom values for configurations defined in the ConfigMap. Once KfDef is conformed to kustomize v3, it will not allow users to specify the parameters. Therefore, when deploying with kfctl command line, there is a need for extra steps to merge the users-provided configurations with the default from the KfDef. An example usage is dex-auth where a department/organization specific password/secret should only merge with the deployment for that department/organization. kpt fn is a convenient and good approach for this purpose.

This function requires a configuration file defined such as follow:

apiVersion: v1alpha1
kind: ConfigMapMerge
metadata:
  name: dex-auth-merge
  annotations:
    config.kubernetes.io/function: |
      container:
        image: aipipeline/kpt-fn:latest
spec:
  configMaps:
  - name: dex-parameters
    behavior: merge
    literals:
    - application_secret="<the-client-secret-of-oidc-service>"
    - github_client_id="<github oauth app client id>"
    - github_client_secret="<github oauth app client secret>"
    - github_hostname="github.ibm.com"
    - github_org_name="whitewater-analytics"
    - oidc_provider="http://dex.auth.svc.cluster.local:5556/dex"
    - oidc_redirect_uris='["/login/oidc"]'

This file will be copied to the application's directory where its kustomization.yaml file locates.

Then run the following command

kpt fn run .

to merge the configurations.

kubeflow-bot commented 4 years ago

This change is Reviewable

k8s-ci-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign adrian555 You can assign the PR to them by writing /assign @adrian555 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: - **[OWNERS](https://github.com/kubeflow/kfctl/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
adrian555 commented 4 years ago

/cc @shawnzhu

@jlewi could you please review? Thanks.

jlewi commented 4 years ago

@adrian555 Could you explain what the merge behavior for this kpt function is? kustomize does support merging config maps via an overlay. Could you explain why that functionality doesn't work in this case and what your function is doing differently?

I believe kustomize only supports adding or overwriting entire files in the configmap. So I suspect that doesn't work because you are trying to modify the contents of a specific file?

Is that file YAML?

shawnzhu commented 4 years ago

So I suspect that doesn't work because you are trying to modify the contents of a specific file?

Yes. it's for updating a configMapGenerator.

Given kustomize/oidc-authservice/kustomization.yaml:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../../.cache/manifests/manifests-ibm-dex/stacks/ibm/application/oidc-authservice

And function description:

apiVersion: v1alpha1
kind: ConfigMapMerge
metadata:
  name: oidc-authservice-merge
  annotations:
    config.kubernetes.io/function: |
      container:
        image: aipipeline/kpt-fn:latest
spec:
  configMaps:
  - name: oidc-authservice-parameters
    behavior: merge
    literals:
    - application_secret="<the-client-secret-of-oidc-service>"

Then running kpt fn kustomize/oidc-authservice, it will transform kustomize/oidc-authservice/kustomization.yaml into:

apiVersion: kustomize.config.k8s.io/v1beta1
configMapGenerator:
- behavior: merge
  literals:
  - application_secret="<the-client-secret-of-oidc-service>"
  name: oidc-authservice-parameters
kind: Kustomization
resources:
- ../../.cache/manifests/manifests-ibm-dex/stacks/ibm/application/oidc-authservice

PS: It's for supporting auth-enabled deployment: kubeflow/website#2028

adrian555 commented 4 years ago

Thanks @jlewi.

The situation is related to the kustomize v3 change that we no longer are allowed to pass the values for parameters in parameters field in the KfDef configurations. Prior to kustomize v3, we just need one command kfctl apply -V -f kfctl_ibm_dex.yaml to deploy the kubeflow while setting different values for the parameters. Now, we are lacking of such capability. Hence we will have to break the deployment into several commands: kfctl build -V -f kfctl_ibm_dex.yaml to build the initial kustomizations, followed by some commands to set the values to parameters, and then followed by kfctl apply -V -f kfctl_ibm_dex.yaml to deploy.

This kpt fn command is to handle the second step. Yes, adding an overlay to $KFAPP/kustomize/dex-auth may achieve the same functionality to provide org/department specific values for a set of parameters, should it make sense to create an overlay for each org/department . But in case where the only difference between two kubeflow deployments is a password or github secret, I would not think it is a good practice for an overlay. And obviously it is not as convenient as a single kustomize fn. There are multiple commands involved and it also requires kustomize knowledge and set up the directory correctly. On the contrary, a kustomize fn hides that details and with a provided template of ConfigMapMerge, all users required to update are the <key=value>.

To be specific, the dex-crds has this in its kustomization.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: auth
...
configMapGenerator:
- name: dex-parameters
  envs:
  - params.env

and params.env has following k=v:


# Dex Server Parameters (some params are shared with client)
dex_domain=dex.example.com
# Set issuer to https if tls is enabled
issuer=http://dex.example.com:32000
static_email=admin@example.com
static_password_hash=$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W
static_username=admin
static_user_id=08a8684b-db88-4b73-90a9-3cd1661f5466
client_id=ldapdexapp
oidc_redirect_uris=['http://login.example.org:5555/callback/onprem-cluster']
application_secret=pUBnBOY80SnXgjibTYM9ZWNzY2xreNGQok

the simple use case is when we are looking at how to provide different static_password, application_secret etc. The kfctl_ibm_dex.yaml has the dex_auth application with the path. In kustomize v3, we will only has a kustomization.yaml in $KFAPP/kustomize/dex-auth directory. The kustomization.yaml looks like follow:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../../dex-auth/dex-crds/overlays/github

With this kustomize fn and the configuration template

apiVersion: v1alpha1
kind: ConfigMapMerge
metadata:
  name: dex-auth-merge
  annotations:
    config.kubernetes.io/function: |
      container:
        image: aipipeline/kpt-fn:latest
spec:
  configMaps:
  - name: dex-parameters
    behavior: merge
    literals:
    - application_secret="<the-client-secret-of-oidc-service>"
    - static_password_hash="xxx"

users only need to update the values in the ConfigMapMerge configuration. The result kustomization.yaml then becomes

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- ../../dex-auth/dex-crds/overlays/github
configMapGenerator:
- name: dex-parameters
  behavior: merge
  literals:
  - application_secret=pUBnBOY80SnXgjibTYM9ZWNzY2xreNGQok
  - static_password_hash="xxx"

This appears to be simpler then messing with the kustomization.yaml and/or overlays directly.

I believe kustomize only supports adding or overwriting entire files in the configmap. Not quite sure what you referred to. But with the configMapGenerator, you are allowed to update (with merge behavior) some parameters, even all those parameters are part of params.env file in the base kustomization.

Hope I answer your questions without adding more confusion. :)

adrian555 commented 4 years ago

Thanks @shawnzhu for the further explanation.

jlewi commented 4 years ago

To the extent since this is IBM/platform specific you can consider this advisory/non-blocking feedback

kfctl build -V -f kfctl_ibm_dex.yaml to build the initial kustomizations,

In general, I think we want to get away from dynamically generating kustomization.yaml files (kubeflow/manifests#774).

Are you really building kustomization.yamls as opposed to referencing the kustomization.yamls that are checked in.

This appears to be simpler then messing with the kustomization.yaml and/or overlays directly.

I'm not sure I agree but this is just my oppinion. On GCP what we are doing is checking in a directory that contains the overlays the user would need to write/kustomize.

Here's the template https://github.com/kubeflow/gcp-blueprints/tree/master/kubeflow/instance/kustomize/kubeflow-apps

We use kpt setters to make it easy for people to substitute in the values specific to their use case.

I think overlays have two advantages over using a kpt fn in this case

  1. We want to encourage a user to define overlays containing any customizations they want to apply

    • This makes upgrades easier because we can just replace the upstream manifests with an updated copy and apply the user defined overlays ontop of them. So this makes it easy to preserve their kustomizations on upgrades
  2. Using a kpt fn to transform kustomization.yaml feels like you are bringing back the dynamic generation of kustomization.yaml files (kubeflow/manifests#774)

    • In other words, this doesn't seem like a commonly accepted pattern and will most likely cause confusion compared to overlays

Another issue is specific to secrets. Checking in secrets into source control is not good. My expectation would be that kustomization.yamls are checked into source control. It looks like secrets are getting stored in kustomization.yaml

e.g.

configMapGenerator:
- name: dex-parameters
  behavior: merge
  literals:
  - application_secret=pUBnBOY80SnXgjibTYM9ZWNzY2xreNGQok
  - static_password_hash="xxx"

How do people avoid checking these into source control?

On GCP what we do is have users run kubectl create secret ... and use environment variables to pass the value of the secret.

Longer term we will use a custom resource or process running in cluster to create the secrets from secret manager. https://cloud.google.com/secret-manager

/cc @johnugeorge @yanniszark