Configurable wire server ip address#4274
Configurable wire server ip address#4274adjordjevic-microsoft wants to merge 2 commits intoAzure:masterfrom
Conversation
|
@adjordjevic-microsoft please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
This PR makes the Wire Server IP used to query NMAgent “GetSupportedApis” configurable via CNI conflist (wireServerIP), defaulting to the well-known Azure Wire Server IP when unset—enabling OneBox/non-standard environment testing without breaking existing behavior.
Changes:
- Adds
wireServerIPto CNINetworkConfig. - Refactors NMAgent supported-APIs URL construction into
buildNmAgentSupportedApisURL()with adefaultWireServerIP. - Updates the multitenancy Add() path to use the configurable URL and adds unit tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cni/netconfig.go | Adds wireServerIP field to allow configuring the Wire Server address via conflist. |
| cni/network/network.go | Introduces default + helper function for NMAgent URL construction; uses it in multitenancy Add() flow. |
| cni/network/network_test.go | Adds unit tests for the NMAgent supported-APIs URL builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -80,6 +80,7 @@ type NetworkConfig struct { | |||
| RuntimeConfig RuntimeConfig `json:"runtimeConfig,omitempty"` | |||
| WindowsSettings WindowsSettings `json:"windowsSettings,omitempty"` | |||
| AdditionalArgs []KVPair `json:"AdditionalArgs,omitempty"` | |||
| func buildNmAgentSupportedApisURL(wireServerIP string) string { | ||
| if wireServerIP == "" { | ||
| wireServerIP = defaultWireServerIP | ||
| } | ||
| return fmt.Sprintf("http://%s/machine/plugins/?comp=nmagent&type=GetSupportedApis", wireServerIP) | ||
| } |
| name: "custom IPv4 address with port", | ||
| wireServerIP: "10.0.0.1:8080", | ||
| expectedURL: "http://10.0.0.1:8080/machine/plugins/?comp=nmagent&type=GetSupportedApis", | ||
| }, |
| return nwInfo | ||
| } | ||
|
|
||
| func buildNmAgentSupportedApisURL(wireServerIP string) string { |
There was a problem hiding this comment.
Why not use our nmagent.Client for this? https://github.com/Azure/azure-container-networking/blob/master/nmagent/client.go#L188
There was a problem hiding this comment.
I looked into using nmagent.Client here, but nmagent.Client.SupportedAPIs() doesn't return a URL, it makes the full HTTP call and returns parsed results. Using it here would require changing the DetermineSnatFeatureOnHost interface, implementation, and mock. Since this is just to make the Wire Server IP configurable for local OneBox testing, I kept the change minimal - just making the existing hardcoded IP configurable without refactoring the production workflow.
Reason for Change:
The NMAgent supported APIs URL used in the CNI multitenancy path was previously hardcoded to the well-known Azure Wire Server IP (168.63.129.16). This made it impossible to test CNI changes on OneBox environments where the Wire Server is not reachable at that address.
This PR makes the Wire Server IP configurable via the CNI conflist configuration ("wireServerIP" field), allowing operators and developers to override it for testing in non-standard environments (e.g., OneBox, custom labs). When unset, the default 168.63.129.16 is used, preserving full backward compatibility.
Changes:
Example conflist snippet to override: