Skip to content

Commit ab4de23

Browse files
committed
feat: preflight-check batch:TagResource when tags are configured
When tags are set (via --aws-batch-tags or SNAKEMAKE_AWS_BATCH_JOB_TAGS), every job is tagged at submit time, so a missing batch:TagResource surfaces as an opaque AccessDenied an hour into the run. Extend _preflight_validate with a non-destructive tag/untag round-trip on the job queue ARN so the permission gap fails fast at startup instead. Only a permissions error raises; any other error (or no tags / no queue ARN) degrades to a no-op. Adds tag_resource/untag_resource wrappers to BatchClient and documents the batch:TagResource/UntagResource needs.
1 parent 826a5b0 commit ab4de23

4 files changed

Lines changed: 205 additions & 2 deletions

File tree

docs/further.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,19 @@ misconfiguration (a disabled/invalid queue or compute environment, `maxvCpus=0`,
261261
or a non-existent job role) fails fast with a clear error, so you don't wait for
262262
jobs that could never start.
263263

264+
When tags are configured (via `--aws-batch-tags` or the
265+
`SNAKEMAKE_AWS_BATCH_JOB_TAGS` environment variable), the executor also runs a
266+
tag/untag round-trip on the job queue as a best-effort *proxy* for the
267+
`batch:TagResource` permission — otherwise a missing tag permission would only
268+
surface as an opaque `AccessDenied` at submission time. This probes the queue,
269+
whereas jobs are tagged on the job and job-definition resources, so an IAM
270+
policy that scopes `batch:TagResource` per resource cannot be fully verified
271+
this way — treat a pass or failure as a strong hint, not a guarantee. The check
272+
needs `batch:TagResource` and `batch:UntagResource` (and possibly
273+
`ecs:TagResource`, depending on your account's tag-authorization settings); if
274+
the cleanup untag is denied, the throwaway `snakemake-preflight` tag is left on
275+
the queue.
276+
264277
The check is deliberately conservative about *uncertainty*: a transient API
265278
error, a queue mid-update (`status` `CREATING`/`UPDATING`), or a missing
266279
`batch:Describe*` / `iam:GetRole` permission is treated as a warning and the

snakemake_executor_plugin_aws_batch/__init__.py

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
__email__ = "jake.vancampen7@gmail.com"
44
__license__ = "MIT"
55

6+
import os
67
from dataclasses import dataclass, field
78
from pprint import pformat
89
from typing import List, AsyncGenerator, Optional
@@ -11,7 +12,10 @@
1112
from botocore.exceptions import ClientError
1213

1314
from snakemake_executor_plugin_aws_batch.batch_client import BatchClient
14-
from snakemake_executor_plugin_aws_batch.batch_job_builder import BatchJobBuilder
15+
from snakemake_executor_plugin_aws_batch.batch_job_builder import (
16+
BatchJobBuilder,
17+
SNAKEMAKE_AWS_BATCH_JOB_TAGS_ENV_VAR,
18+
)
1519
from snakemake_interface_executor_plugins.executors.base import SubmittedJobInfo
1620
from snakemake_interface_executor_plugins.executors.remote import RemoteExecutor
1721
from snakemake_interface_executor_plugins.settings import (
@@ -30,6 +34,18 @@
3034
FATAL_BATCH_STATUSES = frozenset({"INVALID", "DELETING", "DELETED"})
3135

3236

37+
def _is_access_denied(error: ClientError) -> bool:
38+
"""True if a botocore ClientError is a permissions failure (vs. transient)."""
39+
response = getattr(error, "response", {}) or {}
40+
code = response.get("Error", {}).get("Code", "")
41+
status = response.get("ResponseMetadata", {}).get("HTTPStatusCode")
42+
return code in {
43+
"AccessDenied",
44+
"AccessDeniedException",
45+
"UnauthorizedOperation",
46+
} or status in (401, 403)
47+
48+
3349
# Optional:
3450
# Define additional settings for your executor.
3551
# They will occur in the Snakemake CLI as --<executor-name>-<param-name>
@@ -362,6 +378,72 @@ def _preflight_validate(self) -> None:
362378
"environment(s)."
363379
)
364380
self._validate_job_role()
381+
self._preflight_check_tags()
382+
383+
def _preflight_check_tags(self) -> None:
384+
"""Best-effort probe of ``batch:TagResource`` when tags are configured.
385+
386+
When tags are set (via ``--aws-batch-tags`` or the
387+
``SNAKEMAKE_AWS_BATCH_JOB_TAGS`` env var) every job is tagged at submit
388+
time, so a missing ``batch:TagResource`` would otherwise surface as an
389+
opaque ``AccessDenied`` an hour into the run. This runs a tag/untag
390+
round-trip on the job queue ARN as a *proxy* for that permission and
391+
fails fast if it is denied.
392+
393+
The probe is a heuristic, not a proof: jobs are tagged on the *job* and
394+
*job-definition* resources (via ``submit_job`` / ``register_job_definition``),
395+
not the queue, so an IAM policy that scopes ``batch:TagResource`` per
396+
resource-type/ARN could grant it on the queue while denying it on the
397+
job/job-definition resources (or the reverse). Treat a pass as "the
398+
permission is very likely present" and a denial as "very likely
399+
missing", not a guarantee either way.
400+
401+
Best-effort: only a permissions denial raises; any other error (or a
402+
missing queue ARN / no tags configured) is a no-op. If the cleanup
403+
untag fails, the throwaway ``snakemake-preflight`` tag is left on the
404+
job queue (logged at debug) — so the round-trip is not guaranteed to be
405+
fully non-destructive.
406+
"""
407+
if not self._tags_configured():
408+
return
409+
queue_arn = getattr(self.settings, "job_queue", None)
410+
if not queue_arn:
411+
return
412+
413+
try:
414+
self.batch_client.tag_resource(
415+
resourceArn=queue_arn, tags={"snakemake-preflight": "1"}
416+
)
417+
# Clean up the throwaway tag; failure to untag is non-fatal.
418+
try:
419+
self.batch_client.untag_resource(
420+
resourceArn=queue_arn, tagKeys=["snakemake-preflight"]
421+
)
422+
except Exception as e:
423+
self.logger.debug(
424+
"could not remove throwaway 'snakemake-preflight' tag from "
425+
f"the job queue (left in place): {e}"
426+
)
427+
except ClientError as e:
428+
if _is_access_denied(e):
429+
raise WorkflowError(
430+
"AWS Batch preflight check failed — the executor role was "
431+
"denied batch:TagResource on the job queue, but tags are "
432+
"configured. Job tagging at submit time would very likely "
433+
"fail with AccessDenied. Grant batch:TagResource (and "
434+
"ecs:TagResource if your account requires it) on the job / "
435+
"job-definition resources, or unset the tags."
436+
) from e
437+
self.logger.debug(f"could not verify batch:TagResource: {e}")
438+
except Exception as e:
439+
self.logger.debug(f"could not verify batch:TagResource: {e}")
440+
441+
def _tags_configured(self) -> bool:
442+
"""True if any tags are set via the setting or the env var."""
443+
settings_tags = getattr(self.settings, "tags", None)
444+
if isinstance(settings_tags, dict) and settings_tags:
445+
return True
446+
return bool(os.environ.get(SNAKEMAKE_AWS_BATCH_JOB_TAGS_ENV_VAR, "").strip())
365447

366448
def _queue_problems(self) -> Optional[List[str]]:
367449
"""Return definitive job-queue / compute-environment misconfigurations.

snakemake_executor_plugin_aws_batch/batch_client.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,3 +91,21 @@ def describe_compute_environments(self, **kwargs):
9191
:return: The response from the describe_compute_environments method.
9292
"""
9393
return self.client.describe_compute_environments(**kwargs)
94+
95+
def tag_resource(self, **kwargs):
96+
"""
97+
Add tags to an AWS Batch resource.
98+
99+
:param kwargs: The keyword arguments to pass to the tag_resource method.
100+
:return: The response from the tag_resource method.
101+
"""
102+
return self.client.tag_resource(**kwargs)
103+
104+
def untag_resource(self, **kwargs):
105+
"""
106+
Remove tags from an AWS Batch resource.
107+
108+
:param kwargs: The keyword arguments to pass to the untag_resource method.
109+
:return: The response from the untag_resource method.
110+
"""
111+
return self.client.untag_resource(**kwargs)

tests/test_preflight.py

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,24 @@
77
permission).
88
"""
99

10+
import os
1011
from types import SimpleNamespace
1112
from unittest.mock import MagicMock, patch
1213

1314
import pytest
1415
from botocore.exceptions import ClientError
1516
from snakemake_interface_common.exceptions import WorkflowError
1617

17-
from snakemake_executor_plugin_aws_batch import Executor
18+
from snakemake_executor_plugin_aws_batch import Executor, _is_access_denied
19+
from snakemake_executor_plugin_aws_batch.batch_job_builder import (
20+
SNAKEMAKE_AWS_BATCH_JOB_TAGS_ENV_VAR,
21+
)
22+
23+
24+
def _env_without_tags() -> dict:
25+
return {
26+
k: v for k, v in os.environ.items() if k != SNAKEMAKE_AWS_BATCH_JOB_TAGS_ENV_VAR
27+
}
1828

1929

2030
def _executor(**settings) -> Executor:
@@ -239,6 +249,86 @@ def test_no_queue_configured_does_not_raise(self):
239249
ex._validate_job_role.assert_called_once()
240250

241251

252+
class TestPreflightCheckTags:
253+
"""The tag/untag round-trip that verifies batch:TagResource when tags are set."""
254+
255+
def _access_denied(self) -> ClientError:
256+
return ClientError({"Error": {"Code": "AccessDenied"}}, "TagResource")
257+
258+
def test_no_tags_configured_skips(self):
259+
ex = _executor()
260+
with patch.dict(os.environ, _env_without_tags(), clear=True):
261+
ex._preflight_check_tags()
262+
ex.batch_client.tag_resource.assert_not_called()
263+
264+
def test_no_queue_skips(self):
265+
ex = _executor(job_queue=None, tags={"Env": "prod"})
266+
ex._preflight_check_tags()
267+
ex.batch_client.tag_resource.assert_not_called()
268+
269+
def test_success_tags_then_untags(self):
270+
ex = _executor(tags={"Env": "prod"})
271+
ex._preflight_check_tags() # must not raise
272+
ex.batch_client.tag_resource.assert_called_once()
273+
ex.batch_client.untag_resource.assert_called_once()
274+
275+
def test_env_var_tags_trigger_check(self):
276+
ex = _executor()
277+
with patch.dict(
278+
os.environ, {SNAKEMAKE_AWS_BATCH_JOB_TAGS_ENV_VAR: "Team=data"}
279+
):
280+
ex._preflight_check_tags()
281+
ex.batch_client.tag_resource.assert_called_once()
282+
283+
def test_missing_permission_raises(self):
284+
ex = _executor(tags={"Env": "prod"})
285+
ex.batch_client.tag_resource.side_effect = self._access_denied()
286+
with pytest.raises(WorkflowError, match="batch:TagResource"):
287+
ex._preflight_check_tags()
288+
289+
def test_non_permission_error_degrades(self):
290+
ex = _executor(tags={"Env": "prod"})
291+
ex.batch_client.tag_resource.side_effect = ClientError(
292+
{"Error": {"Code": "ThrottlingException"}}, "TagResource"
293+
)
294+
ex._preflight_check_tags() # must not raise
295+
296+
@pytest.mark.parametrize("code", ["AccessDeniedException", "UnauthorizedOperation"])
297+
def test_alternate_access_denied_codes_raise(self, code):
298+
ex = _executor(tags={"Env": "prod"})
299+
ex.batch_client.tag_resource.side_effect = ClientError(
300+
{"Error": {"Code": code}}, "TagResource"
301+
)
302+
with pytest.raises(WorkflowError, match="batch:TagResource"):
303+
ex._preflight_check_tags()
304+
305+
def test_http_forbidden_status_is_access_denied(self):
306+
# A permissions failure signalled only by HTTP 403 (unrecognized code).
307+
err = ClientError(
308+
{
309+
"Error": {"Code": "Forbidden"},
310+
"ResponseMetadata": {"HTTPStatusCode": 403},
311+
},
312+
"TagResource",
313+
)
314+
assert _is_access_denied(err) is True
315+
316+
def test_untag_failure_is_non_fatal(self):
317+
# Tag succeeds but the cleanup untag fails: must not raise (the stray
318+
# snakemake-preflight tag is left behind and only logged at debug).
319+
ex = _executor(tags={"Env": "prod"})
320+
ex.batch_client.untag_resource.side_effect = Exception("no UntagResource")
321+
ex._preflight_check_tags() # must not raise
322+
ex.batch_client.tag_resource.assert_called_once()
323+
324+
def test_untag_not_called_when_tagging_denied(self):
325+
ex = _executor(tags={"Env": "prod"})
326+
ex.batch_client.tag_resource.side_effect = self._access_denied()
327+
with pytest.raises(WorkflowError, match="batch:TagResource"):
328+
ex._preflight_check_tags()
329+
ex.batch_client.untag_resource.assert_not_called()
330+
331+
242332
class TestValidateJobRole:
243333
def _iam(self, **get_role_kwargs) -> MagicMock:
244334
return MagicMock(get_role=MagicMock(**get_role_kwargs))

0 commit comments

Comments
 (0)