Skip to content

Application Gateway L4 Client IP Preservation Support #37340

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.

Sign up for GitHub

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 2 commits into from
Oct 3, 2025
Merged

Application Gateway L4 Client IP Preservation Support #37340

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffa8dee
added new feature properties
akshimittal1310 Sep 11, 2025
2816fe9
Merge branch 'kamboj-prjwl/release-network-2025-01-01' into akshimitt…
kamboj-prjwl Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ...tion/network/resource-manager/Microsoft.Network/stable/2025-01-01/applicationGateway.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{

Check notice on line 1 in specification/network/resource-manager/Microsoft.Network/stable/2025-01-01/applicationGateway.json

View workflow job for this annotation

GitHub Actions / TypeSpec Requirement 

Your service description will soon be required to convert from OpenAPI to TypeSpec. See https://aka.ms/azsdk/typespec.
"swagger": "2.0",
"info": {
"title": "NetworkManagementClient",
Copy link
Contributor

@sandipsh sandipsh Sep 12, 2025

Choose a reason for hiding this comment

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

enableProbeProxyProtocolHeader

is this going to be part of new api version? work-in-progress?

Copy link
Contributor Author

@akshimittal1310 akshimittal1310 Sep 13, 2025

Choose a reason for hiding this comment

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

Yes, the NRP rollout is already done

https://msazure.visualstudio.com/One/_git/Networking-nrp/pullrequest/11590002

The RP manifest has rolled out as well

Copy link
Contributor

@gary-x-li gary-x-li Sep 18, 2025

Choose a reason for hiding this comment

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

enableProbeProxyProtocolHeader

Consider changing it to enum so that you can add more future values instead of adding more boolean properties.

More context on the topic:
ARM recommends enums over booleans for future proof APIs. Standard guidance is: replace boolean/switch properties with a more meaningful enum whenever possible.

A boolean will forever have two valid values (true or false). A string enum type is always preferred. Also, properties should always provide better values just than True and False. For example two switches "isTypeA" and "isTypeB" should be replaced with one enum "type": [A, B, DefaultType]. Enums are always a more flexible and future proof option because they allow additional values to be added in the future in a non-breaking way, e.g. [Enabled, Disabled, Suspended, Deallocated].

Note: do NOT define a 'boolean enum' with two values 'True and False'. This might be easier to 'extend' in terms of types, but semantically its confusing, and no better than a boolean.

Copy link
Contributor Author

@akshimittal1310 akshimittal1310 Oct 3, 2025

Choose a reason for hiding this comment

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

Thanks Gary for the detailed review and for highlighting this point. I do understand the ARM guidance around using enums over booleans, and agree with the rationale, enums definitely provide more flexibility and allow for future extensibility.

In this specific case, the feature is inherently binary and was designed to always remain that way. The RP changes with this property as a boolean have already been released and were reviewed/accepted by the ARM team earlier during private preview. Revisiting the design now and reworking it into an enum would have a large downstream impact on our RP, SDKs, and clients, and would likely delay our GA timeline.

Given these constraints, I would prefer to keep the current design unchanged for this release, but I’ll ensure this feedback is carried forward and that we adopt the enum-first approach in future features where extensibility is a possibility. Hope this sounds reasonable

Expand Down Expand Up @@ -1176,6 +1176,10 @@
"type": "boolean",
"description": "Whether the host header should be picked from the backend http settings. Default value is false."
},
"enableProbeProxyProtocolHeader": {
"type": "boolean",
"description": "Whether to send Proxy Protocol header along with the Health Probe over TCP or TLS protocol. Default value is false."
},
"match": {
"$ref": "#/definitions/ApplicationGatewayProbeHealthResponseMatch",
"description": "Criterion for classifying a healthy probe response."
Copy link
Contributor

@gary-x-li gary-x-li Sep 18, 2025

Choose a reason for hiding this comment

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

enableL4ClientIpPreservation

Same boolean vs enum comment as above.

Plus, the description for this property is very similar to the other one. It doesn't match with its name.

Copy link
Contributor Author

@akshimittal1310 akshimittal1310 Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For the enum comment: please refer to the previous answer

The properties are not the same, these are part of 2 different object models as part of the whole resource. It is not necessary that proxy protocol in probe (enableProbeProxyProtocolHeader) needs to be enabled for it to be associated with a backend setting with proxy protocol support (enableL4ClientIpPreservation), they can work independently. One of them is for health probes and the other is for customer datapath traffic

Expand Down Expand Up @@ -1970,6 +1974,10 @@
"type": "boolean",
"description": "Whether to pick server name indication from the host name of the backend server for Tls protocol. Default value is false."
},
"enableL4ClientIpPreservation": {
"type": "boolean",
"description": "Whether to send Proxy Protocol header to backend servers over TCP or TLS protocols. Default value is false."
},
"provisioningState": {
"readOnly": true,
"$ref": "./network.json#/definitions/ProvisioningState",
Expand Down Expand Up @@ -2331,6 +2339,10 @@
"$ref": "#/definitions/ApplicationGatewayProbeHealthResponseMatch",
"description": "Criterion for classifying a healthy probe response."
},
"enableProbeProxyProtocolHeader": {
"type": "boolean",
"description": "Whether to send Proxy Protocol header along with the Health Probe over TCP or TLS protocol. Default value is false."
},
"provisioningState": {
"readOnly": true,
"$ref": "./network.json#/definitions/ProvisioningState",
Expand Down