Get rid of KFDef kustomize magic?

[jlewi]

Right now KFDef provides the following functionality

1. The ability to list a set of kustomize packages to be deployed
2. The ability to set parameters for each of those applications
3. The ability to combine multiple overlays like mixins (see comment)

Could we get rid of this magic and rely more on straight kustomize.

• Per kubernetes-sigs/kustomize#171 I believe kustomize supports composition when each base package describes a distinct set of resources. So instead of listing kustomize packages in KFDef I think we could just define a new kustomize package with a kustomization.yaml that looked like

bases:
- jupyter/notebook-controller/overlays/application
- jupyter/jupyter-web-app/overlays/application

A user could then just do

kustomize build to see all resources

Right now our overlays are structured such that we have multiple overlays (e.g. istio and application) which depend on base and then kfctl combines them.

But we don't really need that magic. We could restructure our overlays so that they all use what kustomize supports.

Along these lines we might want to rethink how we do parameter substitution to better leverage https://github.com/kubernetes-sigs/kustomize/blob/master/examples/configGeneration.md

Can we structure it so that overriding parameters) would just be generating config map patches overlays in this package?

Related issues and threads: kubeflow/kfctl#156 kubeflow/kubeflow#3490

[issue-label-bot[bot]]

Issue-Label Bot is automatically applying the labels:

Label	Probability
feature	0.88

Please mark this comment with :thumbsup: or :thumbsdown: to give our bot feedback! Links: app homepage, dashboard and code for this bot.

[jlewi]

From a release stand point I think it could be very useful if we had overlays for each version e.g. if we structured our kustomize apps

bases
  ...
overlays
   master
     ...
    v1.0.0
    v1.1.0

v1.1.0 could potentially use v1.0.0 as its base.

One of the motivations here is to reduce the cost of maintaining release branches.

Right now there is a bunch of work that needs to be done to update the release branch for each application e.g.

1. Bump the version in the Application CR
2. Update the docker image

Right now we make these changes in the base package.

That means we can't make these changes until we cut a release branch. This means its hard to simultaneously

• Start preparing the application for the release
• Continue to fix bugs

If we use overlays for the version then we can start checking that into master and easily keep it in sync as fixes to the underlying base deployment go in.

[SebBlin]

The magic feature of having multiple overlays and combining them is a "really great feature" and should be kept. I use it to build my own repo that contain all my personnal customization to apply during the deployment process.

Here is an exemple below for Dex : Currently Dex is able to delegate authentication to an external OIDC IDentity Provider and this is something I want to use, but this IDP is behind a forward proxy so I need to update the dex container configuration to add env variables to configure http_proxy. In addition, this IDP is secured with a private CA that I need to add in the Dex container (I had to rebuild my own dex container); so I need to upate the name of dex image to load. At last, the Dex parameters in KFDef currently don't support configuring an OIDC provider, it can be done by updating the dex config-map.

With few files in my repo /my-manifests/dex/overlays/my-overlay I can achieve a very custumized deployement (even customizations that are not pre-configurable) and I stil benefit form the standards manifests without duplicating them !

Here is the content of my overlay /my-manifests/dex/overlays/my-overlay config-map.yaml # the new content of the config map with my OIDC configuration deployment.yaml # the patch of the Deployment to just add 'envFrom: Proxies' to the dex container kustomization.yaml # the update for the name of dex image to point to my private registry

Here is the content of my KFDef (extracts)

apiVersion: kfdef.apps.kubeflow.org/v1beta1
kind: KfDef
metadata:
  name: MyKubeflow
  namespace: kubeflow
spec:
  applications:
[ ... ]
  - kustomizeConfig:
      overlays:
        - istio
      parameters:
        - name: namespace
          value: auth
          [ ... Other dex parameters ... ]
      repoRef:
        name: manifests
        path: dex-auth/dex-crds
    name: dex
  - kustomizeConfig:
      overlays:
        - my-overlay
      repoRef:
        name: my-manifests
        path: dex
    name: dex
  [ ... ]
  repos:
  - name: manifests
    uri: https://codeload.github.com/kubeflow/manifests/tar.gz/v0.7-branch
  - name: my-manifests
    uri: https://mygitserver.example.com/kubeflow/kubeflow-manifests/-/archive/master/kubeflow-manifests-master.tar.gz
  version: master

