Skip to content

Updated guide to use NGINX/NGINX Plus as the Ingress Controller in EKS #159

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

Merged
merged 11 commits into from
Feb 7, 2025

Conversation

madebydna
Copy link
Contributor

@madebydna madebydna commented Feb 3, 2025

Proposed changes

Problem: The original version of the NGINX Ingress Controller on AWS EKS guide had many outdated links, some confusing instructions, and a readability score of 9 in the Hemingway editor.

Solution: I followed the instructions on this and various linked pages to set up an open source NGINX Ingress Controller in an AWS EKS cluster. I fixed outdated links along the way. I also tried to improve the flow of the guide by reorganizing some of the instructions on the page as they relate to references on linked pages. This was necessary to avoid confusion as the guide sometimes referenced older versions of linked pages.

I also improved the wording and readability according to the Hemingway editor from a 9 to a 7 (note that I removed code snippets and file paths):

Screenshot 2025-02-02 at 7 29 52 PM

Testing: I tested the instructions several times by setting up an AWS EKS cluster with the NGINX Ingress Controller, and deploying the Cafe sample application for testing functionality.

Please focus on: One part I couldn't get to work properly was to remove the use of the PROXY Protocol as described in the Appendix. Even after I added the (necessary) step of removing the Proxy protocol setting from the AWS target groups, the client IP was still logged in the Kubernetes logs of the NGINX Ingress Controller. This is an indication that client connection information is still being forwarded from the load balancer to the proxy.

Closes #98

Checklist

Before merging a pull request, run through this checklist and mark each as complete.

  • I have read the contributing guidelines
  • I have signed the F5 Contributor License Agreement (CLA)
  • I have ensured that documentation content adheres to the style guide
  • If the change involves potentially sensitive changes, I have assessed the possible impact
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md and CHANGELOG.md)
  • 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

Potentially sensitive changes include anything involving code, personally identify information (PII), live URLs or significant amounts of new or revised documentation.

Please refer to our style guide for guidance about placeholder content.

@madebydna madebydna requested a review from a team as a code owner February 3, 2025 03:42
Copy link

github-actions bot commented Feb 3, 2025

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@madebydna
Copy link
Contributor Author

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

@mjang mjang self-requested a review February 3, 2025 06:28
@mjang mjang added the in review label Feb 3, 2025
@mjang
Copy link
Contributor

mjang commented Feb 3, 2025

Hi @madebydna , you've done a great amount of work on this. I look forward to reviewing it, hopefully in the next couple of days (or at least by the end of the week).

@madebydna
Copy link
Contributor Author

Thanks @mjang! It was a fun challenge. 😄

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.

Hi @madebydna! I'm the assigned TW for Kubernetes stuff: I'm happy to approve and merge this in.

I have made a bunch of edit suggestions. They're things you would have had no chance to know since this document is consistent within itself, but not up to contemporary standards. I have added context to the first instance of each type of suggestion.

Thanks for contributing!

@madebydna
Copy link
Contributor Author

@ADubhlaoich, thanks for the review including the detailed explanations! I committed the suggested changes.

I found the Contributing Docs only after I submitted the PR, unfortunately. Otherwise I would have learned about Hugo shortcodes and avoiding HTML spans, etc. I will also reference the style guide more before submitting my next PR. 🙂

@ADubhlaoich ADubhlaoich enabled auto-merge (squash) February 6, 2025 15:05
auto-merge was automatically disabled February 7, 2025 13:46

Head branch was pushed to by a user without write access

Co-authored-by: yar <[email protected]>
@madebydna
Copy link
Contributor Author

@y82, @ADubhlaoich, the note about not uploading NGINX Plus images doesn't exist as an include yet.

On the content/nginx/admin-guide/installing-nginx/installing-nginx-docker.md page it occurs twice (here and here). There is an include at content/includes/nap-waf/build-nginx-image-cmd.md that contains a similar phrase but it's specific to NGINX App Protect WAF v5 and also has additional content (see here).

I could take a stab at refactoring but I'm wondering if that should be a separate ticket since it would go beyond the scope of this one.

@mjang
Copy link
Contributor

mjang commented Feb 7, 2025

I could take a stab at refactoring but I'm wondering if that should be a separate ticket since it would go beyond the scope of this one.

@madebydna thank you, I think you're correct. IMO, you've gone "above and beyond" with your answer. Since you now have two approvals, I'm going to proceed with merge, and congratulate you on making your first contribution!

@mjang mjang merged commit 82c682e into nginx:main Feb 7, 2025
5 checks passed
@mjang mjang added community and removed in review labels Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analyze NGINX Plus Deployment Guide: Using NGINX/NGINX Plus as the Ingress Controller
4 participants