Stop reimplementing region resolution; let the boto3 session do it

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- `AWS_REGION` ignored for boto compatibility; use `AWS_DEFAULT_REGION` or `INSPECT_EC2_SANDBOX_REGION`
 - custom `Ec2InstanceProvider` must drop the `region` parameter from `find_sandbox_instances()`.
 - `Ec2SandboxEnvironmentConfig.from_settings()` no longer accepts a `session` argument (use Ec2SandboxEnvironment.set_session()).
 - Custom `Ec2InstanceProvider`s are now resolved regardless of entry-point import order.

README.md

@@ -83,7 +83,6 @@ and allow the end-user to specify the rest.
 The following environment variables must be set:
 
 ```bash
-INSPECT_EC2_SANDBOX_REGION=eu-west-1
 INSPECT_EC2_SANDBOX_VPC_ID=vpc-123456
 INSPECT_EC2_SANDBOX_SECURITY_GROUP_ID=sg-56781234
 INSPECT_EC2_SANDBOX_SUBNET_ID=subnet-654321
@@ -94,12 +93,24 @@ INSPECT_EC2_SANDBOX_S3_BUCKET=ec2sandboxstack-databucket123-456
 The following environment variables are optional:
 
 ```bash
+INSPECT_EC2_SANDBOX_REGION=eu-west-1
 INSPECT_EC2_SANDBOX_AMI_ID=ami-123456
 INSPECT_EC2_SANDBOX_INSTANCE_TYPE=t3a.small
 INSPECT_EC2_SANDBOX_S3_KEY_PREFIX=sandbox-comms
 INSPECT_EC2_SANDBOX_EXTRA_TAGS_STR='tagname1=tagvalue1;tagname2=tagvalue2'
 ```
 
+`INSPECT_EC2_SANDBOX_REGION` is only needed to override the region. When it is
+unset the region comes from [boto3's standard configuration chain][boto3-config],
+which raises an error if nothing is configured.
+
+> **Note:** boto3 resolves the region from `AWS_DEFAULT_REGION`, **not**
+> `AWS_REGION`. This differs from the AWS CLI and the JavaScript/Go/Java SDKs,
+> which read `AWS_REGION`. Export `AWS_DEFAULT_REGION` (or set
+> `INSPECT_EC2_SANDBOX_REGION`).
+
+[boto3-config]: https://docs.aws.amazon.com/boto3/latest/guide/configuration.html
+
 ### Configuration
 
 As an alternative to the above environment variables you can specify the configuration directly in code, e.g

src/ec2sandbox/_instance_provider.py

@@ -15,7 +15,6 @@
 from __future__ import annotations
 
 import logging
-import os
 from dataclasses import dataclass
 from typing import TYPE_CHECKING, Any, ClassVar, Protocol, runtime_checkable
 
@@ -35,6 +34,23 @@
 # default provider's ``find_sandbox_instances`` can discover them.
 MARKER_TAG_KEY = "inspect_sandbox"
 
+# EC2 error codes meaning "this AMI ID isn't in this region". Turned into a
+# clear ValueError so the common footgun — running an eval that hardcodes an
+# AMI in a different region from the one the session resolved — is legible.
+_AMI_NOT_FOUND_CODES = frozenset(
+    {"InvalidAMIID.NotFound", "InvalidAMIID.Malformed", "InvalidAMIID.Unavailable"}
+)
+
+
+def _ami_region_mismatch_error(ami_id: str, region: str) -> ValueError:
+    return ValueError(
+        f"AMI '{ami_id}' was not found in region '{region}'. AMI IDs are "
+        "region-scoped, so an eval that hardcodes ami_id only runs in that "
+        "AMI's region. Set INSPECT_EC2_SANDBOX_REGION (or the config's region) "
+        "to the AMI's region, or omit ami_id to auto-resolve the Ubuntu 24.04 "
+        "image for the resolved region."
+    )
+
 
 @dataclass(frozen=True)
 class ProvisionedInstance:
@@ -164,10 +180,18 @@ def get_provider_session(
 
 def _root_device_name(ec2_client: Any, ami_id: str) -> str:
     """Return the root device name (e.g. ``/dev/sda1``) for ``ami_id``."""
-    resp = ec2_client.describe_images(ImageIds=[ami_id])
+    try:
+        resp = ec2_client.describe_images(ImageIds=[ami_id])
+    except ClientError as e:
+        code = e.response.get("Error", {}).get("Code")
+        if code in _AMI_NOT_FOUND_CODES:
+            raise _ami_region_mismatch_error(ami_id, ec2_client.meta.region_name) from e
+        raise
     images = resp.get("Images", [])
     if not images:
-        raise ValueError(f"AMI {ami_id} not found when resolving root device name")
+        # Empty list (no error) for an AMI that exists but this account can't
+        # see — private/deregistered in the region. Same user-facing fix hint.
+        raise _ami_region_mismatch_error(ami_id, ec2_client.meta.region_name)
     return images[0]["RootDeviceName"]
 
 
@@ -206,12 +230,9 @@ class DefaultEc2InstanceProvider:
     """Default :class:`Ec2InstanceProvider` using direct boto3 calls.
 
     Used by the EC2 sandbox when no custom provider has been registered.
-    Reads the infrastructure fields (``region``, ``security_group_id``,
+    Reads the infrastructure fields (``security_group_id``, ``subnet_id``,
     etc.) from the supplied :class:`Ec2SandboxEnvironmentConfig` at the
-    point they are needed — ``create_instance`` requires the full set,
-    while ``terminate_instance`` and ``find_sandbox_instances`` only
-    need a region (the instance's own region for terminate; the
-    configured region for find).
+    point they are needed.
     """
 
     # Process-global Ubuntu 24.04 AMI cache keyed on region. Canonical's
@@ -249,7 +270,6 @@ async def create_instance(
     ) -> ProvisionedInstance:
         cfg = self._config
         required = {
-            "region": cfg.region,
             "security_group_id": cfg.security_group_id,
             "subnet_id": cfg.subnet_id,
             "instance_profile": cfg.instance_profile,
@@ -264,11 +284,15 @@ async def create_instance(
                 "register an Ec2InstanceProvider."
             )
 
+        # Read the resolved region back so the rest of the method (AMI lookup,
+        # SSM client, ProvisionedInstance) has a concrete value, not cfg.region
+        # which may be None.
+        ec2_client = self._session.client("ec2", region_name=cfg.region)
+        region = ec2_client.meta.region_name
+
         if not ami_id:
-            assert cfg.region is not None  # validated above
-            ami_id = self._resolve_ubu24_ami(cfg.region)
+            ami_id = self._resolve_ubu24_ami(region)
 
-        ec2_client = self._session.client("ec2", region_name=cfg.region)
         instance_params: dict[str, Any] = {
             "ImageId": ami_id,
             "InstanceType": instance_type,
@@ -286,7 +310,15 @@ async def create_instance(
                     "Ebs": {"VolumeSize": volume_size},
                 }
             ]
-        response = ec2_client.run_instances(**instance_params, MinCount=1, MaxCount=1)
+        try:
+            response = ec2_client.run_instances(
+                **instance_params, MinCount=1, MaxCount=1
+            )
+        except ClientError as e:
+            code = e.response.get("Error", {}).get("Code")
+            if code in _AMI_NOT_FOUND_CODES:
+                raise _ami_region_mismatch_error(ami_id, region) from e
+            raise
         instance = response["Instances"][0]
         instance_id = instance["InstanceId"]
 
@@ -297,7 +329,7 @@ async def create_instance(
             waiter = ec2_client.get_waiter("instance_running")
             waiter.wait(InstanceIds=[instance_id])
 
-            ssm_client = self._session.client("ssm", region_name=cfg.region)
+            ssm_client = self._session.client("ssm", region_name=region)
             _wait_for_ssm(instance_id, ssm_client)
         except BaseException:
             try:
@@ -310,11 +342,10 @@ async def create_instance(
                 )
             raise
 
-        assert cfg.region is not None  # validated above
         assert cfg.s3_bucket is not None  # validated above
         return ProvisionedInstance(
             instance_id=instance_id,
-            region=cfg.region,
+            region=region,
             s3_bucket=cfg.s3_bucket,
             s3_key_prefix=cfg.s3_key_prefix,
         )
@@ -324,14 +355,11 @@ async def terminate_instance(self, instance_id: str, region: str) -> None:
         ec2.terminate_instances(InstanceIds=[instance_id])
 
     async def find_sandbox_instances(self) -> list[SandboxInstanceInfo]:
-        # cli_cleanup builds this provider with an empty config (no region),
-        # so fall back to AWS_REGION / AWS_DEFAULT_REGION as the session does.
-        region = (
-            self._config.region
-            or os.getenv("AWS_REGION")
-            or os.getenv("AWS_DEFAULT_REGION")
-        )
-        ec2 = self._session.client("ec2", region_name=region or None)
+        # cli_cleanup builds this provider with an empty config, so region is
+        # usually None here and the session resolves it. Read the resolved
+        # region back off the client to stamp on each SandboxInstanceInfo.
+        ec2 = self._session.client("ec2", region_name=self._config.region)
+        region = ec2.meta.region_name
         response = ec2.describe_instances(
             Filters=[
                 {"Name": f"tag:{MARKER_TAG_KEY}", "Values": ["true"]},
@@ -353,7 +381,7 @@ async def find_sandbox_instances(self) -> list[SandboxInstanceInfo]:
                     SandboxInstanceInfo(
                         instance_id=instance["InstanceId"],
                         name=name,
-                        region=region or "",
+                        region=region,
                     )
                 )
         return results

src/ec2sandbox/schema.py

@@ -5,7 +5,6 @@
  Inspect EC2 sandbox environments.
 """
 
-import os
 from typing import Optional, Tuple
 
 from pydantic import BaseModel, ConfigDict
@@ -66,10 +65,13 @@ class Ec2SandboxEnvironmentConfig(BaseModel):
     s3_key_prefix: str = ""
     volume_size: Optional[int] = None
 
+    # Optional explicit region override. None -> the boto3 session resolves the
+    # region when it builds a client (see README); a set value overrides that.
+    region: Optional[str] = None
+
     # Direct-EC2-path fields — required when no Ec2InstanceProvider is
     # registered, ignored otherwise. ``sample_init`` validates these at
     # call time when the direct path is taken.
-    region: Optional[str] = None
     # TODO is vpc_id actually needed? We could just force a subnet ID.
     vpc_id: Optional[str] = None
     security_group_id: Optional[str] = None
@@ -102,16 +104,6 @@ def from_settings(cls, **kwargs):
         # Override with any provided kwargs
         params.update(kwargs)
 
-        region = params["region"]
-        if region is None:
-            region = os.getenv("AWS_REGION")
-        if not isinstance(region, str):
-            raise ValueError(
-                "Region must be specified either in settings,"
-                f" or as an environment variable {env_prefix}REGION or AWS_REGION."
-            )
-        params["region"] = region
-
         # AMI resolution is deferred to DefaultEc2InstanceProvider.create_instance
         # so that callers who only need terminate/find don't pay for an SSM
         # AMI lookup just to construct a config.

tests/ec2sandboxtest/test_from_settings.py

@@ -36,23 +36,26 @@ def test_kwarg_overrides_env_var() -> None:
     assert config.instance_type == "t3a.xlarge"
 
 
-def test_region_falls_back_to_aws_region() -> None:
-    """AWS_REGION is used when INSPECT_EC2_SANDBOX_REGION is unset."""
-    env_vars = env_vars_all()
-    env_vars.pop("INSPECT_EC2_SANDBOX_REGION")
-    env_vars["AWS_REGION"] = "eu-west-1"
-    with mock.patch.dict(os.environ, env_vars, clear=True):
+def test_explicit_region_env_var_sets_region() -> None:
+    """INSPECT_EC2_SANDBOX_REGION is carried through as an explicit override."""
+    with mock.patch.dict(os.environ, env_vars_all(), clear=True):
         config = Ec2SandboxEnvironmentConfig.from_settings()
-    assert config.region == "eu-west-1"
+    assert config.region == "eu-west-2"
+
 
+def test_region_left_none_for_session_to_resolve() -> None:
+    """Without INSPECT_EC2_SANDBOX_REGION, from_settings leaves region None.
 
-def test_missing_region_raises_value_error() -> None:
-    """With no region from any source, from_settings raises a clear error."""
+    Region resolution is deferred to the boto3 session at client-construction
+    time, so from_settings must not reimplement the chain by reading AWS_REGION
+    itself — that would shadow ~/.aws/config and the rest of botocore's chain.
+    """
     env_vars = env_vars_all()
     env_vars.pop("INSPECT_EC2_SANDBOX_REGION")
+    env_vars["AWS_REGION"] = "eu-west-1"
     with mock.patch.dict(os.environ, env_vars, clear=True):
-        with pytest.raises(ValueError, match="Region must be specified"):
-            Ec2SandboxEnvironmentConfig.from_settings()
+        config = Ec2SandboxEnvironmentConfig.from_settings()
+    assert config.region is None
 
 
 def test_s3_key_prefix_leading_slash_raises() -> None:

tests/ec2sandboxtest/test_instance_provider.py

@@ -1,6 +1,7 @@
 from unittest import mock
 
 import pytest
+from botocore.exceptions import ClientError
 
 from ec2sandbox._instance_provider import DefaultEc2InstanceProvider
 from ec2sandbox.schema import Ec2SandboxEnvironmentConfig
@@ -105,6 +106,82 @@ async def test_create_instance_resolves_and_caches_ami_when_empty():
     assert ssm_client.get_parameters.call_count == 1
 
 
+@pytest.mark.asyncio
+async def test_create_instance_stamps_session_resolved_region():
+    """With no region in config, it's resolved off the client and stamped.
+
+    The ec2 client is built with region_name=None so the session resolves the
+    region; ProvisionedInstance.region then carries the concrete value.
+    """
+    provider, ec2_client, _ = _make_provider_with_mocks(_make_config(region=None))
+    ec2_client.meta.region_name = "us-east-1"
+
+    result = await provider.create_instance(
+        instance_type="t3a.micro",
+        ami_id="ami-123",
+        tags=[("Name", "x")],
+    )
+
+    assert result.region == "us-east-1"
+    ec2_call = next(
+        c for c in provider._session.client.call_args_list if c.args[0] == "ec2"
+    )
+    assert ec2_call.kwargs["region_name"] is None
+
+
+@pytest.mark.asyncio
+async def test_run_instances_ami_not_found_raises_region_hint():
+    """A region-scoped AMI missing in the resolved region gets a clear error."""
+    provider, ec2_client, _ = _make_provider_with_mocks(_make_config())
+    ec2_client.meta.region_name = "us-east-1"
+    ec2_client.run_instances.side_effect = ClientError(
+        {"Error": {"Code": "InvalidAMIID.NotFound", "Message": "nope"}},
+        "RunInstances",
+    )
+
+    with pytest.raises(ValueError, match="region-scoped"):
+        await provider.create_instance(
+            instance_type="t3a.micro",
+            ami_id="ami-123",
+            tags=[("Name", "x")],
+        )
+
+
+@pytest.mark.asyncio
+async def test_volume_size_ami_empty_result_raises_region_hint():
+    """describe_images returning no images (private/deregistered AMI) is translated."""
+    provider, ec2_client, _ = _make_provider_with_mocks(_make_config(volume_size=100))
+    ec2_client.meta.region_name = "us-east-1"
+    ec2_client.describe_images.return_value = {"Images": []}
+
+    with pytest.raises(ValueError, match="region-scoped"):
+        await provider.create_instance(
+            instance_type="t3a.micro",
+            ami_id="ami-123",
+            tags=[("Name", "x")],
+            volume_size=100,
+        )
+
+
+@pytest.mark.asyncio
+async def test_volume_size_ami_not_found_error_raises_region_hint():
+    """A foreign-region AMI makes describe_images raise NotFound; translate it too."""
+    provider, ec2_client, _ = _make_provider_with_mocks(_make_config(volume_size=100))
+    ec2_client.meta.region_name = "us-east-1"
+    ec2_client.describe_images.side_effect = ClientError(
+        {"Error": {"Code": "InvalidAMIID.NotFound", "Message": "nope"}},
+        "DescribeImages",
+    )
+
+    with pytest.raises(ValueError, match="region-scoped"):
+        await provider.create_instance(
+            instance_type="t3a.micro",
+            ami_id="ami-123",
+            tags=[("Name", "x")],
+            volume_size=100,
+        )
+
+
 @pytest.mark.asyncio
 async def test_create_instance_with_volume_size_sets_block_device_mappings():
     provider, ec2_client, _ = _make_provider_with_mocks(_make_config(volume_size=100))