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.

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

Add option to change port names #344

Closed
KarstenSiemer opened this issue Mar 1, 2022 · 4 comments
Closed

Add option to change port names #344

KarstenSiemer opened this issue Mar 1, 2022 · 4 comments

Comments

@KarstenSiemer
Copy link

KarstenSiemer commented Mar 1, 2022

Requirement - what kind of business use case are you trying to solve?

Hi there!
We are currently using Jaeger and want to add authentication and authorization to the UI.
We are doing this via istio service mesh and a dex/oauth2-proxy flow.

Problem - what in Jaeger blocks you from solving the requirement?

Sadly though, istio does not recognize the jaeger-query port as http and we need to prefix the
port name with http to force the selection to have rules for the traffic to apply.
The helm chart jaeger however, doesn't give an option to configure port names.

When some other protocol than http get's selected for the port, the http rules we supply
to activate the authentication flow never apply.

As a quick fix we deploy an additional service with the same selector to force selection of http, but
it would be nice if we could just configure the port name.

Proposal - what do you suggest to solve the problem or improve the existing situation?

query:
  service:
    annotations: {}
    type: ClusterIP
    # List of IP ranges that are allowed to access the load balancer (if supported)
    loadBalancerSourceRanges: []
    port: 80
    # Specify a custom port name
    portName: query
    # Specify a custom target port (e.g. port of auth proxy)
    # targetPort: 8080
    # Specify a specific node port when type is NodePort
    # nodePort: 32500

I'd like to give the service sections an interface to configure the port names via a name field.
The default value should be is currently hardcoded. Folks that use istio or something else can then just insert whatever they desire. Like query.service.portName: http-query

For us doing it on the query service would suffice, but we should follow through and make the name configurable on all services.
e.g agent:

  service:
    annotations: {}
    # List of IP ranges that are allowed to access the load balancer (if supported)
    loadBalancerSourceRanges: []
    type: ClusterIP
    # zipkinThriftPort :accept zipkin.thrift over compact thrift protocol
    zipkinThrift:
      name: zipkin-compact
      port: 5775
    # compactPort: accept jaeger.thrift over compact thrift protocol
    compact:
      name: jaeger-compact
      port: 6831
    # binaryPort: accept jaeger.thrift over binary thrift protocol
    binary:
      name: jaeger-binary
      port: 6832
    # samplingPort: (HTTP) serve configs, sampling strategies
    sampling:
      name: http
      port: 5778

Any open questions to address

  • We could also make this easier to configure by adding an istio options field and activate port naming convention and auto configure correct prefixes in this chart.
  • Another idea is to just hardcode istio portnaming convention, as the port names don't hurt people who are not using istio

Or we add maps for port options

query:
  service:
    annotations: {}
    type: ClusterIP
    # List of IP ranges that are allowed to access the load balancer (if supported)
    loadBalancerSourceRanges: []
    port: 
      name: query
      number: 80
    # Specify a custom target port (e.g. port of auth proxy)
    # targetPort: 8080
    # Specify a specific node port when type is NodePort
    # nodePort: 32500

If this feature request is accepted, I'd be happy to contribute a PR.
Thanks for the consideration

@cnvergence
Copy link
Contributor

cnvergence commented Apr 15, 2022

I am also interested in this feature.
@KarstenSiemer your idea with adding Istio port naming convention could be a good start for improving this.

@andreacoccodi
Copy link

I saw the changes to the ports http-config-rest and http-c-tchan-trft. In my case the only way to get rid of "Could not send spans over gRPC" is to rename the grpc-http port to http2-grpc (http-grpc would not work, tried that already). Although grpc is a recognised istio protocol and should behave as http2, it is actually not working. Any chance we could rename the grpc-http port or make it configurable?
Thanks

@Hronom
Copy link

Hronom commented Jun 9, 2023

Please do this also for collector-svc here https://artifacthub.io/packages/helm/jaegertracing/jaeger otherwise when you use Istio with mTLS mode strict it blocks sending traces from otlp with error unexpected EOF

@cnvergence
Copy link
Contributor

@Hronom I have opened the PR, you can track it here
#478
Although it is only renaming those ports in the helm charts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants