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

[loki-distributed] fix: force nginx gateway to resolve upstream hostname on missing locations #3206

Conversation

rsicart
Copy link
Contributor

@rsicart rsicart commented Jul 3, 2024

In order to force nginx to resolve hostnames in proxy_pass directive, we
need to set a variable and use it in proxy_pass. Otherswise nginx only
resolves hostnames at boot time.

@rsicart rsicart changed the title fix: force loki-distributed nginx gateway to resolve upstream hostname on missing locations [loki-distributed] fix: force nginx gateway to resolve upstream hostname on missing locations Jul 9, 2024
@rsicart rsicart force-pushed the rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass branch from 02071bf to 2b1a7a4 Compare July 9, 2024 07:38
Copy link
Collaborator

@Sheikh-Abubaker Sheikh-Abubaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there @rsicart, thanks for the contribution, Have you tested it ?

@rsicart
Copy link
Contributor Author

rsicart commented Jul 25, 2024

Hey there @rsicart, thanks for the contribution, Have you tested it ?

helm template works correctly.

Installing the chart locally in a kind cluster works fine, gateway is Running and nginx config syntax is ok:

~/repos/github.com/rsicart/grafana-helm-charts/charts/loki-distributed k8s:kind-kind:: ~ git:rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass » helm upgrade --install loki-distributed . --set gateway.nginxConfig.resolver=10.96.0.10
....

~/repos/github.com/rsicart/grafana-helm-charts/charts/loki-distributed k8s:kind-kind:: ~ git:rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass » kubectl get pods                                                                       
NAME                                              READY   STATUS    RESTARTS   AGE
loki-distributed-distributor-5f98995f99-tjthf     1/1     Running   0          10m
loki-distributed-gateway-74c6945df-47j97          1/1     Running   0          6m
loki-distributed-ingester-0                       1/1     Running   0          10m
loki-distributed-querier-0                        1/1     Running   0          10m
loki-distributed-query-frontend-cbf67c5df-8tgb4   1/1     Running   0          10m

~/repos/github.com/rsicart/grafana-helm-charts/charts/loki-distributed k8s:kind-kind:: ~ git:rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass » kubectl exec -ti loki-distributed-gateway-74c6945df-47j97 -- nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful

@rsicart rsicart force-pushed the rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass branch from 2b1a7a4 to 90e60bf Compare August 5, 2024 08:45
charts/loki-distributed/README.md Outdated Show resolved Hide resolved
charts/loki-distributed/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@Sheikh-Abubaker Sheikh-Abubaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rsicart Could you please address the requested changes and fix CI lint error ? Thank you!

…e on missing locations

Signed-off-by: R.Sicart <[email protected]>

In order to force nginx to resolve hostnames in proxy_pass directive, we
need to set a variable and use it in proxy_pass. Otherswise nginx only
resolves hostnames at boot time.
Signed-off-by: R.Sicart <[email protected]>
@rsicart rsicart force-pushed the rsi/loki-distributed-force-nginx-resolve-dns-proxy-pass branch from 0ab10d4 to 648d633 Compare August 12, 2024 08:16
@Sheikh-Abubaker Sheikh-Abubaker merged commit 3d188c3 into grafana:main Aug 12, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

3 participants