From 603f8667788f9731af6001766c32439467d84166 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 31 Oct 2026 13:05:15 +0100 Subject: [PATCH 1/2] fix(generator): derive ALB priorities, check them at plan, require name, validate compute routing.priority is now optional: without it the generator derives a stable priority from team/name in 1000-50000, in a slot that leaves room for the login rules. Explicit priorities (1-999) are kept, so live rules do not move. tf-plan checks planned listener rules against the live ones and fails with the holder's host and a free number instead of PriorityInUse at apply. name is required everywhere (detect, generator, CLI); the repo-name default in the generator is gone. compute.cpu/memory must be a Fargate task size, port 1-65535, desired_count >= 0. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/detect.yml | 4 + .github/workflows/expand-terraform.yml | 1 - .github/workflows/tf-plan.yml | 6 + CLAUDE.md | 1 + docs/app-yaml-reference.md | 34 ++++-- docs/reusable-workflows.md | 4 +- scripts/check-alb-priorities.py | 136 +++++++++++++++++++++ scripts/generate-modules.py | 97 +++++++++++++-- scripts/tests/test_check_alb_priorities.py | 105 ++++++++++++++++ scripts/tests/test_generate_modules.py | 88 ++++++++++++- 10 files changed, 449 insertions(+), 27 deletions(-) create mode 100755 scripts/check-alb-priorities.py create mode 100644 scripts/tests/test_check_alb_priorities.py diff --git a/.github/workflows/detect.yml b/.github/workflows/detect.yml index 787bccc..610b97e 100644 --- a/.github/workflows/detect.yml +++ b/.github/workflows/detect.yml @@ -59,6 +59,10 @@ jobs: echo "has_pnpm=$(test -f pnpm-lock.yaml && echo true || echo false)" >> "$GITHUB_OUTPUT" echo "has_eb=$(test -d .elasticbeanstalk && echo true || echo false)" >> "$GITHUB_OUTPUT" if [ -f app.yaml ]; then + if [ -z "$(yq '.name // ""' app.yaml)" ]; then + echo "::error file=app.yaml::name is required (e.g. name: my-app): lowercase letters, digits and hyphens, starting with a letter, max 20 chars" + exit 1 + fi echo "app_name=$(yq '.name // ""' app.yaml)" >> "$GITHUB_OUTPUT" echo "app_team=$(yq '.team // ""' app.yaml)" >> "$GITHUB_OUTPUT" else diff --git a/.github/workflows/expand-terraform.yml b/.github/workflows/expand-terraform.yml index cfcfcad..cba2aa2 100644 --- a/.github/workflows/expand-terraform.yml +++ b/.github/workflows/expand-terraform.yml @@ -48,7 +48,6 @@ jobs: - name: Generate modules from app.yaml env: - APP_SERVICE: ${{ github.event.repository.name }} AWS_ACCOUNT_ID: ${{ inputs.aws_account_id }} AWS_REGION: ${{ inputs.aws_region }} TF_ROOT: ${{ inputs.tf_root }} diff --git a/.github/workflows/tf-plan.yml b/.github/workflows/tf-plan.yml index 5828c10..ffa084e 100644 --- a/.github/workflows/tf-plan.yml +++ b/.github/workflows/tf-plan.yml @@ -121,6 +121,12 @@ jobs: id: plan run: .platform/scripts/run-plan.sh "${{ inputs.tf_root }}" + # ALB priorities are unique per listener and AWS only says so at + # apply (PriorityInUse): check the planned rules against the live ones. + - name: Check ALB rule priorities + if: steps.plan.outputs.has_changes == 'true' + run: python3 .platform/scripts/check-alb-priorities.py "${{ inputs.tf_root }}" + - name: Upload plan to S3 id: upload if: steps.plan.outputs.has_changes == 'true' diff --git a/CLAUDE.md b/CLAUDE.md index 6e8b747..b166c9f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -184,6 +184,7 @@ terraform/state/ |--------|------| | `scripts/bootstrap.sh` | One-time: create state bucket + lock tables | | `scripts/generate-modules.py` | CI: validates app.yaml, generates fmt-clean module blocks into terraform/ (tests: `scripts/tests/`) | +| `scripts/check-alb-priorities.py` | CI (tf-plan): fails the plan when a listener rule would take an ALB priority already in use (tests: `scripts/tests/`) | | `scripts/registry.py` | Module sources + boilerplate templates for generate-modules.py | | `scripts/resolve-tool-version.sh` | CI: resolve node/java version from the app repo's version files | | `scripts/sync-registered-teams.py` | CI: registry `teams/*.yaml` → `registered-teams.auto.tfvars` (needs App token) | diff --git a/docs/app-yaml-reference.md b/docs/app-yaml-reference.md index 0bd36f9..5a0713a 100644 --- a/docs/app-yaml-reference.md +++ b/docs/app-yaml-reference.md @@ -12,7 +12,6 @@ name: moresleep team: core routing: host: moresleep.javazone.no - priority: 120 ``` Without `routing:` the service runs with no load balancer (a worker). @@ -58,7 +57,7 @@ alarms: ### name (required) -Service name. Starts with a letter; lowercase letters, digits, and hyphens. Max 20 characters. Defaults to the repo name. +Service name. Starts with a letter; lowercase letters, digits, and hyphens. Max 20 characters. Required: there is no default from the repository name (repository names such as `2027.javazone.no` are not valid names), and the pipeline's detect step, the generator and the `javabin` CLI all fail on an `app.yaml` without one. ```yaml name: moresleep @@ -84,10 +83,10 @@ Container compute configuration. All fields have defaults. ```yaml compute: - cpu: 512 # default: 512 (0.5 vCPU) - memory: 1024 # default: 1024 (1 GB) - port: 8080 # default: 8000 - desired_count: 1 # default: 1 + cpu: 512 # default: 512 (0.5 vCPU); 256, 512, 1024, 2048, 4096, 8192 or 16384 + memory: 1024 # default: 1024 (1 GB); MiB, must fit cpu (table below) + port: 8080 # default: 8000; 1-65535 + desired_count: 1 # default: 1; 0 or more health_check: /health # default: /health — ALB target group health check path health_check_matcher: "200" # default: "200" user: "1000" # default: "1000" — container user (non-root) @@ -96,7 +95,17 @@ compute: Images that write outside `/tmp` (caches, uploads to local disk) need `readonly_root_filesystem: false`, or their tasks crash on start; the deployment circuit breaker then rolls the deploy back. -Valid CPU/memory combinations follow [Fargate task size constraints](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-cpu-memory-error.html). +`cpu`, `memory`, `port` and `desired_count` must be whole numbers, and `cpu`/`memory` a [Fargate task size](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-tasks-services.html#fargate-tasks-size); anything else fails the `expand` step instead of the apply: + +| cpu | memory (MiB) | +|-----|--------------| +| 256 | 512, 1024, 2048 | +| 512 | 1024-4096, steps of 1024 | +| 1024 | 2048-8192, steps of 1024 | +| 2048 | 4096-16384, steps of 1024 | +| 4096 | 8192-30720, steps of 1024 | +| 8192 | 16384-61440, steps of 4096 | +| 16384 | 32768-122880, steps of 8192 | ### routing @@ -105,14 +114,15 @@ ALB routing configuration. ```yaml routing: host: moresleep.javazone.no # required if routing is specified; also java.no / javabin.no - priority: 100 # required, unique 1-50000 + priority: 120 # optional; default: derived from team and name deregistration_delay: 30 # default: 30 (seconds) adopt_dns: false # default: false — take over an existing record for host ``` - `host` must be exactly one label under `javazone.no`, `java.no` or `javabin.no` (e.g. `moresleep.javazone.no`, `medlem.java.no`, `styret.javabin.no`). Those are the domains with a wildcard cert on the platform ALB; deeper names like `a.b.java.no` are rejected because the wildcard cert does not cover them. - `host` creates a Route53 A record (in the zone matching the host's domain) pointing to the platform ALB and an ALB listener rule matching the Host header. -- `priority` determines listener rule evaluation order. Must be unique across all apps. Coordinate via the registry. +- `priority` is the ALB listener rule priority, which must be unique on the shared listener. Leave it out: the generator derives a stable one from `team/name` in 1000-50000 (printed as a notice in the `expand` step), in a slot of 11 that keeps the 10 numbers below it free for `routing.login` rules. Set it only to keep an existing app's rule where it is (existing apps keep theirs, no rule is moved) or to move off a collision; explicit values must be 1-999, so they never land in the derived range. +- tf-plan checks every listener rule the plan creates or moves against the live rules on the listener (`scripts/check-alb-priorities.py`) and fails with the host that holds a taken priority and a free number to use, instead of apply failing with `PriorityInUse`. Two derived priorities can collide (rarely); the same check catches that, and setting `priority` to the suggested number fixes it. - `adopt_dns: true` takes over an A record for `host` that already exists (for example a hand-made record of an app moving onto the platform). It must be approved first: the host has to be listed for your team in `ADOPTIONS` in `scripts/registry.py` (a javaBin/platform PR), otherwise generation fails. Route53 records are not protected by team permissions, so this is what stops one team from taking over a hostname another app serves. Without `adopt_dns`, the apply fails because the record exists. With it, Route53 overwrites the record (UPSERT) to point at the platform ALB, and from then on Terraform manages it. The plan shows the record as a *create*. Changing `host` later replaces the record, and removing routing deletes it, as for any managed record. See [Adopting existing resources](#adopting-existing-resources). #### routing.login — sign-in at the load balancer @@ -162,7 +172,7 @@ What happens: Rules and priorities: -- Each rule becomes its own ALB listener rule at the priorities **directly below** `routing.priority`: the first rule gets `priority - 1`, the next `priority - 2`, and so on. Leave that room free: if another app already uses one of those priorities, apply fails with `PriorityInUse`. `priority` must be larger than the number of rules. Nothing checks this across apps yet, so pick a `priority` with at least 10 free numbers below it (e.g. a multiple of 20 when registering a new app); apps whose priority is right next to another app's (e.g. 100 and 101) cannot add rules without moving. +- Each rule becomes its own ALB listener rule at the priorities **directly below** `routing.priority`: the first rule gets `priority - 1`, the next `priority - 2`, and so on. A derived priority always has that room. With an explicit `priority`, it must be larger than the number of rules, and if another app already uses one of the numbers below it, tf-plan fails naming that app's host and suggesting a free number; apps whose priority is right next to another app's (e.g. 100 and 101) cannot add rules without moving (or dropping `priority` to take a derived one, which moves the rule once). - At most 10 rules. A path may appear only once, and a rule after a `/*` rule is an error (it could never match). - Protect `/*` and open the exceptions with `public: true`, rather than protecting a few subpaths. The load balancer matches the raw request path: case-sensitively, without decoding `%xx` and without resolving `..`. An app or framework that does any of these can be reached past a subpath rule, e.g. `/Admin`, `/%61dmin/` or `/x/../admin/` past `/admin/*`. The generator warns when no rule covers `/*`. @@ -447,12 +457,10 @@ Apps that need users from both pools should run two services for now. ### domain -Convenience alias for `routing.host`. If both are set, `routing.host` takes precedence. `routing.priority` is still required. +Convenience alias for `routing.host`. If both are set, `routing.host` takes precedence. `routing.priority` is optional, as with `routing.host`. ```yaml domain: cake-redux.javazone.no -routing: - priority: 110 ``` ### environment diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index 76c7ad0..c351353 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -92,6 +92,8 @@ Checks for: `app.yaml`, `terraform/`, `Dockerfile`, `pom.xml`, `build.gradle(.kt **Outputs:** `has_yaml`, `has_tf`, `has_docker`, `has_maven`, `has_gradle`, `has_pnpm`, `has_eb`, `app_name`, `app_team` +Fails when `app.yaml` exists without `name` (the name is never taken from the repository name). + ### build-jvm.yml Maven (`mvn verify`) or Gradle (`./gradlew build`) build + test. Uploads test reports. @@ -133,7 +135,7 @@ Docker BuildKit build + push to ECR repo `{team}-{app_name}`. Uses GitHub Action ### tf-plan.yml -Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. +Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. Before the upload, `scripts/check-alb-priorities.py` fails the plan if a listener rule it creates or moves would take an ALB priority another rule already holds (read-only `describe-rules`). **AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-plan-{team}` (read-only: refresh, state read, state lock, plan upload; tf-plan runs on any branch, so it cannot change resources); the review step uses the broker OIDC role to invoke `javabin-plan-review` **Outputs:** `has_changes`, `plan_key`, `plan_sha256`, `risk_level`, `planned_sha` diff --git a/scripts/check-alb-priorities.py b/scripts/check-alb-priorities.py new file mode 100755 index 0000000..4e263fc --- /dev/null +++ b/scripts/check-alb-priorities.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +"""Fail an app's plan when it would put an ALB listener rule on a taken priority. + +Listener rule priorities are unique per listener, and AWS only says so at +apply time (CreateRule: PriorityInUse). This reads the saved plan, takes every +aws_lb_listener_rule it creates or moves, and checks the priority against the +live rules on that listener (read-only elbv2:DescribeRules, which team roles +have). A rule the plan deletes or moves in the same run frees its old +priority. + +Usage (tf-plan, after run-plan.sh, with the team credentials): + python3 scripts/check-alb-priorities.py + +Exits 1 with a ::error annotation on a conflict. If the plan or the live rules +cannot be read it only warns: apply still catches a conflict, just later. +""" + +import json +import subprocess +import sys + +# Keep in step with generate-modules.py (explicit routing.priority range, and +# the number of login rules that sit directly below the host rule). +EXPLICIT_PRIORITY_MAX = 999 +LOGIN_MAX_RULES = 10 + + +def planned_rules(plan): + """(wanted, freed): rules the plan creates or re-prioritises, and ARNs it deletes. + + wanted: [{address, listener_arn, priority}] + """ + wanted, freed = [], set() + for rc in plan.get("resource_changes") or []: + if rc.get("type") != "aws_lb_listener_rule": + continue + change = rc.get("change") or {} + actions = change.get("actions") or [] + before, after = change.get("before") or {}, change.get("after") or {} + # delete-then-create (the default replace) frees the old priority + # first; create-before-destroy does not. + if actions and actions[0] == "delete" and before.get("arn"): + freed.add(before["arn"]) + if "create" not in actions and "update" not in actions: + continue + if "update" in actions and before.get("priority") == after.get("priority"): + continue + if "update" in actions and before.get("arn"): + freed.add(before["arn"]) # moves off its old priority in this run + if not isinstance(after.get("priority"), int) or not after.get("listener_arn"): + continue # unknown until apply + wanted.append({ + "address": rc.get("address", "aws_lb_listener_rule"), + "listener_arn": after["listener_arn"], + "priority": after["priority"], + }) + return wanted, freed + + +def live_priorities(rules): + """{priority: rule} for a describe-rules response (the default rule has none).""" + return {int(r["Priority"]): r for r in rules if str(r.get("Priority", "")).isdigit()} + + +def rule_hosts(rule): + hosts = [] + for cond in rule.get("Conditions") or []: + if cond.get("Field") == "host-header": + hosts += (cond.get("HostHeaderConfig") or {}).get("Values") or cond.get("Values") or [] + return ", ".join(hosts) or rule.get("RuleArn", "another rule") + + +def suggest_priority(taken, room=LOGIN_MAX_RULES): + """The lowest explicit priority with `room` free numbers below it, or None.""" + for p in range(room + 1, EXPLICIT_PRIORITY_MAX + 1): + if all(q not in taken for q in range(p - room, p + 1)): + return p + return None + + +def conflicts(wanted, freed, live): + """Error messages for wanted rules whose priority a live rule holds.""" + errors = [] + taken = {p for p, r in live.items() if r.get("RuleArn") not in freed} + for w in wanted: + holder = live.get(w["priority"]) + if not holder or holder.get("RuleArn") in freed: + continue + free = suggest_priority(taken) + hint = (f"set routing.priority in app.yaml to a free number, e.g. {free} (it leaves " + f"{LOGIN_MAX_RULES} free below it for routing.login rules)") if free else \ + "set routing.priority in app.yaml to a free number 1-999" + errors.append( + f"{w['address']}: ALB listener priority {w['priority']} is already used by the rule for " + f"{rule_hosts(holder)}, so apply would fail with PriorityInUse. Login rules take the " + f"priorities directly below routing.priority. To fix: {hint}, or remove routing.priority " + "to get one derived from team and name." + ) + return errors + + +def _run_json(cmd, cwd=None): + out = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, check=True).stdout + return json.loads(out) + + +def main(): + tf_root = sys.argv[1] if len(sys.argv) > 1 else "terraform" + try: + plan = _run_json(["terraform", "show", "-json", "tfplan"], cwd=tf_root) + except (subprocess.CalledProcessError, OSError, ValueError) as e: + print(f"::warning::check-alb-priorities: cannot read the plan ({e}); skipping") + return 0 + + wanted, freed = planned_rules(plan) + errors = [] + for listener in sorted({w["listener_arn"] for w in wanted}): + try: + rules = _run_json(["aws", "elbv2", "describe-rules", "--listener-arn", listener, "--output", "json"]) + except (subprocess.CalledProcessError, OSError, ValueError) as e: + detail = getattr(e, "stderr", "") or e + print(f"::warning::check-alb-priorities: cannot list rules on {listener} ({detail}); skipping") + continue + live = live_priorities(rules.get("Rules") or []) + errors += conflicts([w for w in wanted if w["listener_arn"] == listener], freed, live) + + for error in errors: + print(f"::error file=app.yaml::{error}") + print(f"ERROR: {error}", file=sys.stderr) + if not errors and wanted: + print(f"ALB priorities free: {', '.join(str(w['priority']) for w in wanted)}") + return 1 if errors else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/generate-modules.py b/scripts/generate-modules.py index d2acdf5..7627ad5 100755 --- a/scripts/generate-modules.py +++ b/scripts/generate-modules.py @@ -13,10 +13,11 @@ Usage: python3 generate-modules.py -Required env vars: APP_SERVICE, AWS_ACCOUNT_ID, AWS_REGION, TF_ROOT +Required env vars: AWS_ACCOUNT_ID, AWS_REGION, TF_ROOT Optional env vars: PLATFORM_REF (git ref for module source, default: main) """ +import hashlib import json import os import re @@ -113,6 +114,29 @@ class AppYamlError(ValueError): LOGIN_PATH_RE = re.compile(r"^/[A-Za-z0-9_\-.$/~\"'@:+*?&]{0,127}$") GROUP_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$") +# ALB listener rule priorities. Explicit routing.priority values are picked by +# hand from 1-999. Without one, the app gets a stable slot derived from +# team/name in 1000-50000: each slot holds the host rule at its top and room +# for LOGIN_MAX_RULES login rules directly below it. A hash can still collide; +# scripts/check-alb-priorities.py catches that (and any other taken priority) +# at plan time against the live listener rules. +EXPLICIT_PRIORITY_MAX = 999 +AUTO_PRIORITY_MIN, AUTO_PRIORITY_MAX = 1000, 50000 +AUTO_PRIORITY_STRIDE = LOGIN_MAX_RULES + 1 +AUTO_PRIORITY_SLOTS = (AUTO_PRIORITY_MAX - AUTO_PRIORITY_MIN + 1) // AUTO_PRIORITY_STRIDE + +# Fargate task sizes: cpu units -> allowed memory (MiB) +FARGATE_MEMORY = { + 256: (512, 1024, 2048), + 512: tuple(range(1024, 4097, 1024)), + 1024: tuple(range(2048, 8193, 1024)), + 2048: tuple(range(4096, 16385, 1024)), + 4096: tuple(range(8192, 30721, 1024)), + 8192: tuple(range(16384, 61441, 4096)), + 16384: tuple(range(32768, 122881, 8192)), +} +DEFAULT_CPU, DEFAULT_MEMORY, DEFAULT_PORT, DEFAULT_DESIRED_COUNT = 512, 1024, 8000, 1 + NAME_RE = re.compile(r"^[a-z][a-z0-9-]{0,19}$") TEAM_RE = re.compile(r"^[a-z]{1,12}$") RESOURCE_NAME_RE = re.compile(r"^[a-z0-9][a-z0-9_-]*$") @@ -258,6 +282,43 @@ def login_warnings(rules): return [] +def derived_priority(team, name): + """A stable ALB priority for team/name in the auto range, with room for + LOGIN_MAX_RULES login rules directly below it inside the same slot.""" + digest = hashlib.sha256(f"{team}/{name}".encode()).digest() + slot = int.from_bytes(digest[:8], "big") % AUTO_PRIORITY_SLOTS + return AUTO_PRIORITY_MIN + slot * AUTO_PRIORITY_STRIDE + LOGIN_MAX_RULES + + +def _is_int(value): + return isinstance(value, int) and not isinstance(value, bool) + + +def _validate_compute(compute): + for key in ("cpu", "memory", "port", "desired_count"): + if key in compute and not _is_int(compute[key]): + raise AppYamlError(f"compute.{key}: {compute[key]!r} must be a whole number") + cpu = compute.get("cpu", DEFAULT_CPU) + memory = compute.get("memory", DEFAULT_MEMORY) + if cpu not in FARGATE_MEMORY: + raise AppYamlError( + f"compute.cpu: {cpu} is not a Fargate size; use one of {', '.join(map(str, FARGATE_MEMORY))}" + ) + allowed = FARGATE_MEMORY[cpu] + if memory not in allowed: + step = allowed[1] - allowed[0] + raise AppYamlError( + f"compute.memory: {memory} MiB is not valid with cpu {cpu}; Fargate allows " + + (", ".join(map(str, allowed)) if len(allowed) <= 4 else f"{allowed[0]}-{allowed[-1]} in steps of {step}") + + " (docs: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/fargate-tasks-services.html#fargate-tasks-size)" + ) + port = compute.get("port", DEFAULT_PORT) + if not 1 <= port <= 65535: + raise AppYamlError(f"compute.port: {port} must be 1-65535") + if compute.get("desired_count", DEFAULT_DESIRED_COUNT) < 0: + raise AppYamlError(f"compute.desired_count: {compute['desired_count']} must be 0 or more") + + def _check_adoption(team, kind, identifier, where): if identifier not in ADOPTIONS.get(team, {}).get(kind, set()): raise AppYamlError( @@ -303,11 +364,22 @@ def validate(app): if allowed is not None and section in app and not (section == "auth" and isinstance(app[section], str)): _check_keys(section, app[section], allowed) - if not NAME_RE.match(str(app.get("name", ""))): - raise AppYamlError(f"name: {app.get('name')!r} must be lowercase letters, digits and hyphens, max 20 chars") + if not app.get("name"): + raise AppYamlError( + "name is required: it names the ECS service, ECR repo and IAM role (e.g. name: my-app); " + "it starts with a letter and has lowercase letters, digits and hyphens, max 20 chars" + ) + if not NAME_RE.match(str(app["name"])): + raise AppYamlError( + f"name: {app['name']!r} must start with a letter and have only lowercase letters, digits and " + "hyphens, max 20 chars (dots are not allowed: for repo 2027.javazone.no use e.g. javazone-2027)" + ) if not TEAM_RE.match(str(app.get("team", ""))): raise AppYamlError(f"team: {app.get('team')!r} must be lowercase letters only, max 12 chars") + if "compute" in app: + _validate_compute(app["compute"] or {}) + routing = app.get("routing") or {} if routing: if not routing.get("host") and not app.get("domain"): @@ -318,8 +390,13 @@ def validate(app): f"routing.host: {host!r} must be one label under " f"{', '.join(APP_DOMAINS)} (e.g. my-app.{APP_DOMAINS[0]})" ) - if not isinstance(routing.get("priority"), int) or not 1 <= routing["priority"] <= 50000: - raise AppYamlError("routing.priority is required: a unique integer 1-50000") + if "priority" in routing: + priority = routing["priority"] + if not _is_int(priority) or not 1 <= priority <= EXPLICIT_PRIORITY_MAX: + raise AppYamlError( + f"routing.priority: {priority!r} must be a whole number 1-{EXPLICIT_PRIORITY_MAX}, or left out " + f"to get a stable one derived from team and name ({AUTO_PRIORITY_MIN}-{AUTO_PRIORITY_MAX})" + ) normalize_login(routing) if "adopt_dns" in routing and not isinstance(routing["adopt_dns"], bool): raise AppYamlError("routing.adopt_dns must be true or false") @@ -689,9 +766,12 @@ def generate(app, account_id, region, platform_ref): # `domain:` is shorthand for routing.host; routing.host wins. if app.get("domain") and not routing.get("host"): - if not routing.get("priority"): - raise AppYamlError("domain: requires routing.priority (unique 1-50000) to create the ALB rule") routing["host"] = app["domain"] + # No routing.priority: a stable one derived from team/name (see + # derived_priority). Explicit priorities are kept as they are. + if routing.get("host") and "priority" not in routing: + routing["priority"] = derived_priority(team, name) + print(f"::notice file=app.yaml::routing.priority not set; using {routing['priority']} (derived from {team}/{name})") imports = [] # (resource address, import id) for existing resources to adopt @@ -1058,7 +1138,6 @@ def src(mod): def main(): - service = os.environ["APP_SERVICE"] account_id = os.environ["AWS_ACCOUNT_ID"] region = os.environ.get("AWS_REGION", "eu-central-1") tf_root = os.environ.get("TF_ROOT", "terraform") @@ -1070,8 +1149,6 @@ def main(): sys.exit(1) app = load_yaml(app_yaml_path) or {} - if isinstance(app, dict) and not app.get("name"): - app["name"] = service try: files = generate(app, account_id, region, platform_ref) diff --git a/scripts/tests/test_check_alb_priorities.py b/scripts/tests/test_check_alb_priorities.py new file mode 100644 index 0000000..49b41dd --- /dev/null +++ b/scripts/tests/test_check_alb_priorities.py @@ -0,0 +1,105 @@ +"""Tests for check-alb-priorities.py — run: python3 -m unittest discover scripts/tests""" + +import importlib.util +import os +import unittest + +HERE = os.path.dirname(os.path.abspath(__file__)) +spec = importlib.util.spec_from_file_location("check_alb_priorities", os.path.join(HERE, "..", "check-alb-priorities.py")) +check = importlib.util.module_from_spec(spec) +spec.loader.exec_module(check) + +LISTENER = "arn:aws:elasticloadbalancing:eu-central-1:1:listener/app/javabin-platform-alb/x/y" + + +def rule(priority, host, arn=None): + return { + "RuleArn": arn or f"arn:rule/{priority}", + "Priority": str(priority), + "Conditions": [{"Field": "host-header", "Values": [host], "HostHeaderConfig": {"Values": [host]}}], + } + + +LIVE = [ + {"RuleArn": "arn:rule/default", "Priority": "default", "Conditions": []}, + rule(10, "password.javazone.no"), + rule(100, "cupcake.javazone.no"), + rule(101, "cupcake-backend.javazone.no"), + rule(150, "watch.javazone.no"), +] + + +def change(address, actions, before=None, after=None): + return {"address": address, "type": "aws_lb_listener_rule", + "change": {"actions": actions, "before": before, "after": after}} + + +def plan(*changes): + return {"resource_changes": [ + {"address": "module.ecr.aws_ecr_repository.this", "type": "aws_ecr_repository", + "change": {"actions": ["create"], "before": None, "after": {}}}, + *changes, + ]} + + +def run(p, live=LIVE): + wanted, freed = check.planned_rules(p) + return check.conflicts(wanted, freed, check.live_priorities(live)) + + +class CheckAlbPrioritiesTest(unittest.TestCase): + def test_new_rule_on_a_taken_priority_fails_with_the_holder_and_a_free_number(self): + errors = run(plan(change("module.routing.aws_lb_listener_rule.this", ["create"], + after={"priority": 100, "listener_arn": LISTENER}))) + self.assertEqual(len(errors), 1) + self.assertIn("priority 100 is already used by the rule for cupcake.javazone.no", errors[0]) + self.assertIn("PriorityInUse", errors[0]) + self.assertIn("e.g. 21", errors[0]) # 11-21 is the first free run of 11 (10 is taken) + + def test_free_priority_passes(self): + self.assertEqual(run(plan(change("module.routing.aws_lb_listener_rule.this", ["create"], + after={"priority": 2297, "listener_arn": LISTENER}))), []) + + def test_login_rule_below_the_host_rule_is_checked_too(self): + errors = run(plan( + change("module.routing.aws_lb_listener_rule.this", ["create"], + after={"priority": 151, "listener_arn": LISTENER}), + change('module.routing.aws_lb_listener_rule.login["p1"]', ["create"], + after={"priority": 150, "listener_arn": LISTENER}), + )) + self.assertEqual(len(errors), 1) + self.assertIn('login["p1"]', errors[0]) + self.assertIn("watch.javazone.no", errors[0]) + + def test_unchanged_and_in_place_updates_of_the_own_rule_pass(self): + own = {"arn": "arn:rule/101", "priority": 101, "listener_arn": LISTENER} + self.assertEqual(run(plan(change("r", ["no-op"], before=own, after=own))), []) + self.assertEqual(run(plan(change("r", ["update"], before=own, after={**own, "tags": {}}))), []) + + def test_moving_onto_a_priority_the_plan_frees_passes(self): + # one app moves its host rule from 150 onto 151 and a login rule onto 150 + host = {"arn": "arn:rule/150", "priority": 150, "listener_arn": LISTENER} + self.assertEqual(run(plan( + change("module.routing.aws_lb_listener_rule.this", ["update"], before=host, after={**host, "priority": 151}), + change('module.routing.aws_lb_listener_rule.login["p1"]', ["create"], + after={"priority": 150, "listener_arn": LISTENER}), + )), []) + + def test_delete_then_create_frees_but_create_before_destroy_does_not(self): + old = {"arn": "arn:rule/100", "priority": 100, "listener_arn": LISTENER} + new = {"priority": 100, "listener_arn": LISTENER} + self.assertEqual(run(plan(change("r", ["delete", "create"], before=old, after=new))), []) + self.assertEqual(len(run(plan(change("r", ["create", "delete"], before=old, after=new)))), 1) + + def test_unknown_values_are_skipped(self): + self.assertEqual(run(plan(change("r", ["create"], after={"priority": None, "listener_arn": LISTENER}))), []) + self.assertEqual(run(plan(change("r", ["create"], after={"priority": 100, "listener_arn": None}))), []) + + def test_suggestion_leaves_room_for_login_rules(self): + taken = {11, 30} + self.assertEqual(check.suggest_priority(taken), 22) + self.assertIsNone(check.suggest_priority(set(range(1, 1000, 5)))) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/tests/test_generate_modules.py b/scripts/tests/test_generate_modules.py index 02cae0c..0c79106 100644 --- a/scripts/tests/test_generate_modules.py +++ b/scripts/tests/test_generate_modules.py @@ -100,8 +100,18 @@ def test_unimplemented_features_explain_alternative(self): def test_team_must_be_lowercase_letters(self): self.assertRejected({**CUPCAKE, "team": "cup-cake"}, "team:") - def test_routing_needs_priority(self): - self.assertRejected({**CUPCAKE, "routing": {"host": "x.javazone.no"}}, "routing.priority") + def test_name_is_required(self): + app = {k: v for k, v in CUPCAKE.items() if k != "name"} + self.assertRejected(app, "name is required") + self.assertRejected({**CUPCAKE, "name": ""}, "name is required") + + def test_name_with_dots_explains_the_fix(self): + self.assertRejected({**CUPCAKE, "name": "2027.javazone.no"}, "dots are not allowed") + + def test_explicit_priority_must_be_in_the_hand_picked_range(self): + for bad in (0, 1000, 50000, "100", True): + self.assertRejected({**CUPCAKE, "routing": {"host": "x.javazone.no", "priority": bad}}, + "routing.priority") def test_host_must_be_under_an_app_domain(self): self.assertRejected({**CUPCAKE, "routing": {"host": "cupcake.example.com", "priority": 101}}, @@ -119,6 +129,80 @@ def test_auth_pool_both_is_rejected(self): self.assertRejected({**CUPCAKE, "auth": {"pool": "both"}}, "'both' is not supported") +class ComputeValidationTest(unittest.TestCase): + def assertRejected(self, compute, fragment): + with self.assertRaises(gen.AppYamlError) as cm: + generate({**CUPCAKE, "compute": compute}) + self.assertIn(fragment, str(cm.exception)) + + def test_valid_fargate_sizes_pass(self): + for compute in ({}, {"cpu": 256, "memory": 512}, {"cpu": 256, "memory": 2048}, + {"cpu": 1024, "memory": 8192}, {"cpu": 4096, "memory": 30720}, + {"cpu": 8192, "memory": 61440}, {"cpu": 512}, {"memory": 4096}): + generate({**CUPCAKE, "compute": compute}) + + def test_cpu_must_be_a_fargate_size(self): + self.assertRejected({"cpu": 300}, "compute.cpu: 300 is not a Fargate size") + + def test_memory_must_fit_the_cpu(self): + self.assertRejected({"cpu": 256, "memory": 4096}, "allows 512, 1024, 2048") + self.assertRejected({"cpu": 1024, "memory": 1024}, "2048-8192 in steps of 1024") + self.assertRejected({"memory": 512}, "not valid with cpu 512") # default cpu + self.assertRejected({"cpu": 8192, "memory": 17408}, "in steps of 4096") + + def test_numbers_must_be_whole_numbers(self): + self.assertRejected({"cpu": "512"}, "compute.cpu: '512' must be a whole number") + self.assertRejected({"desired_count": True}, "compute.desired_count") + self.assertRejected({"port": 80.5}, "compute.port") + + def test_port_and_desired_count_ranges(self): + self.assertRejected({"port": 0}, "compute.port: 0 must be 1-65535") + self.assertRejected({"port": 70000}, "compute.port") + self.assertRejected({"desired_count": -1}, "compute.desired_count: -1") + generate({**CUPCAKE, "compute": {"desired_count": 0}}) + + +class PriorityTest(unittest.TestCase): + def generate_quiet(self, app): + with contextlib.redirect_stdout(io.StringIO()) as out: + files = generate(app) + return files, out.getvalue() + + def test_explicit_priority_is_kept(self): + main = generate(CUPCAKE)["main.tf"] + self.assertIn("listener_rule_priority = 101", main) + + def test_missing_priority_is_derived_from_team_and_name(self): + app = {**CUPCAKE, "routing": {"host": "cupcake-backend.javazone.no"}} + files, out = self.generate_quiet(app) + expected = gen.derived_priority("cupcake", "cupcake-backend") + self.assertIn(f"listener_rule_priority = {expected}\n", files["main.tf"]) + self.assertIn(f"using {expected}", out) + self.assertEqual(expected, gen.derived_priority("cupcake", "cupcake-backend")) # stable + self.assertNotEqual(expected, gen.derived_priority("cupcake", "cupcake")) + + def test_derived_priorities_stay_in_range_with_room_for_login_rules(self): + for i in range(2000): + p = gen.derived_priority("team", f"app-{i}") + self.assertLessEqual(p, gen.AUTO_PRIORITY_MAX) + self.assertGreaterEqual(p - gen.LOGIN_MAX_RULES, gen.AUTO_PRIORITY_MIN) + # the slot's top: login rules below it stay inside the slot + self.assertEqual((p - gen.AUTO_PRIORITY_MIN) % gen.AUTO_PRIORITY_STRIDE, gen.LOGIN_MAX_RULES) + + def test_derived_priority_fits_the_maximum_login_rules(self): + login = [{"paths": [f"/p{i}/*"], "public": True} for i in range(gen.LOGIN_MAX_RULES)] + app = {**CUPCAKE, "routing": {"host": "cupcake-backend.javazone.no", "login": login}} + files, _ = self.generate_quiet(app) + top = gen.derived_priority("cupcake", "cupcake-backend") + self.assertIn(f"priority = {top - gen.LOGIN_MAX_RULES}\n", files["main.tf"]) + + def test_domain_shorthand_needs_no_priority(self): + app = {**{k: v for k, v in CUPCAKE.items() if k != "routing"}, "domain": "cupcake-backend.javazone.no"} + files, _ = self.generate_quiet(app) + self.assertIn(f"listener_rule_priority = {gen.derived_priority('cupcake', 'cupcake-backend')}", + files["main.tf"]) + + class GenerationTest(unittest.TestCase): def test_both_auth_forms_produce_the_same_wiring(self): for app in (CUPCAKE, FROSTING): From 37159939e1c707b01990e8e9ea0f664707d9a47c Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 31 Oct 2026 13:12:16 +0100 Subject: [PATCH 2/2] fix(ci): post the plan comment on an ALB priority conflict, drop repo-name app fallback tf-plan now records the conflict, posts it at the top of the plan comment and fails the job in a final step. auth-doctor no longer falls back to the repository name for the app name. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/tf-apply.yml | 2 -- .github/workflows/tf-plan.yml | 9 +++++ docs/app-yaml-reference.md | 2 +- docs/reusable-workflows.md | 2 +- scripts/auth-doctor.py | 8 +++-- scripts/check-alb-priorities.py | 22 ++++++++++-- scripts/post-plan-comment.sh | 11 ++++++ scripts/tests/test_auth_doctor.py | 16 +++++++++ scripts/tests/test_check_alb_priorities.py | 41 ++++++++++++++++++++++ scripts/tests/test_post_plan_comment.py | 14 +++++++- 10 files changed, 117 insertions(+), 10 deletions(-) diff --git a/.github/workflows/tf-apply.yml b/.github/workflows/tf-apply.yml index b69a8ea..c310d9e 100644 --- a/.github/workflows/tf-apply.yml +++ b/.github/workflows/tf-apply.yml @@ -100,6 +100,4 @@ jobs: - name: Auth doctor if: hashFiles('app.yaml') != '' continue-on-error: true - env: - APP_SERVICE: ${{ github.event.repository.name }} run: python3 .platform/scripts/auth-doctor.py --app-yaml app.yaml --wait 180 diff --git a/.github/workflows/tf-plan.yml b/.github/workflows/tf-plan.yml index ffa084e..f62423c 100644 --- a/.github/workflows/tf-plan.yml +++ b/.github/workflows/tf-plan.yml @@ -123,8 +123,11 @@ jobs: # ALB priorities are unique per listener and AWS only says so at # apply (PriorityInUse): check the planned rules against the live ones. + # A conflict goes into the PR comment; the last step fails the job. - name: Check ALB rule priorities + id: alb_priorities if: steps.plan.outputs.has_changes == 'true' + continue-on-error: true run: python3 .platform/scripts/check-alb-priorities.py "${{ inputs.tf_root }}" - name: Upload plan to S3 @@ -159,3 +162,9 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} run: sh .platform/scripts/post-plan-comment.sh "${{ inputs.tf_root }}/plan-output.txt" "${{ steps.plan.outputs.has_changes }}" + + - name: Fail on ALB rule priority conflict + if: steps.alb_priorities.outcome == 'failure' + run: | + echo "::error file=app.yaml::ALB rule priority conflict (see the Check ALB rule priorities step); apply would fail with PriorityInUse" + exit 1 diff --git a/docs/app-yaml-reference.md b/docs/app-yaml-reference.md index 5a0713a..57690de 100644 --- a/docs/app-yaml-reference.md +++ b/docs/app-yaml-reference.md @@ -122,7 +122,7 @@ routing: - `host` must be exactly one label under `javazone.no`, `java.no` or `javabin.no` (e.g. `moresleep.javazone.no`, `medlem.java.no`, `styret.javabin.no`). Those are the domains with a wildcard cert on the platform ALB; deeper names like `a.b.java.no` are rejected because the wildcard cert does not cover them. - `host` creates a Route53 A record (in the zone matching the host's domain) pointing to the platform ALB and an ALB listener rule matching the Host header. - `priority` is the ALB listener rule priority, which must be unique on the shared listener. Leave it out: the generator derives a stable one from `team/name` in 1000-50000 (printed as a notice in the `expand` step), in a slot of 11 that keeps the 10 numbers below it free for `routing.login` rules. Set it only to keep an existing app's rule where it is (existing apps keep theirs, no rule is moved) or to move off a collision; explicit values must be 1-999, so they never land in the derived range. -- tf-plan checks every listener rule the plan creates or moves against the live rules on the listener (`scripts/check-alb-priorities.py`) and fails with the host that holds a taken priority and a free number to use, instead of apply failing with `PriorityInUse`. Two derived priorities can collide (rarely); the same check catches that, and setting `priority` to the suggested number fixes it. +- tf-plan checks every listener rule the plan creates or moves against the live rules on the listener (`scripts/check-alb-priorities.py`) and fails with the host that holds a taken priority and a free number to use, instead of apply failing with `PriorityInUse`. The conflict is shown at the top of the plan comment on the PR. Two derived priorities can collide (rarely); the same check catches that, and setting `priority` to the suggested number fixes it. - `adopt_dns: true` takes over an A record for `host` that already exists (for example a hand-made record of an app moving onto the platform). It must be approved first: the host has to be listed for your team in `ADOPTIONS` in `scripts/registry.py` (a javaBin/platform PR), otherwise generation fails. Route53 records are not protected by team permissions, so this is what stops one team from taking over a hostname another app serves. Without `adopt_dns`, the apply fails because the record exists. With it, Route53 overwrites the record (UPSERT) to point at the platform ALB, and from then on Terraform manages it. The plan shows the record as a *create*. Changing `host` later replaces the record, and removing routing deletes it, as for any managed record. See [Adopting existing resources](#adopting-existing-resources). #### routing.login — sign-in at the load balancer diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index c351353..d2a22a1 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -135,7 +135,7 @@ Docker BuildKit build + push to ECR repo `{team}-{app_name}`. Uses GitHub Action ### tf-plan.yml -Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. Before the upload, `scripts/check-alb-priorities.py` fails the plan if a listener rule it creates or moves would take an ALB priority another rule already holds (read-only `describe-rules`). +Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. `scripts/check-alb-priorities.py` checks whether a listener rule the plan creates or moves would take an ALB priority another rule already holds (read-only `describe-rules`); on a conflict the plan comment still gets posted, with the conflict at the top, and the last step fails the job. **AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-plan-{team}` (read-only: refresh, state read, state lock, plan upload; tf-plan runs on any branch, so it cannot change resources); the review step uses the broker OIDC role to invoke `javabin-plan-review` **Outputs:** `has_changes`, `plan_key`, `plan_sha256`, `risk_level`, `planned_sha` diff --git a/scripts/auth-doctor.py b/scripts/auth-doctor.py index 475494a..b23a27c 100755 --- a/scripts/auth-doctor.py +++ b/scripts/auth-doctor.py @@ -363,7 +363,7 @@ def main(argv=None): p = argparse.ArgumentParser(description=__doc__.split("\n")[0]) p.add_argument("--app-yaml", default="app.yaml") p.add_argument("--team", help="default: app.yaml team") - p.add_argument("--app", help="default: app.yaml name, then $APP_SERVICE, then the repository name") + p.add_argument("--app", help="default: app.yaml name (required there)") p.add_argument("--wait", type=int, default=0, help="seconds to wait for app-auth-sync to finish (CI: 180)") p.add_argument("--no-tasks", action="store_true", help="skip the ECS task check") p.add_argument("--json", action="store_true", help="print results as JSON") @@ -374,8 +374,10 @@ def main(argv=None): return 0 gen = _load_generator() app = gen.load_yaml(args.app_yaml) or {} - name = args.app or app.get("name") or os.environ.get("APP_SERVICE") or \ - os.environ.get("GITHUB_REPOSITORY", "").rsplit("/", 1)[-1] + name = args.app or app.get("name") + if not name: + print(f"{args.app_yaml}: name is required (or pass --app)", file=sys.stderr) + return 1 try: report = diagnose(app, args.team, name, args.wait, tasks=not args.no_tasks) except gen.AppYamlError as e: diff --git a/scripts/check-alb-priorities.py b/scripts/check-alb-priorities.py index 4e263fc..6f1fc8f 100755 --- a/scripts/check-alb-priorities.py +++ b/scripts/check-alb-priorities.py @@ -11,11 +11,15 @@ Usage (tf-plan, after run-plan.sh, with the team credentials): python3 scripts/check-alb-priorities.py -Exits 1 with a ::error annotation on a conflict. If the plan or the live rules -cannot be read it only warns: apply still catches a conflict, just later. +Exits 1 with a ::error annotation on a conflict, and writes the conflicts to +alb-priority-errors.txt in the current directory, which post-plan-comment.sh +puts at the top of the PR comment (tf-plan posts the comment before it fails +the job). If the plan or the live rules cannot be read it only warns: apply +still catches a conflict, just later. """ import json +import os import subprocess import sys @@ -24,6 +28,9 @@ EXPLICIT_PRIORITY_MAX = 999 LOGIN_MAX_RULES = 10 +# Read by post-plan-comment.sh (both run from the app repo root). +ERRORS_FILE = "alb-priority-errors.txt" + def planned_rules(plan): """(wanted, freed): rules the plan creates or re-prioritises, and ARNs it deletes. @@ -104,8 +111,18 @@ def _run_json(cmd, cwd=None): return json.loads(out) +def write_errors(errors, path=ERRORS_FILE): + """Leave the conflicts for the PR comment; remove a stale file when there are none.""" + if errors: + with open(path, "w") as f: + f.writelines(f"- {e}\n" for e in errors) + elif os.path.exists(path): + os.remove(path) + + def main(): tf_root = sys.argv[1] if len(sys.argv) > 1 else "terraform" + write_errors([]) try: plan = _run_json(["terraform", "show", "-json", "tfplan"], cwd=tf_root) except (subprocess.CalledProcessError, OSError, ValueError) as e: @@ -127,6 +144,7 @@ def main(): for error in errors: print(f"::error file=app.yaml::{error}") print(f"ERROR: {error}", file=sys.stderr) + write_errors(errors) if not errors and wanted: print(f"ALB priorities free: {', '.join(str(w['priority']) for w in wanted)}") return 1 if errors else 0 diff --git a/scripts/post-plan-comment.sh b/scripts/post-plan-comment.sh index 363a14d..3f576dc 100644 --- a/scripts/post-plan-comment.sh +++ b/scripts/post-plan-comment.sh @@ -5,6 +5,8 @@ # # If review-output.txt exists in the current directory, it is appended # to the comment. This avoids posting two separate comments. +# If alb-priority-errors.txt exists (written by check-alb-priorities.py on a +# conflict), it is shown above the plan. # # Env: GH_TOKEN (or gh auth), GITHUB_REPOSITORY, PR_NUMBER @@ -23,6 +25,14 @@ else STATUS=":white_check_mark: **No changes** — infrastructure is up to date." fi +ALB_ERRORS="" +if [ -s alb-priority-errors.txt ]; then + ALB_ERRORS=":x: **ALB rule priority conflict**: apply would fail with \`PriorityInUse\`, so this check fails. + +$(cat alb-priority-errors.txt) +" +fi + # --- Filter plan: strip refresh/read noise, keep only the meaningful diff --- # GitHub caps a comment at 65,536 characters, so the plan shown here is cut at # PLAN_COMMENT_CHARS. Say so when it is: readers (and anyone pasting the @@ -49,6 +59,7 @@ cat > /tmp/plan-comment.md <Plan output \`\`\` diff --git a/scripts/tests/test_auth_doctor.py b/scripts/tests/test_auth_doctor.py index ee57d09..2b28f38 100644 --- a/scripts/tests/test_auth_doctor.py +++ b/scripts/tests/test_auth_doctor.py @@ -1,9 +1,13 @@ """Tests for auth-doctor.py — run: python3 -m unittest discover scripts/tests""" +import contextlib import importlib.util +import io import json import os +import tempfile import unittest +from unittest import mock HERE = os.path.dirname(os.path.abspath(__file__)) spec = importlib.util.spec_from_file_location("auth_doctor", os.path.join(HERE, "..", "auth-doctor.py")) @@ -189,5 +193,17 @@ def test_no_recorded_change_skips(self): self.assertEqual([r["level"] for r in self.check({})], ["SKIP"]) +class NameTest(unittest.TestCase): + def test_name_is_not_taken_from_the_repository(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "app.yaml") + with open(path, "w") as f: + f.write("team: cupcake\nauth: internal\n") + env = {"APP_SERVICE": "cupcake", "GITHUB_REPOSITORY": "javaBin/cupcake"} + with mock.patch.dict(os.environ, env), contextlib.redirect_stderr(io.StringIO()) as err: + self.assertEqual(doc.main(["--app-yaml", path]), 1) + self.assertIn("name is required", err.getvalue()) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/tests/test_check_alb_priorities.py b/scripts/tests/test_check_alb_priorities.py index 49b41dd..fb17113 100644 --- a/scripts/tests/test_check_alb_priorities.py +++ b/scripts/tests/test_check_alb_priorities.py @@ -1,7 +1,11 @@ """Tests for check-alb-priorities.py — run: python3 -m unittest discover scripts/tests""" +import contextlib import importlib.util +import io import os +import sys +import tempfile import unittest HERE = os.path.dirname(os.path.abspath(__file__)) @@ -101,5 +105,42 @@ def test_suggestion_leaves_room_for_login_rules(self): self.assertIsNone(check.suggest_priority(set(range(1, 1000, 5)))) +class ErrorsFileTest(unittest.TestCase): + """The conflicts are left in a file for the PR comment (post-plan-comment.sh).""" + + def setUp(self): + self.cwd = os.getcwd() + self.tmp = tempfile.TemporaryDirectory() + os.chdir(self.tmp.name) + self.run_json = check._run_json + self.argv = sys.argv + + def tearDown(self): + check._run_json, sys.argv = self.run_json, self.argv + os.chdir(self.cwd) + self.tmp.cleanup() + + def main(self, priority): + p = plan(change("module.routing.aws_lb_listener_rule.this", ["create"], + after={"priority": priority, "listener_arn": LISTENER})) + check._run_json = lambda cmd, cwd=None: p if cmd[0] == "terraform" else {"Rules": LIVE} + sys.argv = ["check-alb-priorities.py", "tf"] + with contextlib.redirect_stdout(io.StringIO()), contextlib.redirect_stderr(io.StringIO()): + return check.main() + + def test_conflict_is_written_for_the_comment(self): + self.assertEqual(self.main(100), 1) + with open(check.ERRORS_FILE) as f: + text = f.read() + self.assertTrue(text.startswith("- module.routing.aws_lb_listener_rule.this: "), text) + self.assertIn("priority 100 is already used by the rule for cupcake.javazone.no", text) + + def test_no_conflict_removes_a_stale_file(self): + with open(check.ERRORS_FILE, "w") as f: + f.write("- old\n") + self.assertEqual(self.main(500), 0) + self.assertFalse(os.path.exists(check.ERRORS_FILE)) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/tests/test_post_plan_comment.py b/scripts/tests/test_post_plan_comment.py index 230b5dd..8212b2d 100644 --- a/scripts/tests/test_post_plan_comment.py +++ b/scripts/tests/test_post_plan_comment.py @@ -9,7 +9,7 @@ SCRIPT = os.path.join(HERE, "..", "post-plan-comment.sh") -def post(plan_text, limit): +def post(plan_text, limit, alb_errors=None): """Run the script with a fake `gh` that copies the comment body; return it.""" with tempfile.TemporaryDirectory() as tmp: bin_dir = os.path.join(tmp, "bin") @@ -23,6 +23,9 @@ def post(plan_text, limit): for name in ("plan-output.txt", "plan-clean.txt"): with open(os.path.join(plan_dir, name), "w") as f: f.write(plan_text) + if alb_errors: + with open(os.path.join(tmp, "alb-priority-errors.txt"), "w") as f: + f.write(alb_errors) env = dict(os.environ, PATH=f"{bin_dir}:{os.environ['PATH']}", PR_NUMBER="1", GITHUB_REPOSITORY="javaBin/platform", PLAN_COMMENT_CHARS=str(limit)) subprocess.run(["sh", SCRIPT, os.path.join(plan_dir, "plan-output.txt"), "true"], @@ -44,6 +47,15 @@ def test_plan_that_fits_has_no_cut_note(self): self.assertIn("aws_x.r99", body) self.assertNotIn("cut at", body) + def test_alb_priority_conflict_is_shown_above_the_plan(self): + body = post(self.PLAN, 60000, alb_errors="- priority 100 is already used by the rule for cupcake.javazone.no\n") + self.assertIn("ALB rule priority conflict", body) + self.assertIn("- priority 100 is already used by the rule for cupcake.javazone.no", body) + self.assertLess(body.index("ALB rule priority conflict"), body.index("Plan output")) + + def test_no_conflict_section_without_errors_file(self): + self.assertNotIn("ALB rule priority conflict", post(self.PLAN, 60000)) + if __name__ == "__main__": unittest.main()