Drop dynamic wrapped client generation

[dprotaso]

/assign @dprotaso

We have lots of code generation to support using a dynamic client driving a typed kubernetes interface. This was introduced in #2210 It's not clear if this is really being used anywhere.

What we do know is

• it's not fully implemented (bunch of panics in the code)
• it causes friction when updating k8s deps
• it prevents downstream implementations from bumping their k8s version to a higher one
  -- Kubernetes 1.26/1.27 support #2725

I propose we drop this logic from knative/pkg

The only use of the dynamic injection I found was tektoncd/triggers#1207 but they don't seem to use the wrapped clients

[dprotaso]

cc @mattmoor in case you have more references to usages

[mattmoor]

We use this downstream to inject clients that exhibit different behavior.

cc @wlynch who I know spent a bunch of time looking at improving things a few weeks ago to streamline updates.

[dprotaso]

exhibit different behavior.

In what way?

We use this downstream...

I'm thinking of making the generation optional - so you can just generate these yourself downstream

[mattmoor]

@dprotaso I think for downstream codegen to work, the clients would need to be in their own go module, so that folks can go mod -replace with additional options.

[dprotaso]

I don't understand - can you elaborate?

I'm suggesting downstream add a generation script like so

pkg/hack/update-codegen.sh

Lines 46 to 54 in db8a353

	OUTPUT_PKG="knative.dev/pkg/client/injection/kube" \
	VERSIONED_CLIENTSET_PKG="k8s.io/client-go/kubernetes" \
	EXTERNAL_INFORMER_PKG="k8s.io/client-go/informers" \
	${REPO_ROOT_DIR}/hack/generate-knative.sh "injection" \
	k8s.io/client-go \
	k8s.io/api \
	"${K8S_TYPES}" \
	--go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt \
	--force-genreconciler-kinds "Namespace,ConfigMap,Deployment,Secret,Pod,CronJob,NetworkPolicy,Node,ValidatingWebhookConfiguration,MutatingWebhookConfiguration"

But add a flag --force-dynamic-clients or something to get the extra dynamic stuff

[dprotaso]

So looking at all the downstream packages that depend on injection.Dynamic I only see registration of dynamic clients and informers (due to codegen/cargo culting with no actual use).

The only downstream dependency that seems to setup the context with a dynamic client is tekton triggers

cache/github.com/tektoncd/triggers@v0.24.0/cmd/eventlistenersink/main.go:	ctx = injection.Dynamic.SetupDynamic(ctx)

But then they setup the injection.Default Informers and clients overriding everything. So injection.Dynamic doesn't really have any usage.

Also based on #2210

One of the long-standing motivations for the injection-based controller architecture has been to enable us to synthesize different backing implementations, which can be made available to controller constructors by substituting what foo.Get(ctx) accesses.

I agree with this statement but I would envision the implementation to just use the same object graph as injection.Default vs. a separate one

ie. I would setup things this way

import "context"
import "k8s.io/client-go/dynamic"
import "knative.dev/pkg/client/injection/kube/client"
import "knative.dev/pkg/injection/clients/dynamicclient"

func main() {
  ctx := context.Background()
  dc := dynamic.NewForConfigOrDie(cfg)

 ctx = context.WithValue(ctx, dynamicclient.Key{}, dynamic.NewForConfigOrDie(cfg))
 ctx = context.WithValue(ctx, client.Key{}, myOwnKubernetesWrapper{dc})

 ... := injection.Default.SetupInformers(ctx)
}

type myOwnKubernetesWrapper struct {
   dc *dynamic.Client
}

var _ kubernetes.Interface = (*myOwnKubernetesWrapper)(nil)

Based on all this I'm inclined to delete the wrapped client, the informers, and injection.Dynamic - it should be fairly trivial to just remove registrations downstream (cause it's all done via codegen)

[pierDipi]

Thanks for the assessment @dprotaso !

[vdemeester]

Arf, this was used in tektoncd/pipeline 😅