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

[Doc] Update Helm chart instructions to avoid UX friction when using Kind #1281

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Nov 26, 2023

Proposed changes

For kind clusters, NodePort services require extra configuration and LoadBalancer services need a third-party controller like MetalLB for external IP assignment. However, the Helm chart creates a LoadBalancer service by default. Therefore, the --wait flag will hang until timeout.

The guide running-on-kind.md asks users to create a Kind cluster by running the make create-kind-cluster command. However, the command neither sets the extraPortMappings Kind configs nor installs MetalLB, so users cannot use NodePort and LoadBalancer directly after running the command.

This PR suggests users disable the creation of NodePort / LoadBalancer Kubernetes service, and uses the port-forwarding command from the running-on-kind.md guide.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added documentation Improvements or additions to documentation helm-chart Relates to helm chart labels Nov 26, 2023
@kevin85421 kevin85421 marked this pull request as ready for review November 26, 2023 08:35
@kevin85421 kevin85421 requested a review from a team as a code owner November 26, 2023 08:35
@kevin85421
Copy link
Contributor Author

Sorry, I did not notice the presence of a pre-commit configuration. I have now fixed the linting error.
Screen Shot 2023-11-29 at 9 53 41 AM

@sjberman
Copy link
Contributor

@kevin85421 We're about to migrate our docs a little bit in the next day or two, so we'll wait to rebase your change on the new structure before we look at merging it.

@kevin85421
Copy link
Contributor Author

Just out of curiosity, what's the reason for using LoadBalancer as the default service instead of ClusterIP? Based on my observation, the L4 load balancing seems to be less popular in the Kubernetes community. Thanks!

@sjberman
Copy link
Contributor

@kevin85421 The main use case of NGINX Gateway Fabric is to get traffic into a cluster, using it as an ingress point. ClusterIP Services are for traffic within the cluster network, whereas the LB Service is for getting traffic into the cluster from an outside source.

@sjberman
Copy link
Contributor

@kevin85421 Our docs have now been migrated if you want to rebase. The kind document now lives at /site/content/installation/running-on-kind.md. Please format the note as {{<note>}}your text{{</note>}}, instead of using ">".

@kevin85421
Copy link
Contributor Author

@kevin85421 The main use case of NGINX Gateway Fabric is to get traffic into a cluster, using it as an ingress point. ClusterIP Services are for traffic within the cluster network, whereas the LB Service is for getting traffic into the cluster from an outside source.

Thank @sjberman for your reply! Recently, I have begun delving into more details about Kubernetes networking to enhance support for Ray Serve on Kubernetes (KubeRay). Therefore, some of my questions might be basic.

I've come across numerous articles and videos (example) that compare the Kubernetes LoadBalancer service (OSI L4) and Ingress/Gateway (OSI L7) as alternatives. Hence, this makes me feel a bit puzzled when they are presented simultaneously.

Based on your reply, I guess:

  • Kubernetes LoadBalancer Service: Each application running on the Kubernetes cluster requires its own LoadBalancer, which can be expensive.
  • Ingress/Gateway: With Ingress/Gateway, users only need to create a LoadBalancer for the Ingress/Gateway controller. This controller can then forward requests to multiple applications within the Kubernetes cluster.

Is my understanding correct? Thanks!

@sjberman
Copy link
Contributor

sjberman commented Nov 30, 2023

@kevin85421 Yes, you're understanding is correct. Ingress/Gateway is fronted by a load balancer for traffic incoming to the cluster, and then the Ingress/Gateway handles routing traffic to all of your backends within the cluster.

Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for nginx-gateway-fabric ready!

Name Link
🔨 Latest commit a2abd4a
🔍 Latest deploy log https://app.netlify.com/sites/nginx-gateway-fabric/deploys/656de890bb0d330008741e3d
😎 Deploy Preview https://deploy-preview-1281--nginx-gateway-fabric.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kevin85421
Copy link
Contributor Author

kevin85421 commented Nov 30, 2023

I have already updated the PR. I will figure out how to build the documentation locally and ensure that it is rendered correctly.

@kevin85421
Copy link
Contributor Author

I followed this doc and ran hugo server -e development -b "http://127.0.0.1/nginx-gateway-fabric/" to build the doc locally, and the note is rendered correctly.

Screen Shot 2023-11-30 at 10 56 09 AM

@sjberman sjberman requested review from a team and removed request for Jcahilltorre November 30, 2023 21:08
Copy link
Contributor

@ADubhlaoich ADubhlaoich left a comment

Choose a reason for hiding this comment

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

LGTM!

@sjberman sjberman enabled auto-merge (squash) December 4, 2023 14:57
@sjberman sjberman merged commit 6bc90a8 into nginxinc:main Dec 4, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation helm-chart Relates to helm chart
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants