Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create option to specify type for Query service #1132

Merged
merged 4 commits into from
Aug 18, 2020
Merged

Create option to specify type for Query service #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9dc854
Allow specifying ServiceType for Query service
Aneurysm9 Jul 14, 2020
3b497ee
Merge branch 'master' of github.com:jaegertracing/jaeger-operator int…
Aneurysm9 Aug 17, 2020
ed1afa2
Add godoc to new ServiceType field in JaegerQuerySpec
Aneurysm9 Aug 17, 2020
15bd265
Fix field name in godoc
Aneurysm9 Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions deploy/crds/jaegertracing.io_jaegers_crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5544,6 +5544,8 @@ spec:
type: object
serviceAccount:
type: string
serviceType:
type: string
tolerations:
items:
properties:
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/jaegertracing/v1/jaeger_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ type JaegerQuerySpec struct {

// +optional
JaegerCommonSpec `json:",inline,omitempty"`

// +optional
Aneurysm9 marked this conversation as resolved.
Show resolved Hide resolved
// ServiceType represents the type of Service to create.
// Valid values include: ClusterIP, NodePort, LoadBalancer, and ExternalName.
// The default, if omitted, is ClusterIP.
// See https://kubernetes.io/docs/concepts/services-networking/service/#publishing-services-service-types
ServiceType v1.ServiceType `json:"serviceType,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jpkrohling jpkrohling Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// JaegerUISpec defines the options to be used to configure the UI
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/jaegertracing/v1/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 9 additions & 2 deletions pkg/service/query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ func NewQueryService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Serv
},
},
Spec: corev1.ServiceSpec{
Selector: selector,
ClusterIP: "",
Selector: selector,
Type: getTypeForQueryService(jaeger),
Ports: []corev1.ServicePort{
{
Name: getPortNameForQueryService(jaeger),
Expand Down Expand Up @@ -83,3 +83,10 @@ func getTargetPortForQueryService(jaeger *v1.Jaeger) int {
}
return 16686
}

func getTypeForQueryService(jaeger *v1.Jaeger) corev1.ServiceType {
if jaeger.Spec.Query.ServiceType != "" {
return jaeger.Spec.Query.ServiceType
}
return corev1.ServiceTypeClusterIP
}
37 changes: 36 additions & 1 deletion pkg/service/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"testing"

"github.com/stretchr/testify/assert"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"

Expand All @@ -22,7 +24,8 @@ func TestQueryServiceNameAndPorts(t *testing.T) {
assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port)
assert.Equal(t, "http-query", svc.Spec.Ports[0].Name)
assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort)
assert.Len(t, svc.Spec.ClusterIP, 0) // make sure we get a cluster IP
assert.Len(t, svc.Spec.ClusterIP, 0) // make sure we get a cluster IP
assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeClusterIP) // make sure we get a ClusterIP service
}

func TestQueryDottedServiceName(t *testing.T) {
Expand All @@ -49,3 +52,35 @@ func TestQueryServiceNameAndPortsWithOAuthProxy(t *testing.T) {
assert.Equal(t, "https-query", svc.Spec.Ports[0].Name)
assert.Equal(t, intstr.FromInt(8443), svc.Spec.Ports[0].TargetPort)
}

func TestQueryServiceNodePortWithIngress(t *testing.T) {
name := "TestQueryServiceNodePortWithIngress"
selector := map[string]string{"app": "myapp", "jaeger": name, "jaeger-component": "query"}

jaeger := v1.NewJaeger(types.NamespacedName{Name: name})
jaeger.Spec.Query.ServiceType = corev1.ServiceTypeNodePort
svc := NewQueryService(jaeger, selector)

assert.Equal(t, "testqueryservicenodeportwithingress-query", svc.ObjectMeta.Name)
assert.Len(t, svc.Spec.Ports, 1)
assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port)
assert.Equal(t, "http-query", svc.Spec.Ports[0].Name)
assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort)
assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeNodePort) // make sure we get a NodePort service
}

func TestQueryServiceLoadBalancerWithIngress(t *testing.T) {
name := "TestQueryServiceNodePortWithIngress"
selector := map[string]string{"app": "myapp", "jaeger": name, "jaeger-component": "query"}

jaeger := v1.NewJaeger(types.NamespacedName{Name: name})
jaeger.Spec.Query.ServiceType = corev1.ServiceTypeLoadBalancer
svc := NewQueryService(jaeger, selector)

assert.Equal(t, "testqueryservicenodeportwithingress-query", svc.ObjectMeta.Name)
assert.Len(t, svc.Spec.Ports, 1)
assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port)
assert.Equal(t, "http-query", svc.Spec.Ports[0].Name)
assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort)
assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeLoadBalancer) // make sure we get a LoadBalancer service
}