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

[Bug]: Ingress jaeger-query should use the default IngressClass if one is not provided #2225

Open
Jeansen opened this issue Jun 4, 2023 · 10 comments · May be fixed by #2622
Open

[Bug]: Ingress jaeger-query should use the default IngressClass if one is not provided #2225

Jeansen opened this issue Jun 4, 2023 · 10 comments · May be fixed by #2622
Assignees
Labels
bug Something isn't working go Pull requests that update Go code good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Jeansen
Copy link

Jeansen commented Jun 4, 2023

What happened?

When I deploy jaeger the operator also createas an Ingress 'jaeger-query'. Although my kubernes IngressClass is configured as default (there is only one Ingress in my setup) and I also set it to watch for Ingress deployments without an IngressClass, the jaeger Ingress always has an IngressClass of ''.

When I delete the Ingress and recreate it, it has the default IngressClass 'nginx'. But after a while, it is overwritten with '' again. Happens only with jaeger . Other, manually created Ingresses, work just fine!

Steps to reproduce

  1. Prepare a kubernetes Cluster. Mine runs 1.27
  2. Install Ingress, like so:
    helm repo add ingress-nginx https://kubernetes.github.io/ingress-nginx
    helm repo update
    helm upgrade --install  ingress-nginx ingress-nginx/ingress-nginx  --set watchIngressWithoutClass=true --set controller.ingressClassResource.default=true --namespace ingress-nginx --create-namespace
  1. Install Jaeger, follow the documentation at https://www.jaegertracing.io/docs/1.18/operator/. The 'AllInOne' Setup should be fine, though I use the production strategey.
  2. Check the jaeger-query is there. It should have an IngressClass of 'nginx'.
  3. Wait an hour or so....check again. Now it should how an IngressClass of ''.
  4. Save the Ingress to to a yaml file: kubectl get ingress jaeger-query -o yaml > /tmp/ingerss.yaml
  5. Create the Ingress again: kubectl replace -f /tmp/ingress.yaml --force
  6. Repeat Steps 4 and 5

Expected behavior

Ingress jaeger-query should always have a default IngressClass 'nginx'.

Relevant log output

No response

Screenshot

No response

Additional context

No response

Jaeger backend version

v1.44.0

SDK

No response

Pipeline

No response

Stogage backend

Open-Search

Operating system

Linux, Debian 11

Deployment model

Kubernetes

Deployment configs

No response

@Jeansen Jeansen added the bug Something isn't working label Jun 4, 2023
@Jeansen Jeansen changed the title [Bug]: Ingress jaeger-query has not IngressClass [Bug]: Ingress jaeger-query has no IngressClass Jun 4, 2023
@iblancasa
Copy link
Collaborator

iblancasa commented Jun 6, 2023

@Jeansen could you provide the CR used to create your Jaeger instance?

If you check how the Jaeger Operator works, it will set the IngressClass only if you provided one to the Jaeger .spec.ingress.ingressclassname:

spec.IngressClassName = i.jaeger.Spec.Ingress.IngressClassName

If you modify the Ingress created by the Jaeger Operator, it will be reconciled and your changes overwritten.

@Jeansen
Copy link
Author

Jeansen commented Jun 6, 2023

PASSWORD=${'$'}(kubectl get -n $ns secret ${clusterName}-admin-password -o=jsonpath="{.data.password}" | base64 --decode)
USERNAME=${'$'}(kubectl get -n $ns secret ${clusterName}-admin-password -o=jsonpath="{.data.username}" | base64 --decode)
kubectl -n $ns create -f https://github.com/jaegertracing/jaeger-operator/releases/download/$version/jaeger-operator.yaml
kubectl -n $ns create secret generic jaeger-secret --from-literal=ES_PASSWORD=${'$'}{PASSWORD} --from-literal=ES_USERNAME=${'$'}USERNAME


cat <<EOF | kubectl apply --force -f -
apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger
  namespace: $ns
spec:
  collector:
    autoscale: false
  strategy: production
  agent:
    strategy: DaemonSet
  storage:
    type: elasticsearch
    options:
      log-level: debug
      query:
        base-path: /jaeger
      es:
        server-urls: https://${clusterName}.${ns}:9200
        tls:
          ca: /es/certificates/ca.crt
    secretName: jaeger-secret
  volumeMounts:
    - name: certificates
      mountPath: /es/certificates/
      readOnly: true
  volumes:
    - name: certificates
      secret:
        secretName: ${clusterName}-http-cert

@Jeansen
Copy link
Author

Jeansen commented Jun 6, 2023

If you modify the Ingress created by the Jaeger Operator, it will be reconciled and your changes overwritten.

