Add docs for rewrite client IP settings #2701

salonichf5 · 2024-10-18T20:08:58Z

Proposed changes

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

Problem: User needs documentation explaining how to configure rewrite client IP settings.

Solution: Adds documentation on how to configure rewrite client IP settings

Testing: Manual testing using make watch docs.

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

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.

github-actions · 2024-10-18T20:11:48Z

✅ Deploy Preview will be available once build job completes!

Name	Link
😎 Deploy Preview	https://frontdoor-test-docs.nginx.com/previews/nginx-gateway-fabric/2701/

codecov · 2024-10-18T20:12:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.38%. Comparing base (88661c1) to head (8c4a0ba).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2701   +/-   ##
=======================================
  Coverage   89.38%   89.38%           
=======================================
  Files         110      110           
  Lines       10913    10913           
  Branches       50       50           
=======================================
  Hits         9755     9755           
  Misses       1100     1100           
  Partials       58       58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ADubhlaoich

LGYTM, some minor nitpicks.

site/content/how-to/data-plane-configuration.md

bjee19

lgtm!

site/content/how-to/data-plane-configuration.md

Co-authored-by: Alan Dooley <[email protected]>

Co-authored-by: Saylor Berman <[email protected]>

Co-authored-by: bjee19 <[email protected]>

sjberman

This was merged before I had a chance to re-review and approve. Please see my change requests.

sjberman · 2024-10-22T03:41:25Z

site/content/how-to/data-plane-configuration.md

+
+## Configure PROXY protocol and RewriteClientIP settings
+
+When the request is passed through multiple proxies or load balancers, the client IP is set to the IP address of the server that last handled the request. To preserve the original client IP address, you can configure `RewriteClientIP` settings in the `NginxProxy` resource. `RewriteClientIP` has the fields: _mode_, _trustedAddresses_ and _setIPRecursively_.


"When a request" not "When the request"

sjberman · 2024-10-22T03:41:59Z

site/content/how-to/data-plane-configuration.md

+spec:
+  config:
+    rewriteClientIP:
+      mode: XForwardedFor


Part of me wonders if it makes more sense to use ProxyProtocol as the example, since that was the main use case we were targeting with this feature.

I am a little confused about that as well. I wanted to mention setIPrecursively field since it only applies to XForwardedFor

But at this point, its causing a lot of confusion so I can include both.

I don't think we need to provide every field in our example. Nor do we need to provide examples of both modes. Our API covers all cases, I think we just provide a basic example of the most common use case.

sjberman · 2024-10-22T03:42:24Z

site/content/how-to/data-plane-configuration.md

+      trustedAddresses:
+      - type: CIDR
+        value: ":1/28"
+      - type: IPAddress
+        value: "192.68.74.28"
+      - type: Hostname
+        value: "cafe.com"


On second thought, I'm not sure we need to specify all three examples here. One should be enough to get the point across. A realistic CIDR would be fine.

Just added this for user convenience to not go refer the spec and figure out types. I think we can have all fields for one example and simply it for our second example of ProxyProtocol

See my comment above, this resource should ideally be something that a user could probably just plug in and use, maybe just altering the single CIDR value for their case. They shouldn't have to go in and delete every field they aren't using.

I guess the way we are thinking about this is a little different. I agree to your point too for it to be a plugin, but there would still be some editing needed to change the field value. I don't know how much of an effort difference would it be edit a field and delete two. I mentioned all types just so we could bring out most of the information in one page for the user without having to check out the spec.

But maybe that's not the right way to think about this and I'll change it to one example.

I get where you're coming from. We just have a pattern of our examples being realistic and not just a showcase of all of the options. And our API reference is there to specifically showcase all the options.

sjberman · 2024-10-22T03:42:48Z

site/content/how-to/data-plane-configuration.md

