istio injection in Kubeflow namespace breaks KF 1.0 configs

[jlewi]

#131 changed kfctl to set a label on the kubeflow namespace to turn on istio sidecar injection in the kubeflow namespace.

This change is not backwards compatible; i.e using an updated kfctl with the existing KFDef 1.0 manifests would cause Kubeflow 1.0 to stop working because kubeflow 1.0 is not comaptible with istio side car injection in the kubeflow namespace.

This is causing the tests on the kubeflow/manifests 1.0 branch to fail because they are testing with kfctl from master.

Proposed course of action:

1. Rollback Enable istio injection in Kubeflow namespace #131
2. Figure out a way to selectively enable istio side car injection only for Kubeflow configs >= 1.1

/assign @Bobgy

[issue-label-bot]

Issue-Label Bot is automatically applying the labels:

Label	Probability
kind/bug	0.96

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

[Bobgy]

Makes sense to me.

So we can guard the behavior of enabling istio injection in kfctl behind a config value in KfDef?
Who's the best person I should talk to? Should I try to contribute this one?

[jlewi]

@Bobgy Yeah I think it would be great if you could pick this up. I think the difficulty will be deciding on a good pattern for implementing this.

Here are some of the options I can think of.

1. Adding an explicit field to KFDef for this
2. Adding logic based on the version/url of the manifests to decide whether its sufficiently high enough version to turn on istio
3. Applying KFDef's labels to the Kubeflow namespace
4. Adding a KFDef annotation
5. Create a new kustomize package that defines the namespace objects with associated labels

I don't think #1 is a good approach. In general, I want to remove logic and KFDef and kfctl and move kfctl in the direction of being dumb, syntactic sugar around kustomize. So I think this would be a step in the wrong direction.

#2 seems brittle.

For #3 I'm not sure how we would achieve consistent semantics. In particular KF is potentially creating multiple namespaces (e.g. namespace for kubeflow components, istio namespace, knative, etc....). I don't think its true that we would want to set istio side car injection in all those namespaces uniformly. So applying the labels to all the namespaces wouldn't work and I don't like the idea of hardcoding special treatment for certain namespaces.

#4 This is probably the obvious quick fix but it feels like a bit of a hack.

#5 I think I like this approach the best. Moving the namespaces we want created into YAML files as opposed to defining them in code seems consistent with the overall direction we want to move in. It also makes it really easy for users to customize. However, there might be some problems I'm not anticipating.

@yanniszark @johnugeorge @animeshsingh WDYT?

[Bobgy]

Thanks for the thorough evaluation of tradeoffs. I totally agree #5 is the best from my point of view (that was actually my initial attempt before forking kfctl, but I figured out it didn't work because namespace need to be created at the beginning, patching additional label during resource creation may cause problems, e.g. deployments created before the istio injection label is added will not get sidecars).

But maybe we can have a way to let kfctl apply namespace from a resource file instead at the beginning, will think about this.

[jlewi]

namespace need to be created at the beginning, patching additional label during resource creation may cause problems, e.g. deployments created before the istio injection label is added will not get sidecars

If the order of operations is

• Create istio namespace with labels
• Create istio resources
• Create kubeflow namespace with labels
• Create kubeflow resources

So kubeflow resources are created after istio and if the namespace is labeled with istio side car injection then istio would already be running and side cars will be injected

[Bobgy]

@jlewi I agree the ordering looks logical, but that requires a sequential dependency between components and extra logic in kfctl (adding labels in namespaces etc.). I think it's hard to express that in KfDef if we want to go with #5

Alternatively, I'm thinking if we can extend the constraint of separation between cluster-scoped resources and namespace resources.

KfDef can have a set of components, some marked as cluster-scoped components.

When kfctl applies manifest, it will apply cluster-scoped components as the first thing after cluster is ready.
So the ordering is like:

• apply cluster-scoped resources (including new namespace resources)
• kfctl existing magic that adds a bunch of stuff
• apply namespace-scoped resources (like all existing components)

So we can put namespace resources into the cluster-scoped resources, extra labels/annotations there won't conflict with kfctl's existing logic and we don't need to let istio components deployed before some kfctl's existing logic.

