Gate AWS Lambda role/external-id requirement behind --aws-lambda-skip-role-and-external-id - #1140
Conversation
|
mani seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
137d686 to
dc3da76
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes client-side enforcement of AWS Lambda assume-role ARN and external ID when configuring Worker Deployment compute settings, relying instead on Temporal server-side policy (require_role_and_external_id) so that role-less configurations (e.g., LocalStack/dev setups) are not blocked by the CLI.
Changes:
- Relaxed CLI validation so AWS Lambda compute provider details only require the function ARN.
- Updated CLI flag documentation/help text to no longer claim role/external ID are always required when a function ARN is set.
- Updated create-version error tests to assert server-side rejection when role/external ID are omitted under default server settings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/temporalcli/commands.yaml | Updated flag descriptions to remove unconditional “required” wording for AWS Lambda role/external ID. |
| internal/temporalcli/commands.worker.deployment.go | Relaxed AWS Lambda provider-details validation to only require ARN. |
| internal/temporalcli/commands.worker.deployment_test.go | Updated error expectations to match server-side validation messages. |
| internal/temporalcli/commands.gen.go | Regenerated/updated Cobra flag help strings consistent with updated YAML descriptions. |
Comments suppressed due to low confidence (3)
internal/temporalcli/commands.yaml:1487
- The update-version compute config command has the same documentation issue: clarify that this flag only applies when
--aws-lambda-function-arnis being used, and that server-side settings may still require it.
AWS IAM role ARN that the Temporal server will assume when invoking
the Lambda function that spawns a new Worker in this Worker
Deployment Version.
internal/temporalcli/commands.yaml:1493
- Likewise, clarify that the external ID is only applicable alongside the AWS Lambda function ARN (and role ARN), and may still be required depending on server configuration.
Temporal server will enforce that the AWS IAM trust policy associated
with the AWS IAM role specified in --aws-lambda-assume-role-arn has
an aws:ExternalId condition that matches the supplied value.
internal/temporalcli/commands.gen.go:4039
- Same documentation concern for
update-version-compute-config: the help text should clarify these flags are only applicable with--aws-lambda-function-arn, and that server-side configuration may still require them.
s.Command.Flags().StringVar(&s.AwsLambdaAssumeRoleArn, "aws-lambda-assume-role-arn", "", "AWS IAM role ARN that the Temporal server will assume when invoking the Lambda function that spawns a new Worker in this Worker Deployment Version.")
s.Command.Flags().StringVar(&s.AwsLambdaAssumeRoleExternalId, "aws-lambda-assume-role-external-id", "", "Temporal server will enforce that the AWS IAM trust policy associated with the AWS IAM role specified in --aws-lambda-assume-role-arn has an aws:ExternalId condition that matches the supplied value.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
02strich
left a comment
There was a problem hiding this comment.
I am sorry for the long delay on review. Instead of making the existing flags optional, I would prefer to have a new flag that indicates that the user wants to disable the security setting - and then either that or the external ID flag need to be set. That way there is no oopsie moment risk.
087599d to
7e56bd9
Compare
dbac0fc to
e0eeaf1
Compare
…ment create-version and update-version-compute-config require --aws-lambda-assume-role-arn and --aws-lambda-assume-role-external-id whenever --aws-lambda-function-arn is specified. The Temporal server governs whether these are actually mandatory via its global require_role_and_external_id setting (default true), so a role-less config is valid against servers where that setting is disabled (e.g. local dev against LocalStack). --aws-lambda-skip-role-and-external-id (bool, default false) opts out of the client-side requirement, so the CLI defers that policy to the server. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e0eeaf1 to
5f42868
Compare
| description: | | ||
| Permit omitting --aws-lambda-assume-role-arn and | ||
| --aws-lambda-assume-role-external-id when --aws-lambda-function-arn | ||
| is specified. Both are required unless this flag is passed. |
There was a problem hiding this comment.
Continue from a previous comment: I think this should clarify whether the two flags (or one of them) can still be passed if this flag is passed? i.e., which of these:
Both are required unless this flag is passed, in which case both must be omitted. (may need to reword)
vs.
Both are required if this flag is not provided, otherwise they are treated as optional.
There was a problem hiding this comment.
done, reworded this to specfically call out that the optional bit, let me know if you had any suggestions on the latest revision.
- Reject --aws-lambda-assume-role-arn / --aws-lambda-assume-role-external-id when --aws-lambda-skip-role-and-external-id is passed (validated in validateAWSLambdaProviderDetails alongside the required-detail checks). - Reword the flag descriptions to state that the role and external ID are required unless the skip flag is passed, in which case both must be omitted. - Drop a verbose inline comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cd64725 to
2da04af
Compare
# Backport for CLI v1.8.3 (monthly public/latest) Cuts the scheduled monthly public/latest release onto `release/1.8.x`, which was sitting exactly on `v1.8.2` with nothing backported since 2026-07-31. **The governing constraint:** this release keeps the embedded dev server on OSS Server **v1.31.2**. Everything below follows from that. `main` has moved to server `v1.32.0-162.0` (a Cloud tag) and `go.temporal.io/api` v1.63.x, so `main` is not publicly releasable and a large share of recent work cannot ship here. ## Summary Of the 29 commits on `main` since `v1.8.2`: | | Count | |---|---| | Cherry-picked as-is | 13 | | Dependency bumps folded into one commit | 5 | | New commits authored for this backport | 2 | | Excluded | 11 | Verified: `go build ./...` and `cliext` build clean, `make gen` produces no diff, full `go test ./...` green, binary reports `Server 1.31.2, UI 2.50.1`. ## Included ### Cherry-picked from `main` | Commit | Change | |---|---| | #1153 | test: fix concurrent start test assertions | | #1140 | Gate AWS Lambda role/external-id behind `--aws-lambda-skip-role-and-external-id` | | #1137 | Delegate help and completion to extensions when applicable | | #1167 | Add `--gcp-cloud-run-scale-down-stabilization-duration` | | #1176 | Fix cliext build, add it to CI workflow | | #1149 | chore(deps): bump the github-actions group with 3 updates | | #1162 | chore(deps): bump docker/login-action 4.4.0 → 4.5.2 | | #1166 | chore(deps): bump docker/login-action 4.5.2 → 4.6.0 | | #1156 | fix(activity): remove no-op `reset-attempts` flag — **adjusted, see below** | | #1061 | feat: add `temporal options` command and declutter help output | | #1171 | test: stabilize activity list pagination | | #1186 | fix: document `start-dev` `--log-level` default | | #1177 | Support AWS AgentCore compute provider | The last three merged to `main` on 2026-09-01, after the initial backport set was assembled, and all three cherry-pick cleanly with no dependency movement. **#1177 (AgentCore)** is a new feature rather than a fix, so it warrants a deliberate look. It carries no api v1.63.x dependency: the provider type is the plain string `"aws-agentcore"` and the provider details are an opaque `map[string]any` encoded to a `commonpb.Payload`. Server v1.31.2 does not validate the provider type — it forwards it as `wciiface.ComputeProviderType` — so acceptance is decided by Cloud-side WCI, not by anything this release pins. Its functional test is `t.Skip`-ed pending AWS fixtures, which matches the existing Lambda and GCP Cloud Run compute-provider tests. **#1171** needed one addition on this line: its new `TestActivity_List_Pagination` calls `activity.GetInfo(ctx)`, and the `go.temporal.io/sdk/activity` import is present on `main` but not in this file on `release/1.8.x`. The import is folded into the #1171 pick so each commit builds standalone. ### New commits **`backport: pin compatible dependency set and adjust #1156 for 1.8.x`** Dependency bumps are applied directly rather than cherry-picked, because taking them as-is pulls `go.temporal.io/api` past what server v1.31.2 can compile against (see *Dependency ceiling* below). Covers the isatty, x/tools, grpc, echo and testify bumps (#1145, #1148, #1132, #1175, #1174). Also pins `cliext` to a **tagged** SDK. `main` currently pins `go.temporal.io/sdk v1.46.1-0.20260720184640-f34dc3da35ab` — a commit SHA — in `cliext/go.mod`, which reaches the root build through `replace github.com/temporalio/cli/cliext => ./cliext`. That violates the tagged-dependencies rule for a public release. **`main` should be fixed separately.** **`fix(activity): use correct update-mask path for --task-queue`** Fixes a real, currently-shipping bug. `v1.8.2` sends update-mask path `task_queue_name`, which the server's `ParseFieldMask` normalizes to `taskQueueName` and which never matches its `taskQueue.name` key — so `temporal activity update-options --task-queue` **silently does nothing**. Verified A/B against the embedded server v1.31.2: - with `task_queue.name` → task queue updates as expected - with `task_queue_name` (what v1.8.2 ships) → unchanged The fix exists upstream only inside #1092, which cannot be backported, so it is extracted here as a one-liner. ## Dependency ceiling `server v1.31.2` **does not compile** against `go.temporal.io/api` ≥ **v1.62.10**: that release adds `CountNexusOperationExecutions` to the `WorkflowServiceClient` interface, which v1.31.2's `clientImpl`, `metricClient` and `retryableClient` do not implement. Because api is a transitive dependency, Go's minimal version selection drags it upward whenever anything that depends on it is bumped. That caps everything: | Dependency | Ceiling | Reason | |---|---|---| | `go.temporal.io/api` | v1.62.9 | v1.62.10 breaks server v1.31.2 | | `github.com/temporalio/ui-server/v2` | v2.50.1 | v2.51.0 → api v1.62.13; v2.53.x → api v1.63.x | | `go.temporal.io/sdk` | v1.42.0 | v1.43.1 → api v1.62.12; v1.46.0 → api v1.63.x | | `go.temporal.io/sdk/contrib/envconfig` | v1.0.0 | v1.0.1 changed `DefaultConfigFilePath` to one return value; `cliext/config.oauth.go` expects two | Resulting set — every Temporal direct dependency unchanged from `v1.8.2` except an api patch bump: ``` go.temporal.io/api v1.62.9 (was v1.62.8) go.temporal.io/server v1.31.2 unchanged go.temporal.io/sdk v1.41.1 unchanged go.temporal.io/sdk/contrib/envconfig v1.0.0 unchanged github.com/temporalio/ui-server/v2 v2.50.1 unchanged ``` **Note for the UI team:** this release ships **UI Server v2.50.1, unchanged**. The natural assumption would be v2.53.3, but that requires api v1.63.5. ## Excluded, and why ### Requires OSS Server v1.32.x / api v1.63.x | Commit | Reason | |---|---| | #1172 bump server for Nexus Query support | The server bump itself — out of scope for this line | | #1092 single SAA operator actions | Uses `Pause/Unpause/Reset ActivityExecutionRequest` and `UpdateActivityExecutionOptionsRequest`, absent from api v1.62.x | | #1152 enable SAA operator and batch commands in dev server | Needs `activity.EnableStandaloneActivityOperatorCommands` and `dynamicconfig.FrontendEnableBatchOperationsForStandaloneActivities`, absent from server v1.31.2 | | #1159 drop `activity unpause --reset-attempts`/`--reset-heartbeats` | Authored on top of #1092; its diff context already uses the new RPC names | | #1150 reject `update-options --start-delay` for workflow Activities | Needs `ActivityOptions.StartDelay`, new in api v1.63.5, via unbackported prerequisite #1113 | | #1131 render links on activity describe | Needs `ActivityExecutionInfo.GetLinks` and `DescribeActivityExecutionResponse.GetCallbacks`, new in api v1.63.5 | | #1151 bump UI server v2.53.1 | Requires api v1.63.4 | | #1164 bump UI server v2.53.3 | Requires api v1.63.5 | ### Excluded for other reasons **#1114 — staged connection diagnosis for opaque dial failures.** Depends on #1017 (*Unwrap System Nexus Operations in event history*), which introduced `dialClientWithCodec` and was never backported. On `release/1.8.x` only the two-value `dialClient` exists, and git silently misapplies #1114's hunks into it, producing three-value returns from a two-value function. Pulling in #1017 is too large for a patch release. **#1158 — docs: clarify `--query` targets Workflow Activities.** Pure documentation describing Standalone Activity semantics ("Omit `--workflow-id` to target a Standalone Activity…"). That behavior does not exist on this line, so backporting it would ship misleading help text. **#1155 — fix(activity): include options in batch `update-options`.** The change itself is correct, but batch `update-options` applies **nothing** on server v1.31.2. Probed directly: after a batch run, task queue is unchanged and `schedule_to_close_timeout` is still `0s`. Its new test `TestActivityOptionsUpdate_BatchMatchAll` fails consistently (3/3). Deferred to the release that carries the server bump. ## Reviewer notes **#1156 was adjusted rather than taken verbatim.** Upstream, `activity reset` had already lost `--reset-heartbeats` to an earlier SAA commit, so taking `main`'s version would have removed both flags at once. Only `--reset-attempts` is a no-op, the surviving help text still documents `--reset-heartbeats`, and its removal belongs to #1159 (excluded). This backport therefore removes only `--reset-attempts` and keeps the batch path on `c.ResetHeartbeats` rather than hardcoding `true`. Worth a careful look. **Known flaky test.** `TestHelp_AllFlag_ShorterCommandPathWinsi` failed on one full-suite run and passed on the next; it passes 5/5 in isolation. It arrives with #1137 and exists identically on `main`, so it is inherited rather than introduced — but expect occasional red CI. **Pre-existing `go vet` findings** (two lock-copy, one context leak) are byte-identical to the `v1.8.2` baseline. Not introduced here. --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Alex Stanfield <13949480+chaptersix@users.noreply.github.com> Co-authored-by: Nanook <nanookclaw@users.noreply.github.com> Co-authored-by: mani-j9 <mani.janumpally@temporal.io> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Jeri Lane <jeri.lane@temporal.io> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Sean Bollin <sean@sean-bollin.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Sean Kane <spkane31@gmail.com> Co-authored-by: Ross Nelson <axcess1@me.com> Co-authored-by: dryrun <dryrun@local> Co-authored-by: justinschoeff <justin.schoeff@temporal.io>
What changed?
Why
create-versionandupdate-version-compute-configcurrently require--aws-lambda-assume-role-arnand--aws-lambda-assume-role-external-idwhenever--aws-lambda-function-arnis set. The Temporal server governs whether these are actually mandatory via the globalrequire_role_and_external_idsetting (defaulttrue), so a role-less config is valid against servers where that setting is disabled — e.g. local dev against LocalStack. CLI's validation needs to be relaxed to allow role less config creation.How
This PR allows the role less config by adding a new CLI parameter
--aws-lambda-skip-role-and-external-id. By default the CLI keeps requiring both(role and id) fields and fails fast with an actionable client-side error that names the missing flag. Passing the flag specifically opts out of the client-side check and defers entirely to the server's policy.Testing
With flag set as default true
for when the local server's require_role_and_external_id flag is set to false