jaegertracing / jaeger-operator

Jaeger Operator for Kubernetes simplifies deploying and running Jaeger on Kubernetes.
https://www.jaegertracing.io/docs/latest/operator/
Apache License 2.0
1.03k stars 345 forks source link

[Question] Custom annotations at the service level #1096

Closed vassilvk closed 2 years ago

vassilvk commented 4 years ago

According to the documentation, custom annotations can be provided at the deployment component level, however it is not clear if and how custom annotations can be set at the service level. For example, when deploying Jaeger using production strategy, the operator creates a query service. Is there a way to set custom annotations for that service object through the Jaeger CR?

objectiser commented 4 years ago

Currently annotations are not configurable for the Service objects. Would you be interested in contributing a PR? The annotations would need to be managed separately from the current set, e.g. ServiceAnnotations.

vassilvk commented 4 years ago

@objectiser - thanks for the response!

I'm definitely interested in contributing, but my current workload would make that difficult.

For the sake of the discussion and clarity, when you say "separately from the current set, eg ServiceAnnotations", is this what you envision:

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  ...
spec:
  ...
  serviceAnnotations:
    query:
      my.annotations.io/a: abc
      my.annotations.io/b: xyz
jpkrohling commented 4 years ago

@vassilvk what's your concrete use case?

The annotations would need to be managed separately from the current set, e.g. ServiceAnnotations.

I'm sure you have a great reason for that, but may I ask "why"? Services could indeed have the same set of annotations/labels as deployments, can't they?

objectiser commented 4 years ago

@jpkrohling That was my suggestion - the problem is if we just apply the same set of annotations/labels, then there is no option to use them just for deployments or just services. Don't have concrete cases, but was thinking from a flexibility perspective?

jpkrohling commented 4 years ago

Unless we have a specific use case for this, I'd rather have one single set of labels/annotations for everything related to the component (query, ingester, collector, ...)

objectiser commented 4 years ago

Ok, I guess that will keep the CR simpler.

vassilvk commented 4 years ago

@jpkrohling - my use case has to do with my environment.

I am running a controller which listens to k8s and automatically registers k8s services with a Consul instance running on the same cluster. The k8s => Consul registration is driven by annotations on the k8s service objects.

There is a long list of downstream services which revolve around that Consul instance - one of them is a reverse proxy which dynamically routes traffic to services registered with Consul. I need access to the annotations of service jaeger-query in order for it to automatically appear behind that reverse proxy gateway.

I hope this makes it clear why setting the same annotations to all services exposed by the operator doesn't make sense when annotations are used to differentiate services and open them for downstream processing.

Please note that my workaround is to create another service which basically mimics the definition of jaeger-query, but allows me to set the annotations I need. This seems hacky to me, as I have a perfectly good service delivered to me by the operator, but I ignore it and run another one in parallel, because the one created by the operator is of no use to me.

@jpkrohling - I have seen your comments on other issues where you were concerned about exposing the services created by the operator into the jaeger custom resource definition. If I understand this correctly, your concern had to do with the fact that services are somehow internal to the workings of the operator and exposing them in the CR would be leaky.

I am not sure that I agree with the thinking that services are internal. What is the purpose of that jaeger-query service if not to be exposed to other parts of the system? And if exposed to other parts of the system, then obviously the system must know about that service and rely on its existence. In that respect, that service is not an internal object and maybe it should be considered an official part of the stack produced by the operator, hence subject to configuration through the CR.

jpkrohling commented 4 years ago

your concern had to do with the fact that services are somehow internal to the workings of the operator and exposing them in the CR would be leaky

Sort of. My concern is that we'll never be able to provide for 100% of the cases, and every new use case we provide for comes with a new complexity for everyone (new property in the CR, documentation, tests, maintenance, ...). For some use cases, I do believe that it's better to manage the service outside of the realm of the operator, either manually or via Helm/templating engine.

For this specific case, I believe it can be done today already, as we do allow annotations and labels to be specified for components (query, collector, ...), which then should propagate to all objects that are part of that (services, deployments, config maps, ...). Would this work for you?

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  ...
spec:
  ...
  query:
    annotations:
      my.annotations.io/a: abc
      my.annotations.io/b: xyz

I absolutely agree with you that the service itself is exposed to the whole cluster though.

vassilvk commented 4 years ago

@jpkrohling - yes, that would definitely work.

Unfortunately custom annotations are not propagated down to services (regardless of which strategy is used).

In addition, the documentation seems to be explicit about this. In section Understanding Custom Resource Definitions, the comment about the custom annotation example reads as follows:

<10> Define annotations to be applied to all deployments **(not services)**. These can be overridden by annotations defined on the individual components.
mwasilew2 commented 4 years ago

