Create option to specify type for Query service

[Aneurysm9]

This change enables the specification of a service type for the query service created by a Jaeger resource. It addresses an issue noted by some commenters on #990, though I don't believe it fixes the original issue raised there.

[codecov]

Codecov Report

Merging #1132 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1132   +/-   ##
=======================================
  Coverage   88.26%   88.27%           
=======================================
  Files          89       89           
  Lines        5487     5491    +4     
=======================================
+ Hits         4843     4847    +4     
  Misses        474      474           
  Partials      170      170           

Impacted Files	Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go	100.00% <ø> (ø)
pkg/service/query.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7438f59...15bd265. Read the comment docs.

[jpkrohling]

While this might fix the current problem, I think the operator can try to come up with the required services to create, based on the environment. For instance, if we detect that we are running on AKS, we might return a list of services that include a NodePort service (in addition, or instead of another service). Hence my suggestion in the original issue to change the following to return a list of services:

jaeger-operator/pkg/service/query.go

Lines 12 to 53 in 1dce126

	// NewQueryService returns a new Kubernetes service for Jaeger Query backed by the pods matching the selector
	func NewQueryService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Service {
	trueVar := true
	annotations := map[string]string{}
	if jaeger.Spec.Ingress.Security == v1.IngressSecurityOAuthProxy {
	annotations["service.alpha.openshift.io/serving-cert-secret-name"] = GetTLSSecretNameForQueryService(jaeger)
	}
	return &corev1.Service{
	TypeMeta: metav1.TypeMeta{
	Kind: "Service",
	APIVersion: "v1",
	},
	ObjectMeta: metav1.ObjectMeta{
	Name: GetNameForQueryService(jaeger),
	Namespace: jaeger.Namespace,
	Labels: util.Labels(GetNameForQueryService(jaeger), "service-query", *jaeger),
	Annotations: annotations,
	OwnerReferences: []metav1.OwnerReference{
	metav1.OwnerReference{
	APIVersion: jaeger.APIVersion,
	Kind: jaeger.Kind,
	Name: jaeger.Name,
	UID: jaeger.UID,
	Controller: &trueVar,
	},
	},
	},
	Spec: corev1.ServiceSpec{
	Selector: selector,
	ClusterIP: "",
	Ports: []corev1.ServicePort{
	{
	Name: getPortNameForQueryService(jaeger),
	Port: int32(GetPortForQueryService(jaeger)),
	TargetPort: intstr.FromInt(getTargetPortForQueryService(jaeger)),
	},
	},
	},
	}
	}

Similar to what we have for the collector:

jaeger-operator/pkg/service/collector.go

Lines 15 to 21 in 1dce126

	// NewCollectorServices returns a new Kubernetes service for Jaeger Collector backed by the pods matching the selector
	func NewCollectorServices(jaeger *v1.Jaeger, selector map[string]string) []*corev1.Service {
	return []*corev1.Service{
	headlessCollectorService(jaeger, selector),
	clusteripCollectorService(jaeger, selector),
	}
	}

If it's a problem to have them created by default in all platforms, then flags could be added to control whether they should be created.

[Aneurysm9]

I've started down the path of allowing a list of ServiceType identifiers to be specified, and I can certainly make the service.NewQueryServices() function return a list of Service definitions, but there are a few places that assume there is a single query service that I'm not sure how to handle. The callers of service.NewQueryService() are already returning lists of Service definitions, so adapting them to receive a list of Service definitions isn't too hard.

The place I'm running into trouble is naming of the services and the places that use service.GetNameForQueryService() and expect a single result. Specifically, in the ingress.QueryIngress and route.QueryRoute types. Do you have suggestions for how to handle that, or places similar situations are handled I can reference?

[jpkrohling]

there are a few places that assume there is a single query service that I'm not sure how to handle

I think those are mostly tests, but if you have questions about concrete places, share the code and I'll take a look.

Do you have suggestions for how to handle that, or places similar situations are handled I can reference?

If the new service is meant to be used by external constructs, like ALB, then the presence of an ingress/route doesn't matter, right? In that case, I would leave them pointing to the current cluster IP service.

[Aneurysm9]

I think those are mostly tests, but if you have questions about concrete places, share the code and I'll take a look.

The specific places I'm looking at are the ingress and route packages.

If the new service is meant to be used by external constructs, like ALB, then the presence of an ingress/route doesn't matter, right? In that case, I would leave them pointing to the current cluster IP service.

The ingress is actually the important part for my use case. We're using the ALB Ingress Controller which requires NodePort (or LoadBalancer) services. This is part of why I assumed it would be more straightforward to allow specifying the ServiceType. As NodePort builds on ClusterIP, and LoadBalancer builds on NodePort, specifying a type with a broader scope should enable additional uses without limiting existing uses.

[jpkrohling]

The specific places I'm looking at are the ingress and route packages.

Makes sense, I thought you were talking about service.GetNameForQueryService()

The ingress is actually the important part for my use case. We're using the ALB Ingress Controller which requires NodePort (or LoadBalancer) services.

I'm almost convinced that allowing people to set the service type is a indeed a better approach, but allow me one last question: can we somehow identify that LoadBalancer is indeed the right default option for the current platform? Is there a way to identify that we are running on AWS? There's typically a CRD that we can check to detect the platform, like we do for OpenShift...

Second point to clarify before allowing users to set any kind of values to the service type: NodePort and LoadBalancer might be service types requiring extra permissions. We might want to prevent bad values from being set on platforms that require higher privileges and possibly write the status to the spec. At the very least, we should try to document the behavior and how to enable NodePort/LoadBalancer.

If you are able to clarify this second point, great. Otherwise, please do create an issue so that I can take a look at it later.

[Aneurysm9]

can we somehow identify that LoadBalancer is indeed the right default option for the current platform? Is there a way to identify that we are running on AWS?

Probably. For EKS at least there is node metadata that would indicate an EKS cluster. I don't think that's the right thing to be looking for, however. The determinant is the ingress controller in use. One could use the Nginx ingress controller on EKS and potentially be fine with a ClusterIP service. The operator could look for the existence of a kubernetes.io/ingress.class annotation on the Ingress definition to determine which ServiceType to use. I would worry about the maintenance load of that approach were I maintaining the operator, and might want more flexibility as a consumer to choose as-yet-unsupported ingress controllers, but I think I can update this PR to work in that manner if that's what you'd prefer.

To the second point, I'm not aware of any RBAC restrictions on ServiceType but I am definitely not an expert in that area and cannot speak authoritatively on the topic.

[jpkrohling]

Let's go with the simplest approach then, and figure out how to make the operator smarter in the future (ie: auto-detect EKS, like we can auto-detect OpenShift).

To the second point, I'm not aware of any RBAC restrictions on ServiceType but I am definitely not an expert in that area and cannot speak authoritatively on the topic.

I can try that on OpenShift and see how it goes.

[jpkrohling]

LGTM. I just tested it on OpenShift as well, and it looks good:

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: simplest
spec:
  query:
    serviceType: NodePort

$ kubectl get services simplest-query
NAME             TYPE       CLUSTER-IP     EXTERNAL-IP   PORT(S)         AGE
simplest-query   NodePort   172.25.70.75   <none>        443:31407/TCP   2m18s

[jpkrohling]

This is good to be merged, pending only a godoc for the possible values for the new property.

[jpkrohling]

Thank you for your contribution!