Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Push all k8s clients into a layer of clients #4863

Closed
steeling opened this issue Jun 30, 2022 · 4 comments · Fixed by #5185
Closed

Push all k8s clients into a layer of clients #4863

steeling opened this issue Jun 30, 2022 · 4 comments · Fixed by #5185
Assignees
Labels
kind/refactor Related to code refactor priority/P1 P1 priority size/XL 20 days (4 weeks)
Milestone

Comments

@steeling
Copy link
Contributor

steeling commented Jun 30, 2022

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 steeling added the kind/refactor Related to code refactor label Jun 30, 2022
@steeling steeling added this to the vFuture milestone Jun 30, 2022
@steeling
Copy link
Contributor Author

steeling commented Jul 1, 2022

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

@github-actions
Copy link

github-actions bot commented Jul 8, 2022

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

@steeling steeling added size/XL 20 days (4 weeks) priority/P1 P1 priority and removed size/needed labels Jul 15, 2022
@trstringer trstringer modified the milestones: vFuture, v1.3 Jul 19, 2022
@steeling steeling moved this to Todo in V1.3 Refactoring Jul 20, 2022
@allenlsy allenlsy moved this from Todo to In Progress in V1.3 Refactoring Sep 8, 2022
@allenlsy allenlsy self-assigned this Sep 12, 2022
@allenlsy allenlsy moved this to In Progress in OSM Roadmap (dev) Sep 12, 2022
@steeling steeling moved this from In Progress to Done in V1.3 Refactoring Sep 26, 2022
@steeling steeling moved this from Done to In Progress in V1.3 Refactoring Sep 26, 2022
@allenlsy
Copy link
Contributor

@steeling can we close this issue now?

@steeling
Copy link
Contributor Author

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

Repository owner moved this from In Progress to Done in V1.3 Refactoring Oct 5, 2022
Repository owner moved this from In Progress to Done in OSM Roadmap (dev) Oct 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/refactor Related to code refactor priority/P1 P1 priority size/XL 20 days (4 weeks)
Projects
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants