Discoverable Alertmanager + fixes + documentation #57
Conversation
ts-mini
force-pushed
the
cortex-chart-tweaks
branch
2 times, most recently
from
be399dd to
68250e1
Compare
templates/nginx-config.yaml
Outdated
| {{- $root := . }} | ||
| {{- range .Values.alertmanager.ui.scopeIDs }} | ||
| {{ $rootScope := . }} | ||
| location = /alerts/{{ $rootScope.scopeName }} { | ||
| proxy_set_header X-Scope-OrgID {{ $rootScope.scopeID }}; | ||
| proxy_pass http://{{ template "cortex.fullname" $root }}-alertmanager.{{ $root.Release.Namespace }}.svc.cluster.local:{{ $root.Values.config.server.http_listen_port }}/api/prom/alertmanager/; | ||
| } | ||
|
|
||
| location ~ /alerts/{{ $rootScope.scopeName }}/(.*) { | ||
| proxy_set_header X-Scope-OrgID {{ $rootScope.scopeID }}; | ||
| proxy_pass http://{{ template "cortex.fullname" $root }}-alertmanager.{{ $root.Release.Namespace }}.svc.cluster.local:{{ $root.Values.config.server.http_listen_port }}/api/prom/alertmanager/$1; | ||
| } | ||
| {{- end }} |
There was a problem hiding this comment.
Since we are setting the orgID header based on the map, could we add a similar location rule that we have for the other endpoints to the alertmanager ui?
templates/ingress-distributor.yaml
Outdated
| {{- with .annotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| nginx.ingress.kubernetes.io/configuration-snippet: | |
There was a problem hiding this comment.
see my other comment about the map in the nginx config, based on that I don't think we need ingress resources for the distributors.
I would also try to avoid using nginx specific ingress annotations since users may use other ingress controllers.
There was a problem hiding this comment.
I agree with both of Roger's items here. We need to keep ingress resource templates agnostic to the kind of controller used, plus in this case, we can use the Nginx config settings which negates the need for another ingress resource.
|
@rsteneteg I will work on your comments asap! thanks for the feedback |
There was a problem hiding this comment.
Thank you @ts-mini for the work here. This PR has a number of changes in it, some of which I think are ready to merge now and others I don't think are a good fit. I'm going to try my best to classify them:
- Documentation: Huge thank you for creating the values documentation
- Alertmanger discovery: There's some feedback here, but this is a great addition
- Nginx tweaks/fixes: Updates for correcting ports, etc ... most welcome
- Multi-tenancy mapping: See my comment about this being a concern we've wanted to keep external from cortex. I really like your thoughts and approach to this, but I don't think it is a good fit for bringing into the chart.
ts-mini
force-pushed
the
cortex-chart-tweaks
branch
4 times, most recently
from
161a61d to
72877d5
Compare
ts-mini
changed the title
|
I've made some changes - stripping out the stuff per @khaines comments. I think this PR is a bit easier to digest now too - seeing as that complexity is gone. I will work on making a secondary chart for our use that implements the multitenant separately from this chart - this will be a good path forward I think. I understand the implications of disclosing implementation details - but have made this 'additional' chart available for anyone that wants to follow in my footsteps: deploy cortex, then deploy MT setup 'our/my way' - with the standard caveats of course. If there's room in cortexproject for specific implementations - I wouldn't mind having it live here if y'all are ok with it, but that can be a future conversation. |
…ager discovery Signed-off-by: Tyler Horvath <[email protected]>
ts-mini
force-pushed
the
cortex-chart-tweaks
branch
3 times, most recently
from
08eac24 to
eb92a62
Compare
|
LGTM |
…ager discovery (cortexproject#57) Signed-off-by: Tyler Horvath <[email protected]>
Just attempting to contribute some of our changes to this chart upstream:
1) We're running multi-tenant - so we wanted to have ingress routes per scopeID when auth is enabled. I've added this under distributor - but I'm not sure if this is the right place (as it ultimately creates a new route pointing to the nginx service)2) I've added more configuration to the nginx service to make some of the debug/routes easier to hit from ingress.
3) Added a per-scope-id alertmanager path into nginx, configurable to alertmanager.ui.scopeids4) added documentation
5) adjusted alertmanager discovery
I'm very flexible on implementation details - just interested in getting these style feature set upstream - so let me know if there are stylistic tweaks needed
And added some readme things