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

v3 helm chart doesn't support native ingress in distributed mode #12582

Closed
rknightion opened this issue Apr 11, 2024 · 5 comments · Fixed by #12932
Closed

v3 helm chart doesn't support native ingress in distributed mode #12582

rknightion opened this issue Apr 11, 2024 · 5 comments · Fixed by #12932
Assignees
Labels
area/helm type/bug Somehing is not working as expected

Comments

@rknightion
Copy link

rknightion commented Apr 11, 2024

Describe the bug
When deploying loki in distributed mode on the v3 helm chart and using native ingress (no gateway) the ingress object created points everything to a backend service "loki" which does not exist.
It's as if it's assuming single binary mode rather than distributed.

To Reproduce
Set up loki in SSD mode and get it functioning with native ingress.
Switch to distributed mode in the chart values file (and apply other relevant config).

Observe the diff in the rules section from helm showing the chart will point to a non-existent "loki" service rather than the distributed specific backend services.

loki, loki, Ingress (networking.k8s.io) has changed:
    rules:
      - host: "loki-gw.ours"
        http:
          paths:
-           - path: /api/prom/tail
+           - path: /api/prom/push
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /loki/api/v1/tail
+           - path: /loki/api/v1/push
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /loki/api
+           - path: /api/prom/tail
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /api/prom/rules
+           - path: /loki/api/v1/tail
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /loki/api/v1/rules
+           - path: /loki/api
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /prometheus/api/v1/rules
+           - path: /api/prom/rules
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /prometheus/api/v1/alerts
+           - path: /loki/api/v1/rules
              pathType: Prefix
              backend:
                service:
-                 name: loki-read
+                 name: loki
                  port:
                    number: 3100
-           - path: /api/prom/push
+           - path: /prometheus/api/v1/rules
              pathType: Prefix
              backend:
                service:
-                 name: loki-write
+                 name: loki
                  port:
                    number: 3100
-           - path: /loki/api/v1/push
+           - path: /prometheus/api/v1/alerts
              pathType: Prefix
              backend:
                service:
-                 name: loki-write
+                 name: loki
                  port:
                    number: 3100

Expected behavior
That distributed mode sets up the correct routes to the backend services.

@slim-bean
Copy link
Collaborator

trying to make sure I understand, you want to avoid running any kind of gateway pod like nginx and have the ingress do the routing directly to Loki backend services?

is this something that worked before and we broke it? (I'm newer to helm but just did a lot of work on the chart changes)

@rknightion
Copy link
Author

@slim-bean exactly! - An ingress object standalone. Some users will have that ingress object served via an nginx-ingress-controller pod but others will use something like an ALB to serve the ingress directly (such as myself). It's an alternative completely to running the gateway (for OSS users removes the need to run 2 sets of nginx deployments as well).
It's all configured in the section https://github.com/grafana/loki/blob/main/production/helm/loki/values.yaml#L1136 (the mimir chart has something similar as well as a more recent addition).

I'm not sure if it worked on the old loki-distributed chart (we've always used the SSD chart but are keen to migrate to the distributed mode of the main v2 chart) but it worked on the SSD version. I think it was introduced in #1585 and evolved on over time.

In the tpl at https://github.com/jkroepke/loki/blob/main/production/helm/loki/templates/_helpers.tpl#L521 and below it dynamically generates the ingress object path routes depending on if the deployment mode and I think that section only has SSD & single binary modes not the new Distributed mode. To resolve it would be a case of mapping the various read paths to the right query service, ruler to the ruler, ingest to ingest etc based on the path (whereas at the moment it's either read/write/backend).

@JStickler JStickler added area/helm type/bug Somehing is not working as expected labels Apr 15, 2024
@vlad-diachenko vlad-diachenko self-assigned this May 8, 2024
@vlad-diachenko
Copy link
Contributor

I will fix it by the end of the week

@vlad-diachenko
Copy link
Contributor

@rknightion could you please check the latest chart version with the fix

@TheRealNoob
Copy link
Contributor

@vlad-diachenko works great! I had been overwriting the ingress object (with my own that I'd written) after deploying with helm. The only endpoint that I was defining that this MR doesn't is /loki/api/v1/delete going to the compactor, but it's not essential and can be added later. Very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm type/bug Somehing is not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants