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

Hardcode kgateway service name #10666

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jenshu
Copy link
Contributor

@jenshu jenshu commented Feb 19, 2025

Since our controller makes an assumption about the control plane service being named kgateway we should hardcode this in the helm chart for now.

Also removed "gloo" references from some constants.

Fixes #10659

Signed-off-by: Jenny Shu <jenny.shu@solo.io>
Signed-off-by: Jenny Shu <jenny.shu@solo.io>
Copy link
Contributor

@lgadban lgadban left a comment

Choose a reason for hiding this comment

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

should we remove the fullnameOverride value for now?

Copy link
Contributor

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

can we fix this the other way around?
i.e. add the service name as an env-var in the deployment, and use that for the xds host?

@yuval-k
Copy link
Contributor

yuval-k commented Feb 21, 2025

for reference, what i'm suggesting is to add an env var in the kgateway deployment:

name: XDS_HOST
value: {{ include "kgateway.fullname" . }}

and use that in GetControlPlaneXdsHost

func GetControlPlaneXdsHost() string {
	return kubeutils.ServiceFQDN(metav1.ObjectMeta{
		Name:      os.Getenv("XDS_HOST"),
		Namespace: namespaces.GetPodNamespace(),
	})
}

We may want to tweak this, so we centralize our processing of env-vars.

@lgadban
Copy link
Contributor

lgadban commented Feb 21, 2025

Adding on to @yuval-k 's last point, the existing "settings" processing is centralized here currently: https://github.com/kgateway-dev/kgateway/blob/main/internal/kgateway/extensions2/settings/settings.go#L7-L12

This was intended to be as lightweight as possible while still providing a standardized approach.
IMO this should be sufficient for the use case we want to accomplish in this PR.

In general though, we may want to reevaluate this approach given that the controller binary is now ran via cobra. Viper is fairly idiomatic and allows to define configuration in a single place and demotes the env-var specifics to an implementation detail.

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.

kgateway service name can get out of sync between helm vs code
3 participants