-
Notifications
You must be signed in to change notification settings - Fork 345
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
Allow specifying ingress host #135
Comments
@objectiser thoughts? I think we talked a bit about having the ingress as a child of the Query spec, and decided to have it as a top-level because we have only one ingress at the moment. If we want to have this ingress at the top-level in the long term, then I'm OK with the approach proposed by @mihau. |
@jpkrohling yes I think the ingress at the top level is fine for now - as it is currently defined. I'm also ok with the approach suggested in this issue, the only question is how this would impact the |
They would be incompatible, as IIRC, the Ingress code is already ignored when |
As shown in this example, OpenShift OAuth is controlled via the |
Right, I meant that the code that creates the actual Kubernetes Or did you mean that the TLS configuration is a candidate for another possible value for |
Yes that was one possibility. So as |
I think they are not related, actually. One is about auth, the other is about securing the pipe. Perhaps we should consider renaming All in all, I like @mihau's approach, as it's practical and fits well when the operator runs in Kubernetes. We should just make sure to log somewhere (as I think we do already) that the other options are ignored if the platform is OpenShift. |
SGTM, so @mihau looks like you have the green light :) |
Hi @mihau, I'm very interested by your proposal and I look forward to seeing your work. |
@mihau This would be great! |
Given that @mihau posted the proposal originally last November, it is possible that someone else may need to take up the implementation. Is there anyone else that would be interested in contributing this feature? |
Will there be an option in JaegerIngressSpec to specify hosts for ingress resource? |
@stevie-, we might. The proposal from this issue has been accepted, but so far, nobody sent a PR implementing it. |
This is my current workaround example if anyone needs something similar. Basically, turn off the generated ingress and create my own ingress. jaeger instance to disable the default ingress apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
name: jaeger
spec:
ingress:
enabled: false separate ingress using nginx ingress, cert-manager, lets-encrypt with http challenge apiVersion: extensions/v1beta1
kind: Ingress
metadata:
name: traces
annotations:
kubernetes.io/ingress.class: "nginx"
certmanager.k8s.io/cluster-issuer: "letsencrypt-production"
certmanager.k8s.io/acme-challenge-type: http01
spec:
tls:
- hosts:
- traces.domain.com
secretName: traces-domain-tls
rules:
- host: traces.domain.com
http:
paths:
- path: /
backend:
serviceName: jaeger-query
servicePort: 16686 To access the UI, you would go to https://traces.domain.com. |
Our Workaround fir Nginx ingress: Caution: May not work if you have more ingress with host set to '*'. |
@stevie- can you give code sample as I didn’t follow completely? I’m a little new to all this. |
Snippet of our Helm Template to create the Jaeger resource
|
Resolves #135 Signed-off-by: Pavels Fjodorovs <me@pfyod.com>
so if someone still struggles to get this work here is jaeger cr config for ingress
|
@prageethw would you mind opening a PR with an example? Here's the right place for it: https://github.com/jaegertracing/jaeger-operator/tree/master/deploy/examples |
@jpkrohling done |
Is there a support for context root as well? Our project needs something like mydomain.com/jaeger for the ingress |
There seems to be no way to specify hostname for query ingress while creating a jaeger resource. Allowing for that as well as TLS settings would make it easy to handle by popular ingress controllers such as nginx.
I would approach it by adding fields to
jaeger
CRD similar to:I could implement it as such if everyone is ok with this approach.
The text was updated successfully, but these errors were encountered: