kaxil commented on code in PR #74504:
URL: https://github.com/apache/airflow/pull/74504#discussion_r4243832055
##########
providers/common/ai/src/airflow/providers/common/ai/sandbox/opensandbox.py:
##########
@@ -307,24 +314,66 @@ def _get_network_policy(spec: SandboxSpec | None) ->
NetworkPolicy | None:
# spellings work at runtime, but only the alias is in the typed
signature.
return NetworkPolicy(defaultAction="deny", egress=rules or None)
+ @staticmethod
+ def _get_egress_enforcement_mode(sandbox: SandboxSync) -> str | None:
+ """Return the mode the sandbox's egress sidecar enforces its policy
in, or ``None`` if unreported."""
+ import httpx
+ from opensandbox.adapters.converter.response_handler import
handle_api_error, require_parsed
+ from opensandbox.api.egress import Client
+ from opensandbox.api.egress.api.policy import get_policy
+ from opensandbox.api.egress.models import PolicyStatusResponse
+ from opensandbox.constants import DEFAULT_EGRESS_PORT
+
+ # get_egress_policy() keeps only the rules from the sidecar's /policy
response and drops
+ # its enforcementMode, so read that response with the SDK's generated
egress client.
+ config = sandbox.connection_config
+ endpoint = sandbox.get_endpoint(DEFAULT_EGRESS_PORT)
+ with Client(
Review Comment:
This can destroy sandboxes on a correctly configured `dns+nft` server, with
"may be running without its egress sidecar". The `Client` here gets no
`transport`, unlike `EgressAdapterSync`, which passes
`transport=connection_config.transport`:
- httpx only reads `HTTP_PROXY`/`HTTPS_PROXY` when no transport is given. On
a worker with a proxy set, this request goes via the proxy and the SDK's other
calls go direct.
- There's no `RetrySyncTransport`, so one 502/503 from the sidecar fails the
read-back.
Passing `httpx_args={"transport": config.new_owned_transport()}` to `Client`
should cover it. That transport belongs to this call, so the `with` block
closing it is fine.
##########
providers/common/ai/docs/sandbox/configuration.rst:
##########
@@ -54,8 +54,13 @@ egress there is a host-level ``sbx policy``, so the backend
honors the default b
refusing to provision unless the Deployment Manager has declared
``host_network_policy="deny-all"``, which means a bare ``SandboxSpec()`` is
refused
under the default ``host_network_policy="unknown"`` rather than silently
getting
-an open sandbox. ``allow_egress_to`` names hosts the sandbox may reach, and
both
-backends refuse it until you have said something that makes it enforceable:
+an open sandbox. On OpenSandbox the default and ``allow_egress_to`` become a
policy
+for the server's egress sidecar, and the backend refuses to provision unless
that
Review Comment:
"Refuses to provision" reads the same as the `sbx` case just above, where
nothing gets created. Here the sandbox is created, the policy is read back, and
then it's destroyed, which `backends.rst` describes correctly. Maybe "destroys
the sandbox and fails the task unless that sidecar runs in `dns+nft` mode"?
##########
providers/common/ai/src/airflow/providers/common/ai/sandbox/opensandbox.py:
##########
@@ -307,24 +314,66 @@ def _get_network_policy(spec: SandboxSpec | None) ->
NetworkPolicy | None:
# spellings work at runtime, but only the alias is in the typed
signature.
return NetworkPolicy(defaultAction="deny", egress=rules or None)
+ @staticmethod
+ def _get_egress_enforcement_mode(sandbox: SandboxSync) -> str | None:
+ """Return the mode the sandbox's egress sidecar enforces its policy
in, or ``None`` if unreported."""
+ import httpx
+ from opensandbox.adapters.converter.response_handler import
handle_api_error, require_parsed
+ from opensandbox.api.egress import Client
+ from opensandbox.api.egress.api.policy import get_policy
+ from opensandbox.api.egress.models import PolicyStatusResponse
+ from opensandbox.constants import DEFAULT_EGRESS_PORT
+
+ # get_egress_policy() keeps only the rules from the sidecar's /policy
response and drops
+ # its enforcementMode, so read that response with the SDK's generated
egress client.
+ config = sandbox.connection_config
+ endpoint = sandbox.get_endpoint(DEFAULT_EGRESS_PORT)
+ with Client(
+ base_url=f"{config.protocol}://{endpoint.endpoint}",
+ headers=endpoint.build_request_headers(config),
+ timeout=httpx.Timeout(config.request_timeout.total_seconds()),
+ ) as client:
+ response = get_policy.sync_detailed(client=client)
+ handle_api_error(response, "Get egress policy")
+ mode = require_parsed(response, PolicyStatusResponse, "Get egress
policy").enforcement_mode
+ return mode if isinstance(mode, str) else None
+
@staticmethod
def _verify_network_policy(sandbox: SandboxSync, requested: NetworkPolicy)
-> None:
wanted = {rule.target for rule in requested.egress or ()}
try:
enforced = sandbox.get_egress_policy()
allowed = {rule.target for rule in enforced.egress or () if
rule.action == "allow"}
- matches = enforced.default_action == "deny" and allowed == wanted
- detail = f"enforced policy is default {enforced.default_action!r}
with allow {sorted(allowed)}"
+ if enforced.default_action != "deny" or allowed != wanted:
+ problem = (
+ f"the enforced policy is default
{enforced.default_action!r} with allow "
+ f"{sorted(allowed)}. The server may be running without its
egress sidecar."
+ )
+ else:
+ mode = OpenSandboxBackend._get_egress_enforcement_mode(sandbox)
+ if mode == _ADDRESS_LAYER_EGRESS_MODE:
+ return
+ problem = (
+ f"its egress sidecar enforces it in {mode!r} mode"
+ if mode is not None
+ else "its egress sidecar does not report an enforcement
mode"
Review Comment:
When does this branch fire in practice? The sidecar has reported
`enforcementMode` since `docker/egress/v1.0.1` (v1.0.0's `policy_server.go` has
no such field), so `None` here most likely means a v1.0.0 sidecar, and setting
`mode = "dns+nft"` won't help that operator. Could this branch say to upgrade
the egress sidecar to v1.0.1 or later, and keep the `dns+nft` advice for the
`'dns'` branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
