Skip to content

fix(secure-gateway): align weights proxy builder with wrap_url (#2662) - #2748

Open
arthi-arumugam-git wants to merge 1 commit into
roboflow:mainfrom
arthi-arumugam-git:fix/secure-gateway-proxy-builder-parity
Open

fix(secure-gateway): align weights proxy builder with wrap_url (#2662)#2748
arthi-arumugam-git wants to merge 1 commit into
roboflow:mainfrom
arthi-arumugam-git:fix/secure-gateway-proxy-builder-parity

Conversation

@arthi-arumugam-git

Copy link
Copy Markdown

Closes the first half of #2662.

roboflow_secure_gateway_proxy_url_builder and wrap_url implement the same
/proxy?url= contract, one for weights traffic and one for server traffic. They had
drifted apart in two ways, both exactly as @alexnorell described:

1. No idempotence guard. An already-wrapped download_url was wrapped again. On main:

https://gw.local/proxy?url=https%3A%2F%2Fgw.local%2Fproxy%3Furl%3Dhttps%253A%252F%252Fapi.roboflow.com%252Fweights

wrap_url gained this guard in #2658; the weights builder did not.

2. The gateway base path was dropped. The builder took urlsplit scheme and netloc
only, so SECURE_GATEWAY=https://gw.local/edge sent weights traffic to gw.local/proxy
while server traffic went to gw.local/edge/proxy.

Neither one raises. The weights are simply fetched from somewhere other than the gateway
you configured, which is the failure mode a secure gateway exists to prevent.

The fix

The builder now mirrors wrap_url line for line: rstrip("/") on the configured value so
the base path survives, then the same gateway_prefix construction and the same
startswith guard.

The two fixes interact, which is worth stating because it is the part that is easy to get
wrong. The idempotence guard has to compare against a prefix that already includes the base
path, otherwise fixing the path reintroduces double-wrapping under /edge. There is a test
for that case specifically.

Tests

Four in inference_models/tests/unit_tests/weights_providers/test_roboflow.py:
idempotence, base path preserved, base path with a trailing slash, and idempotence under a
base path. All four fail on main.

One parametrized cross-package test in tests/inference/unit_tests/core/utils/test_url_utils.py
asserting the two wrappers produce byte-identical output across six gateway spellings, and
that both are idempotent. Six cases, all failing on main, including the plain
gateway.local case because the idempotence half diverged for every spelling.

The issue suggested either sharing one implementation or keeping them behaviour-identical
with cross-package tests. Sharing would mean a dependency between the two packages, so this
takes the second option. The parity test lives in the inference unit suite rather than the
inference_models one because that workflow already does pip install --no-deps ./inference_models, so both are importable there, whereas the inference_models suite runs
with working-directory: inference_models. It is guarded with importorskip regardless.

Not in scope

Part 2 of #2662, the per-run step_execution_mode bypass, is untouched. That one needs
gateway support in inference_sdk's InferenceHTTPClient or a guard at the
StepExecutionMode consumption point, and it is a design decision rather than a divergence
between two functions.

Verification

test_url_utils.py: 12 passed with the fix, 6 failed and 6 passed without it.
test_roboflow.py (weights providers): 72 passed with the fix, 68 passed without it plus
the 4 new failures. test_roboflow_api.py is byte-identical before and after, so nothing
that routes through these wrappers changed behaviour. black and isort clean per the
style target.

roboflow_secure_gateway_proxy_url_builder diverged from wrap_url in two ways,
both reported in roboflow#2662:

- No idempotence guard, so an already-wrapped download_url was proxied again:
  {gateway}/proxy?url={gateway}%2Fproxy%3Furl%3D... wrap_url gained this guard
  in roboflow#2658 and the weights builder did not.
- The gateway base path was dropped. urlsplit scheme+netloc discards any path,
  so SECURE_GATEWAY=https://gw.local/edge sent weights traffic to
  gw.local/proxy while server traffic went to gw.local/edge/proxy.

Neither failure raises: weights are simply fetched from somewhere other than
the configured gateway.

The two packages cannot share an implementation without a dependency between
them, so parity is pinned by a cross-package test in the inference unit suite,
where inference_models is already installed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant