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

Use unqualified names for Alertmanager & service discovery URL #1026

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

yahya-hrln
Copy link
Contributor

Fixes #1025

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@arikalon1 arikalon1 left a comment

Choose a reason for hiding this comment

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

Sorry for the late review @yahya-hrln
Left small comment

@@ -511,7 +511,7 @@ kube-prometheus-stack:
- name: 'null'
- name: 'robusta'
webhook_configs:
- url: 'http://robusta-runner.{{ .Release.Namespace }}.svc.cluster.local/api/alerts'
- url: 'http://robusta-runner.{{ .Release.Namespace }}/api/alerts'
Copy link
Contributor

Choose a reason for hiding this comment

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

@yahya-hrln Can you please make it configurable, so it won't change the default behavior for all users?
i.e., put the DNS suffix in a helm value, defaulted to .svc.cluster.local

My concern is, that some users have defined network policies for the existing url, and this change might break it for them

@@ -18,6 +18,6 @@ def find_service_url(label_selector):
name = svc.metadata.name
namespace = svc.metadata.namespace
port = svc.spec.ports[0].port
url = f"http://{name}.{namespace}.svc.cluster.local:{port}"
url = f"http://{name}.{namespace}:{port}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here,
I would pass the helm value to the runner, using env variable, and use it here

@anakaiti
Copy link

@arikalon1 updated

Copy link
Contributor

@arikalon1 arikalon1 left a comment

Choose a reason for hiding this comment

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

left a small comment @yahya-hrln
WDYT

@@ -540,7 +543,7 @@ kube-prometheus-stack:
- name: 'null'
- name: 'robusta'
webhook_configs:
- url: 'http://robusta-runner.{{ .Release.Namespace }}.svc.cluster.local/api/alerts'
- url: 'http://robusta-runner.{{ .Release.Namespace }}.svc.{{ .Values.global.clusterDomain }}/api/alerts'
Copy link
Contributor

Choose a reason for hiding this comment

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

The url you'd like on your case is http://robusta-runner.default/api/alerts right?
What will you put in clusterDomain for your cluster? is it ""
I think that will result in http://robusta-runner.default.svc./api/alerts
Is it working this way?

Copy link

Choose a reason for hiding this comment

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

In my case, I'll set it to my GKE cluster's custom suffix. So it will still use an FQDN instead of an unqualified name.

@@ -18,6 +20,6 @@ def find_service_url(label_selector):
name = svc.metadata.name
namespace = svc.metadata.namespace
port = svc.spec.ports[0].port
url = f"http://{name}.{namespace}.svc.cluster.local:{port}"
url = f"http://{name}.{namespace}.svc.{CLUSTER_DOMAIN}:{port}"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

@arikalon1 arikalon1 left a comment

Choose a reason for hiding this comment

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

Looks good
Thanks for your PR @yahya-hrln

@arikalon1 arikalon1 merged commit e35979a into robusta-dev:master Nov 6, 2023
8 checks passed
pavangudiwada pushed a commit to pavangudiwada/robusta that referenced this pull request Jun 25, 2024
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.

Runner & prometheus stack alertmanager uses wrong URL when GKE VPC scope DNS is used
4 participants