@iblancasa Well, that's exactly the point. I did not specify an IngressClass and therefore expect the cluster default to be used.

@iblancasa
Copy link
Collaborator

@Jeansen ok, now I understand. Please, could you modify the title of the issue to Ingress jaeger-query should use the default IngressClass if one is not provided? I can take a look into that and try to send a PR

@Jeansen Jeansen changed the title [Bug]: Ingress jaeger-query has no IngressClass [Bug]: Ingress jaeger-query should use the default IngressClass if one is not provided Jun 7, 2023
@Jeansen
Copy link
Author

Jeansen commented Jun 7, 2023

@iblancasa Of course. And thank you for all the help. Looking forward to that PR.

iblancasa added a commit to iblancasa/jaeger-operator that referenced this issue Jun 7, 2023
@iblancasa iblancasa added good first issue Good for newcomers go Pull requests that update Go code labels Jun 16, 2023
@parauliya
Copy link

Hi @iblancasa ,
Can I pick this issue?
Please assign this to me.

@parauliya
Copy link

Hi @iblancasa ,
I am trying to reproduce this issue but I am not able to install the jaeger crds as per the docs of 1.18 version mentioned in the issue.
https://www.jaegertracing.io/docs/1.18/operator/

In the above link the following command is being failed:
[kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/main/deploy/crds/jaegertracing.io_jaegers_crd.yaml](kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/main/deploy/crds/jaegertracing.io_jaegers_crd.yaml)
As the file it is applying does not exist in the above location.
Could you please let me know that which version of jaeger-operator I should install to reproduce this issue?

@parauliya
Copy link

@iblancasa
Also Could you please let me know which tool you use for community wide communication like slack, telegram or Team etc.?
And if possible add me there so that in your absents someone else can reply to my query for faster resolution.

@iblancasa
Copy link
Collaborator

Hi @iblancasa , I am trying to reproduce this issue but I am not able to install the jaeger crds as per the docs of 1.18 version mentioned in the issue. https://www.jaegertracing.io/docs/1.18/operator/

In the above link the following command is being failed: [kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/main/deploy/crds/jaegertracing.io_jaegers_crd.yaml](kubectl create -f https://raw.githubusercontent.com/jaegertracing/jaeger-operator/main/deploy/crds/jaegertracing.io_jaegers_crd.yaml) As the file it is applying does not exist in the above location. Could you please let me know that which version of jaeger-operator I should install to reproduce this issue?

I didn't reproduce it. Try installing the latest version.

@iblancasa Also Could you please let me know which tool you use for community wide communication like slack, telegram or Team etc.? And if possible add me there so that in your absents someone else can reply to my query for faster resolution.

CNCF Slack channel.

@antoniomerlin
Copy link
Contributor

created a pull request addressing this #2458

@antoniomerlin antoniomerlin mentioned this issue Feb 12, 2024
4 tasks
antoniomerlin pushed a commit to antoniomerlin/jaeger-operator that referenced this issue Feb 12, 2024
added function getIngressClass which set default or nginx ingress class to jaeger define ingress for providing loadbalancer IP.

Signed-off-by: Gaurav Singh <[email protected]>
@antoniomerlin antoniomerlin mentioned this issue Feb 12, 2024
4 tasks
iblancasa pushed a commit that referenced this issue Mar 11, 2024
* fixes #1568 #2225

added function getIngressClass which set default or nginx ingress class to jaeger define ingress for providing loadbalancer IP.

Signed-off-by: Gaurav Singh <[email protected]>

* Added test for checking if ingressClass is not set then the default or nginx ingressClass is added to the jaeger ingress.

Signed-off-by: Gaurav Singh <[email protected]>

* Added request chenges mentioned by iblancasa re: test for checking if ingressClass is not set then the default or nginx ingressClass is added to the jaeger ingress.

Signed-off-by: Gaurav Singh <[email protected]>

* changed func name from getInClusterAvailableIngressClasses() to
getInClusterAvailableIngressClass() and updated manifests

Signed-off-by: Gaurav Singh <[email protected]>

* fixed formatting

Signed-off-by: Gaurav Singh <[email protected]>

---------

Signed-off-by: Gaurav Singh <[email protected]>
Co-authored-by: Gaurav Singh <[email protected]>
@frzifus frzifus added the help wanted Extra attention is needed label Mar 22, 2024
antoniomerlin added a commit to antoniomerlin/jaeger-operator that referenced this issue Jul 10, 2024
…ctDefaultIngressClass func updated API call to use client.Reader instead of client.Client

Signed-off-by: amerlin <[email protected]>
@antoniomerlin antoniomerlin linked a pull request Jul 10, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
5 participants