-
Notifications
You must be signed in to change notification settings - Fork 254
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
Conversation
There was a problem hiding this 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
helm/robusta/values.yaml
Outdated
@@ -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' |
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
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
@arikalon1 updated |
There was a problem hiding this 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this 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
Fixes #1025