Skip to content

[ISSUE] update_segment sends empty port-range lists for the port keys the caller did not pass, clearing them #580

Description

@hackerboey

[ISSUE] update_segment sends empty port-range lists for the port keys the caller did not pass, clearing them

Environment

SDK version zscaler-sdk-python 1.9.44 (editable install of master @ e7f5f7ef)
Python 3.11.15
OS macOS (arm64)
Affected product / resource ZPA / application_segment

Found by reading the SDK source and confirmed with the offline reproduction below (a stub
request executor, so no credentials and no network are involved). All identifiers used are
synthetic.


Description

ApplicationSegmentAPI.update_segment builds the PUT body directly from kwargs with no
merge against the segment's current state:

# zscaler/zpa/application_segment.py:396
body = kwargs

and then, for each of the four port keys, explicitly writes an empty list when the caller
did not supply one
:

# zscaler/zpa/application_segment.py:435-453
if "tcp_port_ranges" in body:
    body["tcpPortRanges"] = body.pop("tcp_port_ranges")
else:
    body["tcpPortRanges"] = []

if "tcp_port_range" in body:
    body["tcpPortRange"] = [{"from": pr["from"], "to": pr["to"]} for pr in body.pop("tcp_port_range")]
else:
    body["tcpPortRange"] = []

if "udp_port_ranges" in body:
    body["udpPortRanges"] = body.pop("udp_port_ranges")
else:
    body["udpPortRanges"] = []  # Explicitly clear if not provided

if "udp_port_range" in body:
    body["udpPortRange"] = [{"from": pr["from"], "to": pr["to"]} for pr in body.pop("udp_port_range")]
else:
    body["udpPortRange"] = []  # Explicitly clear if not provided

So a caller who only wants to change, say, the description ends up sending a body that also
zeroes all four port keys. The affected keys are exactly these four — tcpPortRanges, tcpPortRange, udpPortRanges
and udpPortRange. No other key in the method is set to [] on the omitted-key path.

The remaining fields (name, domainNames, segmentGroupId, serverGroups, …) are not
explicitly cleared, but because body = kwargs they are simply absent from the PUT, so
whether they survive depends entirely on the server's PUT semantics. The four port keys are
different in that they are cleared unconditionally, regardless of those semantics.

Two things in the codebase suggest the explicit-[] behaviour is an oversight rather than a
deliberate contract:

  • update_access_rule in the same service filters unset values out of its PUT payload —
    payload = {k: v for k, v in payload.items() if v is not None}, with the comment
    "Filter out None values if you prefer not to send them" (zscaler/zpa/policies.py:710).
    zscaler/ztw/location_template.py does the same.
  • APIClient.format_request_body (zscaler/api_client.py:81-94) skips None values by
    design (if val is None: continue). It is present in the base class but does not appear to
    be referenced anywhere outside its own recursive call, so the ZPA update path never picks up
    that convention.
  • add_segment (zscaler/zpa/application_segment.py:283-291) handles the same four keys with
    if/elif and no clearing branch, so an omitted key is simply not sent. The asymmetry is
    only in update_segment.

Possibly related: #571 raises the same read-modify-write concern for policy rules, including
"appConnectorGroups": [...] or [] surviving the None filter for the same reason.

Reproduction

Fully offline — no credentials, no network.

import json

from zscaler.zpa.application_segment import ApplicationSegmentAPI

SEGMENT_ID = "72058304855090128"
CONFIG = {"client": {"customerId": "72058304855015000"}}


class StubResponse:
    def __init__(self, body):
        self._body = body

    def get_body(self):
        return self._body

    def get_results(self):
        return []

    def get_status(self):
        return 200


class StubExecutor:
    """Stands in for RequestExecutor: records requests, replies with an empty body."""

    def __init__(self):
        self.requests = []

    def create_request(self, method, endpoint, body=None, headers=None, params=None,
                       use_raw_data_for_body=False):
        req = {"method": method, "endpoint": endpoint.strip(),
               "body": {} if body is None else body, "params": params or {}}
        self.requests.append(req)
        return req, None

    def execute(self, request, response_type=None, return_raw_response=False):
        return StubResponse({}), None


ex = StubExecutor()
seg, resp, err = ApplicationSegmentAPI(ex, CONFIG).update_segment(
    SEGMENT_ID, description="new description")

put = [r for r in ex.requests if r["method"] == "PUT"][0]
print("PUT body actually sent:")
print(json.dumps(put["body"], indent=2, sort_keys=True))
print("err =", err)

The equivalent live call, on a segment that has TCP ports configured:

client.zpa.application_segment.update_segment(
    segment_id="72058304855090128",
    description="new description",
)

Expected behaviour

A caller who does not mention the port keys should not have them modified. Either the keys
should be omitted from the request body, or the method should merge them from the segment's
current state, or the destructive behaviour should be documented on the method so it is a
choice rather than a surprise.

Actual behaviour

PUT body actually sent:
{
  "description": "new description",
  "tcpPortRange": [],
  "tcpPortRanges": [],
  "udpPortRange": [],
  "udpPortRanges": []
}
err = None

Root cause

Location Code
zscaler/zpa/application_segment.py:396 body = kwargs — no merge with current state
zscaler/zpa/application_segment.py:438 body["tcpPortRanges"] = [] when omitted
zscaler/zpa/application_segment.py:443 body["tcpPortRange"] = [] when omitted
zscaler/zpa/application_segment.py:448 body["udpPortRanges"] = [] when omitted
zscaler/zpa/application_segment.py:453 body["udpPortRange"] = [] when omitted

Suggested direction

Without knowing whether update_segment is meant to be a full-replacement PUT or a partial
update, I'd rather ask than propose a patch. The options I can see:

  • Drop the four else branches so unmentioned keys are simply not sent. Smallest diff, but it
    removes any way to clear port ranges, unless passing [] explicitly is treated as "clear"
    (which would work, since the presence check would then be meaningful).
  • Reuse the {k: v for k, v in ... if v is not None} filter already used by
    update_access_rule, or wire up APIClient.format_request_body, so the whole body follows
    one convention rather than two.
  • Keep the current behaviour and document update_segment as a full replacement, so callers
    know they must read-modify-write.

I'm happy to open a PR for whichever of these you'd prefer — the first is a four-line change,
but I don't want to guess at the intended contract.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions