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

Update deploy.yaml #2520

Closed
wants to merge 1 commit into from
Closed

Update deploy.yaml #2520

wants to merge 1 commit into from

Conversation

ro6it
Copy link

@ro6it ro6it commented Sep 6, 2024

Updating service annotation

Proposed changes

Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:

Problem: While deploying Gateway API with NLB on EKS cluster, the service stuck in pending stuck and did not provision the NLB.

Solution: After searching a lot and going through the aws loadbalancer annotations sig page, I found it load-balancer-type should be nlb instaed of external.

Testing: Applied the changes in the cluster and it successfully provisioned an internet facing NLB loadbalancer on AWS eks

Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.

Closes #ISSUE

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

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.


Updating service annotation
@ro6it ro6it requested a review from a team as a code owner September 6, 2024 18:25
Copy link

nginx-bot bot commented Sep 6, 2024

Hi @ro6it! Welcome to the project! 🎉

Thanks for opening this pull request!
Be sure to check out our Contributing Guidelines while you wait for someone on the team to review this.

Please make sure to include the issue number in the PR description to automatically close the issue when the PR is merged.
See Linking a pull request to an issue and our Pull Request Guidelines for more information.

@sjberman
Copy link
Contributor

sjberman commented Sep 6, 2024

@ro6it Can you point me to the docs you found relating to this? I'm seeing that our default config is correct for IP targets based on this doc.

Also, you are able to manually change these values to fit your use case without having to change the default values in the repository. This particular deploy value is auto-generated from our helm values example.

@ro6it
Copy link
Author

ro6it commented Sep 6, 2024

@ro6it Can you point me to the docs you found relating to this? I'm seeing that our default config is correct for IP targets based on this doc.

Also, you are able to manually change these values to fit your use case without having to change the default values in the repository. This particular deploy value is auto-generated from our helm values example.

Here is the link - https://docs.nginx.com/nginx/deployment-guides/amazon-web-services/ingress-controller-elastic-kubernetes-services/#appendix:~:text=LoadBalancer%20service%20now.-,Configuring%20a%20LoadBalancer%20Service%20to%20Use%20NLB,-In%20service/loadbalancer

@sjberman
Copy link
Contributor

sjberman commented Sep 6, 2024

Thanks for the reference. Can you update the helm example I linked, and then run make generate-manifests?

@ro6it ro6it closed this Sep 6, 2024
@ro6it ro6it reopened this Sep 6, 2024
Copy link
Contributor

github-actions bot commented Sep 6, 2024

🎉 Thank you for your contribution! It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 Open Source Software (OSS) project. Please kindly read the F5 CLA and reply on a new comment with the following text to agree:


I have hereby read the F5 CLA and agree to its terms


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@ro6it
Copy link
Author

ro6it commented Sep 6, 2024

I noticed that with the existing annotations i.e (target-type - IP and LB -external) it is provisioning "classical" loadbalancer with target-type "instance" and when I changed it to nlb, then it is provisioning "NLB" with target-type - instance.

@lucacome
Copy link
Member

lucacome commented Sep 6, 2024

This is interesting because nlb doesn't even seem to be one of the allowed parameters https://kubernetes-sigs.github.io/aws-load-balancer-controller/latest/guide/service/annotations/#lb-type

This controller reconciles those service resources with this annotation set to either nlb-ip or external.

Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label Sep 21, 2024
@kate-osborn
Copy link
Contributor

@ro6it based on the current feedback we don't believe this change is necessary. If you run into any issues with setting the lb-type using our helm chart, please open a discussion or issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community stale Pull requests/issues with no activity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants