Refactor kubernetes/istio.go to use istio.io/client-go

[lucasponce]

Part of #1372 effort.
Depends on #4388.

This is WIP and experimental, just using this draft to scope the changes and implications of the refactoring.

Note, it's expected this PR will mutate.

Strategy plan for this work during this week:

• Migrate kubernetes.IstioObject generics to proper istio.io classes.
• Resolve low code level conflicts: split a generic into different classes, move some IstioObject functions as helpers/aux functions.
• Milestone 1: to have a build with new classes.
• Review test cases, ensure no regression is performed in logic.
• Milestone 2: test cases are migrated and working.
• Review parsing/marshall scenarios with the UI, potential regression in the protobuff marshalls (new types store enums and some string -> enum could fail).
• Milestone 3: backend + frontend work.

Main rules followed during this work:

• Avoid temptation to refactor design, code involved is old, and things can be improved, but the goal of this work is scoped to the migration of IstioObject types, otherwise, it's pretty easily to jump into recursive refactoring.
• Perform minimal changes in the refactor, even if those can be optimized later, keep current logic is the priority here.

[xeviknal]

I have reviewed all the files now.
I do have a comment about the usage of the istio-client that I'd like to hear from you lucas.
I see that you did the minimal changes as possible to keep all the logic unchanged. Just removing the constant parsing and casting and adapting the necessary methods to support the strong typing.

[xeviknal]

I think this could be removed. If len == 0 the range will return no rules an it will end up returning the very same values.

[xeviknal]

Oh I see. One trade off with having the strong types for this client is that we don't have a generic interface for this.
It would be amazing to have interface similar to IstioObject or IstioSpec for getting the shared fields between configs.

[xeviknal]

This usage of the context makes me think about how we are consuming the istio client.
I see there aren't references in the business layer about context in the main branch. I guess that is because all the usages of the kubernetes client are encapsulated inside the kubernetes packages and under one interface K8SClientInterface.

Shouldn't we do the same here? Prior to this PR all the istio objects were got from the istio client interface.
Something like:

type IstioClientInterface interface {
	GetVirtualService(namespace, name string) (networking_v1alpha3.VirtualService, error)
	GetDestinationRule(namespace, name string) (networking_v1alpha3.DestinationRule, error)
....
	GetProxyStatus() ([]*ProxyStatus, error)
	GetConfigDump(namespace, podName string) (*ConfigDump, error)
	SetProxyLogLevel(namespace, podName, level string) error
	GetRegistryStatus() ([]*RegistryStatus, error)
}

The K8SClientInterface looks like this:

kiali/kubernetes/kubernetes.go