+1

We ( @Gitlab ) had a similar need for applying annotations to a service. We wanted to bind a BackendConfig object with the query service using annotations and our current workaround is to manage the service and ingress objects outside of operator. It would be nice if it supported that.

jpkrohling commented 4 years ago

I missed the notification for the last comment from @vassilvk, sorry about that. I think the skip using the custom annotations for services as it might have influence on the pod selector, but we can certainly work around that.

Anyone willing to give it a try?

vassilvk commented 4 years ago

I ended up replacing the extra service workaround with a KubeMod ModRule which intercepts the deployment of the service created by Jaeger Operator and patches it with the annotations I need before it is deployed to the cluster:

apiVersion: api.kubemod.io/v1beta1
kind: ModRule
metadata:
  name: jaeger-query-service-modrule
spec:
  type: Patch

  match:
    - select: '$.kind'
      matchValue: Service
    - select: '$.metadata.labels.app'
      matchValue: jaeger
    - select: '$.metadata.labels["app.kubernetes.io/component"]'
      matchValue: service-query

  patch:
    - op: add
      path: /metadata/annotations
      value: |-
        my-annotation-1: 'my-annotation-1-value'
        my-annotation-2: 'my-annotation-2-value'
jpkrohling commented 4 years ago

I didn't know about KubeMod, that's a cool trick!

vassilvk commented 4 years ago

Thanks! KubeMod is pretty new - in fact I just released its first beta last weekend :)

lmickh commented 4 years ago

In additional to some of the use cases already mentioned, it is also helpful to pass the labels down to the services so that they can be found by the prometheus-operator when it is configured to filter by labels. The jaeger-operator-metrics service and servicemonitor for example will not function if your Prometheus config uses label matching for service discovery. There isn't much in the way of metrics right now, but I would think that is going to be expanded on in the future.

herbguo commented 3 years ago

Hi everybody, I have commited a PR1526 to reuse the annotation to the service. And this way can keep the CRD not to change.

JesperBerggren commented 3 years ago

Hello everybody,

Another use case is if setting the global Istio mesh option for stricter security:

defaultServiceExportTo: ["."]

Then it gets necessary to set the following annotation to the Jaeger services (or perhaps even wider as in "*"):

annotations:
    networking.istio.io/exportTo: "istio-system"

How's the progress with #1526 ? Isn't time to review it and get it moving?

Best regards Jesper Berggren

JesperBerggren commented 2 years ago

Hi again,

This feature is quite necessary according to the use case described above.

The only way to get access to the jaeger-query UI is either by using some kind of mutation (e.g. KubeMod as described above) or by patching the jaeger-query service manually after deployment. Both ways are "ugly" and hard to maintain.

What is the status of the PR that @herbguo made?

Best regards Jesper Berggren

herbguo commented 2 years ago

Hi again,

This feature is quite necessary according to the use case described above.

The only way to get access to the jaeger-query UI is either by using some kind of mutation (e.g. KubeMod as described above) or by patching the jaeger-query service manually after deployment. Both ways are "ugly" and hard to maintain.

What is the status of the PR that @herbguo made?

Best regards Jesper Berggren

Hi, The PR I submitted is still in the review state, and there is no progress

Maximebb commented 2 years ago

This seems already on its way to completion, but to add to the use cases:

We run a GKE cluster that centralizes traces with jaeger. The collector service needs to be available to workloads in the same private network, so we want to make it an internal load balancer. Google cloud supports this via annotation on a service. The current workaround to duplicate the service definition is fine, but a cleaner solution would be to use the collector annotations.

JesperBerggren commented 2 years ago

Hello,

I've been waiting for the PR and I'm excited that it's finally here. BUT I cannot get the service annotations to work. Can you please let me know what the magic formula is?

I'm using chart version 2.34.0 with the following values file:

webhooks:
  service:
    annotations:
      networking.istio.io/exportTo: "istio-system"

service:
  type: ClusterIP
  annotations:
    networking.istio.io/exportTo: "istio-system"

rbac:
  clusterRole: true

securityContext:
  runAsNonRoot: true

I would expect that the annotation: networking.istio.io/exportTo: "istio-system" would end up on jaeger-query and jaeger-collector as described in the PR but it's only applied to jaeger-operator-webhook-service and jaeger-operator-metrics.

Could you please let me know what I need to do?

Thanks in advance.

Best regards Jesper Berggren

JesperBerggren commented 2 years ago

Never mind.

It's on the Jaeger CRD, of course.

Setting:

spec:
  query:
    annotations:
      networking.istio.io/exportTo: "istio-system"

Did the trick!

Thanks for the fix.

Best regards Jesper Berggren