This is just an exemple for one component but I also use this for others. Currently, combining is not always working exactly the I would like it to work (and I have to workaround); but, please, keep it and improve it.

[kunmingg]

+1 for remove kfdef magic.

• Maintaining non-standard kustomize feature is non-trivial cost.
• Our manifest repo should allow user to apply them with kustomize CLI directly. With kfctl logics user will get different results from kfctl vs kustomize

@SebBlin If we have solid use cases that need merging overlays feature, we can open PR to kustomize repo directly so the feature becomes kustomize standard.

[jlewi]

@SebBlin Thanks. Can't you achieve what you want in Kustomize? i.e. it sounds like you want to define an overlay which would be applied to the Kubeflow Dex manifest. So your overlay would just declare the Kubeflow Dex manifest as its base.

Kustomize supports chaining; i.e. A -> B -> C

• Where B & C are overlays
• And B is using A as a base, and C uses B as its base

What Kubeflow implemented is different we have the case

A->B A->C

and now we want to construct the package

A-> B+C

• So B and C are both overlays defining A as their base and now we want to combine those overlays.

[jlewi]

This is the feature request in Kustomize for mixin support : kubernetes-sigs/kustomize#759

[vpavlin]

For someone new starting with Kustomize and kfctl, the magic the latter adds is quite hard to get handle on - @jlewi is what you described above explained anywhere in KF docs? It makes sense like this, but it was not straightforward when I started with Kubeflow and since I always fell back to Kustomize doc it got even more confusing as it did not match what I was able to learn there:-)

So my 2c - best if this is functionality of Kustomize rather than magic built into kfctl

[mattnworb]

+1 to @vpavlin. Only a subset of Kubernetes users are familiar with kustomize in the first place, and it will always be an even smaller subset who will understand kfctl’s modifications to kustomize.

[SebBlin]

@jlewi hello. it would be fine if this feature is supported directly by kustomize. I agree it would be better not to include this sort of code so specific to kfctl. At least, it is interesting to keep the posibility to declare the public repo, as well as a private and merge both ; this feature less complex, this is just some folders copy at the right place.

regards Sébastien

[jlewi]

@kunmingg has a PR illustrating how we could refactor each application to eliminate the need to combine patches additively. https://github.com/kubeflow/manifests/pull/873

/cc @kkasravi

[yanniszark]

Adding another data point to this:

The kustomization structure of kubebuilder (multiple bases) is currently not compatible with kfctl.

[krishnadurai]

@jlewi @kunmingg

Changing the behaviour of kfctl will make kfdef incompatible across versions, with the change mentioned here:

https://github.com/kubeflow/manifests/pull/873#issue-371679100

Given that we are changing from Kubeflow implementation to be Kustomize friendly:

What Kubeflow implemented is different we have the case

A->B A->C

and now we want to construct the package

A-> B+C

* So B and C are both overlays defining A as their base and now we want to combine those overlays.

How are we thinking of being backwards compatible for:

From a release stand point I think it could be very useful if we had overlays for each version e.g. if we structured our kustomize apps

bases
  ...
overlays
   master
     ...
    v1.0.0
    v1.1.0

v1.1.0 could potentially use v1.0.0 as its base.

Should we leave out overlays field for backwards compatibility?

[jlewi]

Here's the doc: http://bit.ly/kf_kustomize_v3

@krishnadurai I don't think backwards compatibility is an issue.

For the example you give, what kfctl is doing is actually what kustomize would do.

Here's how I think of it:

A->B =  resource.yaml + patchB.yaml
A->C = resource.yaml + patchC.yaml

With kfctl we defined a new package D with bases B and C. The way that was interpreted by kfctl was

D= resource.yaml + patchB.yaml + patchC.yaml

We can express this in a kustomization.yaml

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- resource.yaml
patchesStrategicMerge:
- patchB.yaml
- patchC.yaml

In fact generating a kustomization like this is exactly what kfctl is doing.

My conjecture (@kkasravi will hopefully chime in) is that the reason we didn't do this originally was become of some limitations in early versions of kustomize (see here).

@yanniszark

The kustomization structure of kubebuilder (multiple bases) is currently not compatible with kfctl.

Can you elaborate or provide an example?

[dan-slinky-ckpd]

Folks on this ticket might be interested in this KEP https://github.com/kubernetes/enhancements/pull/1803

[jlewi]

I'm closing this issue because remaining work is being tracked by kubeflow/manifests#1062