Lines 31 to 63 in 76389a6

	type K8SClientInterface interface {
	ForwardGetRequest(namespace, podName string, localPort, destinationPort int, path string) ([]byte, error)
	GetClusterServicesByLabels(labelsSelector string) ([]core_v1.Service, error)
	GetConfigMap(namespace, name string) (*core_v1.ConfigMap, error)
	GetCronJobs(namespace string) ([]batch_v1beta1.CronJob, error)
	GetDaemonSet(namespace string, name string) (*apps_v1.DaemonSet, error)
	GetDaemonSets(namespace string) ([]apps_v1.DaemonSet, error)
	GetDeployment(namespace string, name string) (*apps_v1.Deployment, error)
	GetDeployments(namespace string) ([]apps_v1.Deployment, error)
	GetDeploymentConfig(namespace string, name string) (*osapps_v1.DeploymentConfig, error)
	GetDeploymentConfigs(namespace string) ([]osapps_v1.DeploymentConfig, error)
	GetEndpoints(namespace string, name string) (*core_v1.Endpoints, error)
	GetJobs(namespace string) ([]batch_v1.Job, error)
	GetNamespace(namespace string) (*core_v1.Namespace, error)
	GetNamespaces(labelSelector string) ([]core_v1.Namespace, error)
	GetPod(namespace, name string) (*core_v1.Pod, error)
	GetPodLogs(namespace, name string, opts *core_v1.PodLogOptions) (*PodLogs, error)
	GetPods(namespace, labelSelector string) ([]core_v1.Pod, error)
	GetPodPortForwarder(namespace, podName, portMap string) (*httputil.PortForwarder, error)
	GetReplicationControllers(namespace string) ([]core_v1.ReplicationController, error)
	GetReplicaSets(namespace string) ([]apps_v1.ReplicaSet, error)
	GetSecret(namespace, name string) (*core_v1.Secret, error)
	GetSecrets(namespace string, labelSelector string) ([]core_v1.Secret, error)
	GetSelfSubjectAccessReview(namespace, api, resourceType string, verbs []string) ([]*auth_v1.SelfSubjectAccessReview, error)
	GetService(namespace string, name string) (*core_v1.Service, error)
	GetServices(namespace string, selectorLabels map[string]string) ([]core_v1.Service, error)
	GetServicesByLabels(namespace string, labelsSelector string) ([]core_v1.Service, error)
	GetStatefulSet(namespace string, name string) (*apps_v1.StatefulSet, error)
	GetStatefulSets(namespace string) ([]apps_v1.StatefulSet, error)
	GetTokenSubject(authInfo *api.AuthInfo) (string, error)
	UpdateNamespace(namespace string, jsonPatch string) (*core_v1.Namespace, error)
	UpdateService(namespace string, name string, jsonPatch string) error
	UpdateWorkload(namespace string, name string, workloadType string, jsonPatch string) error

I feels that it would be a bit more consistent with the existent layer split that we have been carrying so far. Business only deals with business logic but the kubernetes, prom, jaeger packages are responsible to connect with their respective backends.

[lucasponce]

The context was not used as a method to pass parameters from upper layers, so internally it was using an empty context, similar as it is being used now.

I see the temptation to go deep into the refactoring, but the scope of the work was limited to the Istio layer on purpose.
I put a disclaimer in the description of the PR:

Yes, there are chances that this logic can be revisited and optimized, but I don't think this PR is the proper place, or there is an inmediate hurry on it.

I'll prepare future issues for it, but at least with this PR we can make a big move removing a couple of technical debt with jaeger (already) and istio deps.

As we have some planned work to introduce the registry as an alternative method to fetch kubernetes information, probably we can address some of these maintenance tasks during that effort.

[xeviknal]

Avoid temptation to refactor design

I am not saying to refactor what we had before, I am saying that there is a change in the design with regards to what we had before this PR. All the methods to fetch Istio objects were living in the kubernetes layer:

kiali/kubernetes/istio.go

Lines 35 to 39 in b0d9b8f

	CreateIstioObject(api, namespace, resourceType, json string) (IstioObject, error)
	DeleteIstioObject(api, namespace, resourceType, name string) error
	GetIstioObject(namespace, resourceType, name string) (IstioObject, error)
	GetIstioObjects(namespace, resourceType, labelSelector string) ([]IstioObject, error)
	UpdateIstioObject(api, namespace, resourceType, name, jsonPatch string) (IstioObject, error)

And all the API calls are hidden for the business logic:

kiali/business/istio_config.go

Line 135 in b0d9b8f

gg, ggErr = in.k8s.GetIstioObjects(criteria.Namespace, kubernetes.Gateways, criteria.LabelSelector)

And now, the business layer is the one calling the Istio Client straigthforwardly.

kiali/business/istio_config.go

Lines 143 to 145 in b4168c8

	drl, e := in.k8s.Istio().NetworkingV1alpha3().DestinationRules(criteria.Namespace).List(ctx, listOpts)
	istioConfigList.DestinationRules = drl.Items
	err = e

This new way, makes the business layer know about how to declare contexts, listOptions or getOptions which is clearly logic regarding to the storage and communication with kubernetes. I think I like the idea of not having to initialize contexts, listOptions, getOptions and, in general, don't need to know about the istio-cli specifics in the business layer.

On the other hand, moving the usage of the istio-cli in the k8s pkg makes changes in the methods easier: if you need to filter some istio objects, e.g. Sidecars, by labels and you have already a GetSidecars(name,namespace) method, adding one more param into the method is trivial GetSidecars(name,namespace, labels) and helps others re-use that code.
Otherwise, I can imagine the situation where someone adds helpers to deal with this: getListOptionsByLabels(labels)(listOption, context). I also see it easier that people hesitate about which context to use: Background or TODO? Before this change, that was clear and handled in the kubernetes client while declaring the kubernetes client.

See how we are doing for the kubernetes calls:

kiali/business/istio_status.go

Line 262 in 8ae1a5b

istiods, err := iss.k8s.GetPods(cfg.IstioNamespace, labels.Set(map[string]string{"app": "istiod"}).String())

and how are defined in the interface:

kiali/kubernetes/kubernetes.go

Lines 31 to 48 in 76389a6

	type K8SClientInterface interface {
	ForwardGetRequest(namespace, podName string, localPort, destinationPort int, path string) ([]byte, error)
	GetClusterServicesByLabels(labelsSelector string) ([]core_v1.Service, error)
	GetConfigMap(namespace, name string) (*core_v1.ConfigMap, error)
	GetCronJobs(namespace string) ([]batch_v1beta1.CronJob, error)
	GetDaemonSet(namespace string, name string) (*apps_v1.DaemonSet, error)
	GetDaemonSets(namespace string) ([]apps_v1.DaemonSet, error)
	GetDeployment(namespace string, name string) (*apps_v1.Deployment, error)
	GetDeployments(namespace string) ([]apps_v1.Deployment, error)
	GetDeploymentConfig(namespace string, name string) (*osapps_v1.DeploymentConfig, error)
	GetDeploymentConfigs(namespace string) ([]osapps_v1.DeploymentConfig, error)
	GetEndpoints(namespace string, name string) (*core_v1.Endpoints, error)
	GetJobs(namespace string) ([]batch_v1.Job, error)
	GetNamespace(namespace string) (*core_v1.Namespace, error)
	GetNamespaces(labelSelector string) ([]core_v1.Namespace, error)
	GetPod(namespace, name string) (*core_v1.Pod, error)
	GetPodLogs(namespace, name string, opts *core_v1.PodLogOptions) (*PodLogs, error)
	GetPods(namespace, labelSelector string) ([]core_v1.Pod, error)

and how are implemented: pretty straightforward but hidding the logic of consuming the istio client.

kiali/kubernetes/kubernetes.go

Lines 373 to 385 in 76389a6

	// GetPods returns the pods definitions for a given set of labels.
	// An empty labelSelector will fetch all pods found per a namespace.
	// It returns an error on any problem.
	func (in *K8SClient) GetPods(namespace, labelSelector string) ([]core_v1.Pod, error) {
	// An empty selector is ambiguous in the go client, could mean either "select all" or "select none"
	// Here we assume empty == select all
	// (see also https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors)
	if pods, err := in.k8s.CoreV1().Pods(namespace).List(in.ctx, meta_v1.ListOptions{LabelSelector: labelSelector}); err == nil {
	return pods.Items, nil
	} else {
	return []core_v1.Pod{}, err
	}
	}

[lucasponce]

I am not saying to refactor what we had before, I am saying that there is a change in the design with regards to what we had before this PR. All the methods to fetch Istio objects were living in the kubernetes layer:

True.

This PR proposes to change that design and consolidate the api contract of fetching the Istio objects on the business layer, in the IstioConfig service. Before, the validations, tls, or other logic queried directly the kubernetes API, now those services use directly the business.IstioConfig, and inside this logic the istio.io/client-go api is called.

The Kubernetes contract is not touched yet and there is still a mix (to not make this PR even bigger), also in the uncoming efforts both validations and services logic would require some changes to accomodate better the service info fetched directly from the Istio registry, so, in short, the current kubernetes/kubernetes.go interface would also change.

Also, note that filters logic has been consolidated as well under the kubernetes/filters.go.

This design is not definitive in any case, the aim of this PR was to introduce the new library without introducing regressions. Any of these suggestions can be also added as soon as we work in the next steps of improving the validations and services with the Istio registry information.

Sorry if my previous comment was not clear enough.

[xeviknal]

Yep, that is a pitty that there is not a generic interface in this istio client. Because it could help reduce methods that exact the same but returning different types.

[lucasponce]

It could return a runtime.Object but we'd be in the same situation to force a cast or reflection to check which type is using.
In this first pass I didn't want to change the design, just moving from dynamic to static type for the cases I found.

[hhovsepy]

Approved. Locally debugged the validation code and tests.
Regression tested.
All validations logic is kept.

[lucasponce]

@xeviknal did you check if there is any regression in the code from your side ?

[lucasponce]

This work will have two important conflicts with PRs:
#4387
#4409

So, I'd prefer if those are merged first and I resolve the potential conflicts on this PR, rather than let the authors to solve the problems, but not sure if those are ready to merge.

Please, @hhovsepy @xeviknal let me know, let's try to progress on consolidate this to unblock other efforts.

[xeviknal]

LGTM! I have manually tested all the kiali.io validation examples and also the appenders.
I did code-review every single changed. Tests are passing. I think we are good to go.
Tough job!

[jshaughn]

I have visually looked at the graph-related changes and I don't see any issues. The updated tests are passing and the new supporting code in models, looks good. I'm +1 to a merge so we can get some soak time before the next release.