osm icon indicating copy to clipboard operation
osm copied to clipboard

Push all k8s clients into a layer of clients

Open steeling opened this issue 2 years ago • 2 comments

We currently have ~5 flavors of kubernetes clients:

  • k8s.Controller
  • generated "controller" clients for our CRD's
  • A large informerCollection type
  • a kube.Client
  • kubernetes.Interface

Instead, we should leverage "layers" of clients, such that our business logic is only aware of 1 layer. This might looks like:

// K8sAPIClient is an abstraction of the K8s API. It has inputs of OSM objects and returns OSM objects.
// It performs the translations, and makes calls to the underlying K8sClient.
type K8sInfraClient struct {
   client K8sClient
}

// K8sClient is aware of k8s types, and has inputs/outputs of k8s objects. It may leverage the informer cache
type K8sClient struct {
   informerCollection
}

As we do this, no functions should accept a client object just to make a call to that client. It should only be accepted for dependency injection. An example of this includes GetKubernetesServerVersionNumber. Instead, this should be called GetServerVersionNumber, and be a method on a k8s client.

This improves our ability to decouple from k8s, and leverage interfaces. Please take a look at the guiding principles section for how we can better leverage interfaces.

The new layer for access to these clients will be:

  1. MeshCatalog a) Contains business logic for constructing the trafficpolicy types b) Has no concept of anything related to K8s c) Has an embedded compute.Interface
  2. The compute.Interface a) Only accepts/returns non-k8s type objects. (The exception to this is custom objects where we do not have an analogous OSM type b) Performs translations to/from k8s type objects c) There may be multiple instances of this, which is how we can interface outside of k8s.
  3. The k8s.Client a) Basic Get/Set/Update to kubernetes API c) Maintains the informer cache d) Has a client for each k8s kind e) Is only aware of k8s types, is not aware of OSM types

Some nice results:

  1. Eventually the MeshCatalog will only contain business logic, so its interface can go away, and any mocks can supply a mock compute.Interface to a real MeshCatalog instance. This will improve our testing of business logic in the MeshCatalog.
  2. With the combination of the Builder pattern, which reduces the need for the mesh catalog, and the potential deconstruction of trafficpolicy types, the mesh catalog may be able to be removed completely. This direction should become more clear in the future.

contains business logic

steeling avatar Jun 30 '22 13:06 steeling

recording an example from a recent code review, of the unforunate way in which we must currently construct these clients:

	kubeClient := k8sClientFake.NewSimpleClientset()
	configClient := configFake.NewSimpleClientset()
	policyClient := policyFake.NewSimpleClientset()
	informerCollection, err := informers.NewInformerCollection(tests.MeshName, stop,
		informers.WithKubeClient(kubeClient),
		informers.WithConfigClient(configClient, tests.OsmMeshConfigName, tests.OsmNamespace),
	)
	if err != nil {
		b.Fatalf("Failed to create informer collection: %s", err)
	}
	kubeController := k8s.NewKubernetesController(informerCollection, policyClient, msgBroker)
	policyController := policy.NewPolicyController(informerCollection, kubeController, msgBroker)
	osmConfigurator := configurator.NewConfigurator(informerCollection, tests.OsmNamespace, tests.OsmMeshConfigName, msgBroker)
	kubeProvider := kube.NewClient(kubeController, osmConfigurator)

This is all to create objects that send HTTPs requests to the same API server. These should be consolidated into a single client for ease of use. Particularly, there are more clients we have access to in the code, and this is not the worst it could be

steeling avatar Jul 01 '22 16:07 steeling

Added default label size/needed. Please consider re-labeling this issue appropriately.

github-actions[bot] avatar Jul 08 '22 00:07 github-actions[bot]

@steeling can we close this issue now?

allenlsy avatar Sep 30 '22 13:09 allenlsy

I have one final PR i'll have out early next week

steeling avatar Sep 30 '22 15:09 steeling