These are my initial rough ideas (haven't tried with a demo). What do you think?

[jlewi]

The general direction we want to move is not putting more custom rollout logic into kfctl. We'd like to rely on existing tools to figure out the proper way to sequence objects. For example, I think kustomize, kpt, and ACM are already smart enough to deploy CRDs before creating instances of the resources.

I agree the ordering looks logical, but that requires a sequential dependency between components and extra logic in kfctl (adding labels in namespaces etc.).

I think what I'm proposing would eliminate custom logic in manifests. We would basically define "kustomize" packages 1 per namespace for each namespace we wanted to create. The labels would then be defined in the manifest as opposed to kfctl go code.

KFDef uses a list for the applications(kustomize manifests) so we just need to list the packages in the appropriate order.

As an example on GCP we want to support GitOps using ACM. ACM is

1. Oppinionated about repo structure
2. Only works with hydrated manifests (hydrated= fully rendered YAML)
3. Supports creation of namespaces and objects within those namespaces.

So using ACM the process would look something like

kfctl build ...  | <hydrate by running kustomize build> | <layout manifests according to ACM>

So apply ordering logic is handled by ACM.

So I think pulling the namespace definitions out of go code and putting them in YAML is a step in the right direction.

[Bobgy]

The general direction we want to move is not putting more custom rollout logic into kfctl.

Totally agree on the direction. If we remove all the kfctl custom rollout logic, we can use namespace resources right away, without parameters in kfdef about apply order.

I'm more concerned with how we can merge back the istio injection label before that, especially when kfctl is in the progress of removing the custom logic, how can namespace resources be applied at the right timing? Can we add hack like parsing the resources and apply namespaces first, before we remove all other kfctl custom rollout logic.

Because I don't think I will have bandwidth (and the knowledge) to contribute to refactoring kfctl, I'd like some feedback how we can push this forward. I'd prefer some workaround that can get this feature in without causing a problem for the long-term direction.

[Bobgy]

TODO for me:
I can test using kfctl-v1 with namespace resource, report what was the blocking issue back here.

[jlewi]

@Bobgy Sounds good. If you look at the logic;

kfctl/pkg/kfapp/kustomize/kustomize.go

Line 176 in 660fae1

for _, app := range kustomize.kfDef.Spec.Applications {

Applications are applied in the order they appear in KfDef.Application. So we just need to put the namespace applications at the right position in KFDef.Applications to control ordering.

[jlewi]

@Bobgy I've been experimenting with installing Kubeflow with kpt and kcc.
Here's my experiment
https://github.com/jlewi/kf-templates-gcp/blob/f3a4694c4daf2664230c6fc093ada744f65244cd/kubeflow/Makefile#L63

As you can see there I have a kustomize package which defines the namespaces. So any labels could just be added to the respective namespace resource.

That package is applied before applying anything that depends on those namespaces.

[Bobgy]

@Bobgy I've been experimenting with installing Kubeflow with kpt and kcc.
Here's my experiment
https://github.com/jlewi/kf-templates-gcp/blob/f3a4694c4daf2664230c6fc093ada744f65244cd/kubeflow/Makefile#L63

As you can see there I have a kustomize package which defines the namespaces. So any labels could just be added to the respective namespace resource.

That package is applied before applying anything that depends on those namespaces.

Thanks @jlewi! That looks great! Do we plan to release KF 1.1 gcp env with kpt and kcc approach?

Sorry I've been busy with some other stuff, didn't have time to look into this yet.

[jlewi]

@Bobgy yes I think for GCP and KF 1.1 kpt and kcc will be the recommended approach.

[Bobgy]

@jlewi I tried using kfctl 1.0.2 with an additional application containing just the namespace at the beginning of kfdef yaml file. Looks like it successfully patched kubeflow namespace to enable istio label.

Then let's go with this approach, sounds good?

[jlewi]

LGTM

[Bobgy]

/close
I think we can close this and keep tracking in kubeflow/kubeflow#3866

[k8s-ci-robot]

@Bobgy: Closing this issue.

In response to this:

/close
I think we can close this and keep tracking in kubeflow/kubeflow#3866

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.