telemetry: resolve no-TLS link-local bind by interface#721
Open
hdwhdw wants to merge 3 commits into
Open
Conversation
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
20 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the telemetry server’s --noTLS bind-address safety checks to support SmartSwitch DPU deployments that must expose plaintext gNMI/gNOI only on a chassis-internal IPv4 link-local midplane address, while keeping the existing “loopback-only” protection as the default.
Changes:
- Introduces a default-off
--allow_no_tls_link_localflag to explicitly opt into plaintext binding on IPv4 link-local unicast addresses. - Extends
--noTLSvalidation to permit IPv4 link-local only when opted in, and rejects using that opt-in with a non-default--gnmi_vrf. - Expands unit tests to cover allowed/rejected combinations for loopback, IPv4 link-local, private IPv4, and IPv6 link-local addresses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| telemetry/telemetry.go | Adds the new opt-in flag and enforces updated --noTLS bind-address/VRF validation rules. |
| telemetry/telemetry_test.go | Updates and extends the --noTLS bind-address validation test matrix for the new behavior. |
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Comment on lines
188
to
191
| Insecure: fs.Bool("insecure", false, "Skip providing TLS cert and key, for testing only!"), | ||
| NoTLS: fs.Bool("noTLS", false, "disable TLS, for testing only!"), | ||
| NoTLSLinkLocalInterface: fs.String("no_tls_link_local_interface", "", "Bind --noTLS to the only IPv4 link-local address on this interface."), | ||
| AllowNoClientCert: fs.Bool("allow_no_client_auth", false, "When set, telemetry server will request but not require a client certificate."), |
Comment on lines
+282
to
+285
| ip := net.ParseIP(*telemetryCfg.BindAddress) | ||
| if ip == nil || !ip.IsLoopback() { | ||
| return nil, nil, fmt.Errorf("--noTLS requires --bind_address to be a loopback address (e.g. 127.0.0.1 or ::1)") | ||
| } |
Comment on lines
+396
to
+403
| func selectIPv4LinkLocalAddress(interfaceName string, addresses []net.Addr) (string, error) { | ||
| var linkLocalAddresses []string | ||
| for _, address := range addresses { | ||
| ip, _, err := net.ParseCIDR(address.String()) | ||
| if err == nil && ip.To4() != nil && ip.IsLinkLocalUnicast() { | ||
| linkLocalAddresses = append(linkLocalAddresses, ip.String()) | ||
| } | ||
| } |
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Contributor
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
telemetry/telemetry.go:406
- selectIPv4LinkLocalAddress currently parses each net.Addr via net.ParseCIDR(address.String()). If interface Addrs include *net.IPAddr entries (no CIDR mask in String()), ParseCIDR fails and the address is silently ignored, which can incorrectly report 0 link-local addresses and trigger an unnecessary timeout/failure. Consider handling *net.IPNet/*net.IPAddr directly (with a String()-parse fallback only if needed).
for _, address := range addresses {
ip, _, err := net.ParseCIDR(address.String())
if err == nil && ip.To4() != nil && ip.IsLinkLocalUnicast() {
linkLocalAddresses = append(linkLocalAddresses, ip.String())
}
| } else { | ||
| ip := net.ParseIP(*telemetryCfg.BindAddress) | ||
| if ip == nil || !ip.IsLoopback() { | ||
| return nil, nil, fmt.Errorf("--noTLS requires --bind_address to be a loopback address (e.g. 127.0.0.1 or ::1)") |
hdwhdw
changed the title
donghaolicd
self-requested a review
July 23, 2026 00:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
SmartSwitch DPUs expose plaintext gNMI/gNOI only on their chassis-internal
IPv4 link-local midplane. The NPU proxy must reach that address, but the current
--noTLSvalidation permits loopback addresses only. This makes it impossibleto restore proxy connectivity without removing the cleartext bind protection
entirely.
This is the binary-side dependency for
sonic-net/sonic-buildimage#28540.
How I did it
--no_tls_link_local_interfaceoption.--noTLS.that concrete address.
remains unavailable or ambiguous.
non-default gNMI VRF.
zero/one/multiple link-local address selection.
How to verify it
validation and compilation of the telemetry binary.
eth0-midplaneIPv4 link-local address itself.concrete midplane link-local address. Direct gNOI
System.Timeand the realNPU UDS metadata-routed DPUProxy call succeeded.
management address or loopback.
Which release branch to backport (provide reason below if selected)
Tracking issue/work item for backport/cherry-pick request (GitHub issue or Microsoft ADO): N/A
Failure type: regression
Tested branch
Test result
gNOI validation passed.
Description for the changelog
Allow explicitly selected plaintext gNMI binding to an interface's IPv4 link-local address.
Link to config_db schema for YANG module changes
N/A