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

Prevent paths in HTTPRoute matches from conflicting with internal locations in NGINX #1445

Merged
merged 12 commits into from
Jan 16, 2024

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Jan 4, 2024

Proposed changes

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

Problem: To simplify routing, generated locations should be different from defined http route locations.

Solution:

  1. Updated internal location block prefix to be named to `rule%d-route%d
  2. Removed internal directive from server block
  3. Simplified Rewrite & Redirect for url

Testing:

  1. Updated unit tests
  2. Did manual testing

curl request with version 2 header returns response from coffee-2

sa.choudhary@JX46X2W221 cafe % curl --resolve cafe.example.com:$GW_PORT:$GW_IP http://cafe.example.com:$GW_PORT/coffee -H "version:v2"
Handling connection for 8080
Server address: 10.244.0.25:8080
Server name: coffee-v2-7d47fc86cb-4mkkf
Date: 04/Jan/2024:23:46:55 +0000
URI: /coffee
Request ID: c71364174305814d8385e33a05575aa9

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 #428

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

@salonichf5 salonichf5 added the bug Something isn't working label Jan 4, 2024
@salonichf5 salonichf5 requested review from a team as code owners January 4, 2024 23:50
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed bug Something isn't working labels Jan 4, 2024
@salonichf5 salonichf5 force-pushed the location-prefix branch 3 times, most recently from 538b9d8 to e64c291 Compare January 10, 2024 06:50
@ADubhlaoich
Copy link
Contributor

LGTM on the docs side - since the changes in the PR are primarily code-based I will not make an approval.

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

thanks @salonichf5

this looks good. per review comment, let's update the prefix so that we don't need any restrictions on paths in HTTPRoute routing rules

internal/mode/static/nginx/config/http/config.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/config/http/config.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Jan 10, 2024
@kate-osborn
Copy link
Contributor

@salonichf5 can you update the PR description to reflect the new approach?

@sjberman
Copy link
Contributor

@pleshakov Do you think with this change we still need the internal; directive? Is it redundant, or does it still add value?

@pleshakov
Copy link
Contributor

@sjberman
Is it redundant

salonichf5 and others added 4 commits January 16, 2024 09:43
Co-authored-by: Alan Dooley <github@adubhlaoi.ch>
Co-authored-by: Michael Pleshakov <pleshakov@users.noreply.github.com>
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

🚀

@salonichf5 salonichf5 merged commit b2d078b into nginxinc:main Jan 16, 2024
27 checks passed
@lucacome lucacome added the bug Something isn't working label Mar 13, 2024
@pleshakov pleshakov changed the title fix: Ensure unique generated locations Prevent paths in HTTPRoute matches from conflicting with internal locations in NGINX Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ensure uniqueness of generated locations
6 participants