embedded controller manifests break coupling to upstream controllers

[hellt]

Hi, With #221 merged, I am worried about who is going to unify the embedded manifests with the ones from the upstream repo.

For example, the embedded controller manifest for srlinux-controller contains the outdated controller image version https://github.com/openconfig/kne/blob/7e017bf8866d87e0c81f9fdd1d4ab2e485c23875/manifests/controllers/srlinux/manifest.yaml#L397 Current version is 0.4.3.

Basically any change in the upstream controller spec will require someone to update the generated manifest for the said controller...

Keeping track of what has changed upstream may be unbearable, why don't we instruct kne deploy to use the upstream installation procedures instead? For example, deploying srl-controller can be as simple as shelling out and calling kubectl apply -k <URL>.

This is of course a brute approach, which requires a cluster to have internet access for kustomize to fetch artifacts from a git repository of srl-controller. But this seems to be what users outside of Google will use all the time. For internal use cases, the checked-in manifest might be the only option, but still, I don't think it should be a default way of deploying 3rd party controllers to KNE.

[alexmasi]

At the time of merging that PR, 0.4.3 was causing SR Linux node creation to fail (could it be due to needing a newer srlinux container image? we have not begun automated ingestion/validation of srlinux internally yet so that could be the issue) so I specified the version we have been testing at internally.

The idea is just like the other dependencies (metallb, meshnet) we do not pull from head but we maintain copies of the manifests that are guaranteed to work with KNE. When we need a new feature we can roll forward to newer versions. If you have a new feature you'd like to introduce into KNE through the srl-controller you can submit a PR updating this default manifest. In the docs update thats coming I can point out to users that if they desire controllers at head then they can pull from the controller repo directly and apply that configuration instead.

Internally we actually have a supporting repo with other versions of the manifests, so that is not the issue here.

But I also understand that srl-controller can be applied at specific versions: kubectl apply -k https://github.com/srl-labs/srl-controller/config/default?ref=v0.3.1

@marcushines to further comment, we have yet to fully outline the controller container/manifest + router container ingestion/validation process both internally and externally so this is valuable discussion

[marcushines]

so the deployment of kne should be hermetic to itself. All versions of dependencies should be well known and tracked so they can be tested in our CI

With controllers there is a contract between controller version and node as the node generally uses a clientset to publish the needed crds. so the deployed controller has to align with clientset

[hellt]

Yes, I see the positive sides of fixing the dependencies. What if we add to the controllers spec an option to provide a customization file?

something like

cluster:
  kind: Kind

ingress:

cni:

controllers:
  - kind: SRLinux
    spec:
      kustomization: "https://github.com/srl-labs/srl-controller/config/default"

with such controller spec KNE will invoke kuectl apply -k .controllers[0].spec.kustomization and effectively install the controller from the upstream?

[marcushines]

hmm... that doesn't offend me but let's talk about this a bit more

[hellt]

This is not relevant anymore, as now the process of updating the manifests is clear (just kustomize build ./config/default from the kubebuilder-styled repo)