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

Kube services considerations #9194

Merged
merged 2 commits into from
Feb 26, 2024
Merged

Kube services considerations #9194

merged 2 commits into from
Feb 26, 2024

Conversation

Nadine2016
Copy link
Contributor

@Nadine2016 Nadine2016 commented Feb 26, 2024

Description

Please include a high level summary of the changes.

This bug fixes ... \ This new feature can be used to ...

Fill out any of the following sections that are relevant and remove the others

API changes

  • Added x field to y resource
  • ...

Code changes

  • Fix error in Foo() function
  • Add Bar() function
  • ...

CI changes

  • Adjusted schedule for x job
  • ...

Docs changes

  • Added guide about feature x to public docs
  • Updated README to account for y behavior
  • ...

Context

Users ran into this bug doing ... \ Users needed this feature to ...

See slack conversation here

Interesting decisions

We chose to do things this way because ...

Testing steps

I manually verified behavior by ...

Notes for reviewers

Be sure to verify intended behavior by ...

Please proofread comments on ...

This is a complex PR and may require a huddle to discuss ...

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves #9164

@Nadine2016 Nadine2016 requested a review from a team as a code owner February 26, 2024 19:22
@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Feb 26, 2024
@solo-changelog-bot
Copy link

Issues linked to changelog:
#9164

@Nadine2016
Copy link
Contributor Author

/skip-ci

@soloio-bulldozer soloio-bulldozer bot merged commit ce7474c into main Feb 26, 2024
17 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the docs-kube-services-note branch February 26, 2024 19:36
Copy link

Visit the preview URL for this PR (updated for commit 14e79dc):

https://gloo-edge--pr9194-docs-kube-services-n-471835tq.web.app

(expires Mon, 04 Mar 2024 19:36:27 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677


{{< highlight yaml "hl_lines=6-10" >}}
1. Enable routing to Kubernetes services by setting `settings.disableKubernetesDestinations: true` in your Gloo Edge Helm chart. By default, routing to Kubernetes services is disabled in Gloo Edge due to the negative performance impact on translation and load balancing time in clusters with a lot of Kubernetes services.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the mixture of positives/negatives a bit confusing in this statement (which to be clear, is more based on the API naming). We are enabling routing to kubernetes services by disabling kubernetes destinations? But the default is also disabled?
What I can't quite tell is should the value be settings.disableKubernetesDestinations=false?


* `ref` is a {{< protobuf name="core.solo.io.ResourceRef">}} to the service that should receive traffic
* `port` is an `int` which represents the port on which the service is listening. This must be one of the ports defined in the Kubernetes service spec
Consider the following information before choosing a Kubernetes service as your routing destination:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we don't recommend routing directly to a Kubernetes service, how would I, as an operator of Gloo Edge, route traffic to pods?

Copy link
Contributor

Choose a reason for hiding this comment

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

As we have production recommendations, I also think it's helpful if we document that in our kube2e tests. By that I mean, in all of our kube2e tests, we install gloo with some helm values (ie https://github.com/solo-io/gloo/blob/main/test/kube2e/helm/artifacts/helm.yaml), should we set the value (even if it is the default) to the production recommendation?


{{< highlight yaml "hl_lines=6-10" >}}
1. Enable routing to Kubernetes services by setting `settings.disableKubernetesDestinations: true` in your Gloo Edge Helm chart. By default, routing to Kubernetes services is disabled in Gloo Edge due to the negative performance impact on translation and load balancing time in clusters with a lot of Kubernetes services.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update https://github.com/solo-io/gloo/blob/main/projects/gloo/api/v1/settings.proto#L676 to both clarify the default value, and perhaps link to this document?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve docs on preferred ways to route to K8S Services and Pods
4 participants