+EOF
+```
+
+For more information, see the `NginxProxy spec` in the [API reference]({{< relref "reference/api.md" >}}).


Instead of "For more information", I'd say "For the full configuration API".

sjberman · 2024-10-22T03:43:14Z

site/content/how-to/data-plane-configuration.md

+
+For more information, see the `NginxProxy spec` in the [API reference]({{< relref "reference/api.md" >}}).
+
+{{< note >}} When sending curl requests to a server expecting proxy information, use the flag `--harproxy-protocol` to avoid broken header errors. {{< /note >}}


--haproxy-protocol is misspelled

salonichf5 requested review from a team as code owners October 18, 2024 20:08

github-actions bot added the documentation Improvements or additions to documentation label Oct 18, 2024

salonichf5 force-pushed the docs/rewrite-client-ip-settings branch from 4415cc8 to d119f84 Compare October 18, 2024 20:09

salonichf5 force-pushed the docs/rewrite-client-ip-settings branch from d119f84 to 5aa6353 Compare October 18, 2024 20:18

ADubhlaoich approved these changes Oct 21, 2024

View reviewed changes

site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved

site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved

site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved

sjberman reviewed Oct 21, 2024

View reviewed changes

salonichf5 requested a review from sjberman October 21, 2024 18:33

bjee19 requested changes Oct 21, 2024

View reviewed changes

site/content/how-to/data-plane-configuration.md Outdated Show resolved Hide resolved

salonichf5 requested a review from bjee19 October 21, 2024 21:16

bjee19 approved these changes Oct 21, 2024

View reviewed changes

sindhushiv reviewed Oct 21, 2024

View reviewed changes

site/content/how-to/data-plane-configuration.md Show resolved Hide resolved

salonichf5 and others added 7 commits October 21, 2024 14:40

update docs for rewrite client IP settings

663439b

Update site/content/how-to/data-plane-configuration.md

3cc09d3

Co-authored-by: Alan Dooley <[email protected]>

Update site/content/how-to/data-plane-configuration.md

352578d

Co-authored-by: Alan Dooley <[email protected]>

Update site/content/how-to/data-plane-configuration.md

40dc120

Co-authored-by: Alan Dooley <[email protected]>

updates based on reviews

3750bef

Update site/content/how-to/data-plane-configuration.md

bc296d7

Co-authored-by: Saylor Berman <[email protected]>

Update site/content/how-to/data-plane-configuration.md

8c4a0ba

Co-authored-by: bjee19 <[email protected]>

salonichf5 force-pushed the docs/rewrite-client-ip-settings branch from f3a223e to 8c4a0ba Compare October 21, 2024 21:40

salonichf5 merged commit cb547d6 into main Oct 21, 2024
41 checks passed

salonichf5 deleted the docs/rewrite-client-ip-settings branch October 21, 2024 22:03

sjberman reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docs for rewrite client IP settings #2701

Add docs for rewrite client IP settings #2701

salonichf5 commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

ADubhlaoich left a comment

bjee19 left a comment

sjberman left a comment

sjberman Oct 22, 2024

sjberman Oct 22, 2024

salonichf5 Oct 22, 2024

sjberman Oct 22, 2024

sjberman Oct 22, 2024

salonichf5 Oct 22, 2024

sjberman Oct 22, 2024

salonichf5 Oct 22, 2024

sjberman Oct 22, 2024

sjberman Oct 22, 2024 •

edited

Loading

sjberman Oct 22, 2024


		## Configure PROXY protocol and RewriteClientIP settings

		When the request is passed through multiple proxies or load balancers, the client IP is set to the IP address of the server that last handled the request. To preserve the original client IP address, you can configure `RewriteClientIP` settings in the `NginxProxy` resource. `RewriteClientIP` has the fields: _mode_, _trustedAddresses_ and _setIPRecursively_.


		For more information, see the `NginxProxy spec` in the [API reference]({{< relref "reference/api.md" >}}).

		{{< note >}} When sending curl requests to a server expecting proxy information, use the flag `--harproxy-protocol` to avoid broken header errors. {{< /note >}}

Add docs for rewrite client IP settings #2701

Add docs for rewrite client IP settings #2701

Conversation

salonichf5 commented Oct 18, 2024

Proposed changes

Checklist

Release notes

github-actions bot commented Oct 18, 2024

✅ Deploy Preview will be available once build job completes!

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

ADubhlaoich left a comment

Choose a reason for hiding this comment

bjee19 left a comment

Choose a reason for hiding this comment

sjberman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjberman Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2024 •

edited

Loading

sjberman Oct 22, 2024 •

edited

Loading