fix: Enforce workflow namespace authorization#3780
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughAdds a standalone Temporal server with certificate-based authorization, custom CLI startup, Docker packaging, local kind integration, and CI checks and image publishing. ChangesTemporal server integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant startServer
participant NewClaimMapper
participant NewAuthorizer
participant TemporalServer
CLI->>startServer: start command and service options
startServer->>NewClaimMapper: configure certificate claim mapping
startServer->>NewAuthorizer: configure authorization policy
startServer->>TemporalServer: initialize and start selected services
TemporalServer-->>CLI: stopped-server status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
What changedThis PR adds certificate-identity authorization to the NICo Temporal server and uses that server in local Kind/DevSpace. I exercised the clean bootstrap ordering, the live server image, mutual TLS, and every distinct admin, control, site, flow, and unknown-identity authorization branch at head Scenario and setupI used a dedicated development VM, a clean isolated checkout at the tested head, an isolated kubeconfig, and a new Kind cluster: HEAD=d2715beceddc68fc4f8c50e5d682ade54c75b1bd
CLUSTER=pr3780
CTX="kind-${CLUSTER}"
export KUBECONFIG="$PWD/.pr3780-kubeconfig"
git checkout "$HEAD"
test "$(git rev-parse HEAD)" = "$HEAD"
kind create cluster --name "$CLUSTER" --kubeconfig "$KUBECONFIG"After the clean Temporal bootstrap, I created cert-manager A VERIFY_NS=pr3780-verify
VERIFY_POD=temporal-verifier
TEMPORAL_ADDRESS=temporal-frontend.temporal:7233
SERVER_NAME=interservice.server.temporal.local
run_temporal() {
role=$1
shift
kubectl --context "$CTX" -n "$VERIFY_NS" exec "$VERIFY_POD" -- \
temporal "$@" \
--address "$TEMPORAL_ADDRESS" \
--tls-cert-path "/certs/${role}/tls.crt" \
--tls-key-path "/certs/${role}/tls.key" \
--tls-ca-path "/certs/${role}/ca.crt" \
--tls-server-name "$SERVER_NAME" \
--command-timeout 15s \
--color never
}
expect_denied() {
label=$1
role=$2
shift 2
if output=$(run_temporal "$role" "$@" 2>&1); then
echo "FAIL ${label} was allowed"
return 1
fi
case "$output" in
*unauthorized*|*Unauthorized*|*PermissionDenied*|*"not authorized"*|*"permission denied"*)
echo "PASS ${label} denied"
;;
*)
printf 'FAIL %s returned an unexpected error: %s\n' "$label" "$output"
return 1
;;
esac
}VerificationStep 1: Build and load the custom Temporal server before Helm waitsWhy this step exists: A clean cluster previously failed because Helm referenced Runnable command: devspace purge \
--kubeconfig "$KUBECONFIG" \
--kube-context "$CTX" \
-n nico-system \
--no-colors \
--no-warnObserved result: Why this proves the behavior: The clean bootstrap built and loaded the exact image before installing and waiting for Temporal. Step 2: Run all four Temporal services from the custom imageWhy this step exists: A successful image build is not proof that the Temporal services actually use it. Runnable command: kubectl --context "$CTX" -n temporal get deployment \
temporal-frontend temporal-history temporal-matching temporal-worker \
-o custom-columns='NAME:.metadata.name,READY:.status.readyReplicas,DESIRED:.spec.replicas,IMAGE:.spec.template.spec.containers[0].image'Observed result: Why this proves the behavior: Every Temporal server role was ready and running the PR's custom server image. Step 3: Allow cluster read operations for the control identityWhy this step exists: The control identity must be able to perform read-only cluster-scope operations. Runnable command: run_temporal control operator namespace list -o json >/dev/null
echo "PASS control cluster namespace list allowed"Observed result: Why this proves the behavior: The live frontend accepted Step 4: Restrict namespace registration to canonical lowercase UUIDsWhy this step exists: The control identity may register site namespaces, but not arbitrary or non-canonical names. Runnable command: run_temporal control operator namespace create \
--namespace aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa \
--retention 24h >/dev/null
echo "PASS lowercase canonical UUID registration allowed"
expect_denied "uppercase UUID registration" control \
operator namespace create \
--namespace BBBBBBBB-BBBB-4BBB-8BBB-BBBBBBBBBBBB \
--retention 24hObserved result: Why this proves the behavior: Registration accepted the canonical site namespace and rejected the non-canonical uppercase UUID. Step 5: Enforce control workflow namespace boundariesWhy this step exists: The control identity is a writer for Runnable command: run_temporal control workflow start \
--namespace cloud --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-control-cloud >/dev/null
echo "PASS control cloud workflow allowed"
run_temporal control workflow start \
--namespace site --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-control-site >/dev/null
echo "PASS control shared site workflow allowed"
run_temporal control workflow start \
--namespace 11111111-1111-1111-1111-111111111111 \
--task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-control-site-uuid >/dev/null
echo "PASS control UUID workflow allowed"
expect_denied "control flow workflow" control workflow start \
--namespace flow --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-control-flowObserved result: Why this proves the behavior: The same live Step 6: Deny operator APIs for the control identityWhy this step exists: Cluster read access must not grant the control identity unrestricted Operator Service access. Runnable command: expect_denied "control operator API" control operator search-attribute listObserved result: Why this proves the behavior: The control certificate could not call the Operator Service. Step 7: Enforce site identity namespace boundariesWhy this step exists: A site identity is a writer only for the shared Runnable command: run_temporal site workflow start \
--namespace site --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-site-shared >/dev/null
echo "PASS site identity shared site workflow allowed"
run_temporal site workflow start \
--namespace 11111111-1111-1111-1111-111111111111 \
--task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-site-own >/dev/null
echo "PASS site identity own UUID workflow allowed"
expect_denied "site identity other UUID workflow" site workflow start \
--namespace 22222222-2222-2222-2222-222222222222 \
--task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-site-other
expect_denied "site identity cloud workflow" site workflow start \
--namespace cloud --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-site-cloud
expect_denied "site identity flow workflow" site workflow start \
--namespace flow --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-site-flowObserved result: Why this proves the behavior: The site certificate was confined to the shared site namespace and the UUID encoded in its certificate identity. Step 8: Enforce flow identity namespace boundariesWhy this step exists: The flow identity is a writer only for the Runnable command: run_temporal flow workflow start \
--namespace flow --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-flow-own >/dev/null
echo "PASS flow identity flow workflow allowed"
expect_denied "flow identity site workflow" flow workflow start \
--namespace site --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-flow-site
expect_denied "flow identity cloud workflow" flow workflow start \
--namespace cloud --task-queue pr3780-verify \
--type VerifyWorkflow --workflow-id verify-flow-cloudObserved result: Why this proves the behavior: The live frontend allowed only the flow namespace for the flow certificate. Step 9: Allow health checks but deny application APIs for an unknown identityWhy this step exists: Unknown verified certificates may reach health checks but must not gain Temporal API access. Runnable command: run_temporal unknown operator cluster health
expect_denied "unknown identity namespace list" unknown operator namespace listObserved result: Why this proves the behavior: The health exception remained available while the same unknown certificate was denied a cluster read API. Step 10: Require a client certificate at the mutual-TLS boundaryWhy this step exists: Authorization must not be reachable without a verified client certificate. Runnable command: kubectl --context "$CTX" -n "$VERIFY_NS" exec "$VERIFY_POD" -- \
temporal operator cluster health \
--address "$TEMPORAL_ADDRESS" \
--tls \
--tls-ca-path /certs/unknown/ca.crt \
--tls-server-name "$SERVER_NAME" \
--command-timeout 15s \
--color neverObserved result: Why this proves the behavior: The server rejected the TLS connection before serving the health request when no client certificate was supplied. Step 11: Allow unrestricted admin operator accessWhy this step exists: The interservice admin certificate must retain full access after the custom authorizer is enabled. Runnable command: ADMIN_POD=$(kubectl --context "$CTX" -n temporal get pod \
-l app.kubernetes.io/component=admintools \
-o jsonpath='{.items[0].metadata.name}')
kubectl --context "$CTX" -n temporal exec "$ADMIN_POD" -- \
temporal operator search-attribute list \
--namespace cloud \
--address "$TEMPORAL_ADDRESS" \
--tls-cert-path /var/secrets/temporal/certs/server-interservice/tls.crt \
--tls-key-path /var/secrets/temporal/certs/server-interservice/tls.key \
--tls-ca-path /var/secrets/temporal/certs/server-interservice/ca.crt \
--tls-server-name "$SERVER_NAME" \
--command-timeout 15s \
--color never >/dev/null
echo "PASS admin identity operator API allowed"Observed result: Why this proves the behavior: The admin certificate could call the Operator Service that the control identity was denied. Step 12: Run the focused Temporal server testsWhy this step exists: The live checks cover representative public calls; the focused package tests cover the full claim-mapping and method-metadata branch table. Runnable command: cd rest-api/temporal-server
go test ./... -count=1Observed result: Why this proves the behavior: The complete focused authorization test package passed at the same head used by the live cluster. A supplementary full application |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-23 18:21:05 UTC | Commit: d2715be |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2715beced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| repository: localhost:5000/nico-temporal-server | ||
| tag: latest | ||
| pullPolicy: Never |
There was a problem hiding this comment.
For non-Kind Helm installs this override is never read; only values-kind.yaml now points at nico-temporal-server, while the default rest-api/temporal-helm/temporal/values.yaml still uses server.image.repository: temporalio/server. That means production Temporal pods keep running the upstream server without the new claim mapper/authorizer, so the namespace authorization fix is not actually enforced outside local Kind.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/rest-lint-and-test.yml (2)
265-270: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winGo module cache not extended for the new
temporal-servermatrix entry.
style(Lines 33-35) andlint-go(Lines 79-81) jobs were updated to includerest-api/temporal-server/go.sumincache-dependency-path, but thetestjob's "Set up Go" step still only referencesrest-api/go.sumeven though the matrix (Line 246) now runsmake rest-api/test-temporal-server, which downloads a separate, large dependency tree. This causes cache misses on every CI run for the temporal-server module's tests.🔧 Proposed fix
- name: Set up Go uses: actions/setup-go@v5 with: go-version: ${{ env.GO_VERSION }} cache: true - cache-dependency-path: rest-api/go.sum + cache-dependency-path: | + rest-api/go.sum + rest-api/temporal-server/go.sum🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/rest-lint-and-test.yml around lines 265 - 270, Update the test job’s “Set up Go” step to include rest-api/temporal-server/go.sum alongside rest-api/go.sum in cache-dependency-path, matching the existing style and lint-go job configuration for the temporal-server matrix tests.Source: Path instructions
93-98: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
golangci-lintstep isn't extended to the newtemporal-servermodule.
go fmt,revive, andgo vetwere all duplicated fortemporal-server, but thisgolangci-lintstep still only lintsrest-api(working-directory: rest-api,args: ... ./...). The new module ships without golangci-lint coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/rest-lint-and-test.yml around lines 93 - 98, Extend the “Run golangci-lint” workflow step to lint both the existing rest-api module and the new temporal-server module, preserving the current timeout and exit-code settings. Update the working-directory and target configuration so ./... is executed for each module, matching the duplicated go fmt, revive, and go vet coverage.Source: Path instructions
🧹 Nitpick comments (4)
rest-api/temporal-server/internal/auth/authorizer.go (1)
103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate canonical-UUID validation logic.
This canonical-UUID check (
uuid.Parse+ string round-trip equality) is reimplemented independently inclaim_mapper.go'smapIdentity(lines 86-93). See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/temporal-server/internal/auth/authorizer.go` around lines 103 - 106, Consolidate the canonical UUID validation used by isSiteNamespace and claim_mapper.go’s mapIdentity instead of maintaining duplicate uuid.Parse plus string round-trip logic. Reuse isSiteNamespace or extract a shared validation helper, preserving the existing requirement that parsing succeeds and the canonical string exactly matches the input.dev/deployment/devspace/bootstrap-prereqs.sh (1)
556-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the Makefile's build/load targets instead of duplicating them here.
rest-api/Makefilealready definesdocker-build-local/kind-loadtargets that build and loadnico-temporal-serverwith the same registry/tag conventions. Inlining an independentdocker build/kind loadhere risks drifting from the Makefile's build flags over time.♻️ Proposed direction
- log "Building ${temporal_image}" - docker build -t "${temporal_image}" \ - -f "${REPO_ROOT}/rest-api/docker/local/Dockerfile.nico-temporal-server" \ - "${REPO_ROOT}/rest-api" - - log "Loading ${temporal_image} into ${current_context}" - kind load docker-image --name "${current_context#kind-}" "${temporal_image}" + log "Building and loading ${temporal_image}" + make -C "${REPO_ROOT}/rest-api" docker-build-local kind-load \ + KIND_CLUSTER_NAME="${current_context#kind-}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dev/deployment/devspace/bootstrap-prereqs.sh` around lines 556 - 568, Replace the inline docker build and kind load commands in the kind-* bootstrap branch with calls to the existing rest-api Makefile targets docker-build-local and kind-load. Pass the current kind context as required by kind-load, and preserve the surrounding prerequisite checks and logging.rest-api/docker/production/Dockerfile.nico-temporal-server (2)
21-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the third-party base image by digest in production.
temporalio/server:1.26.2is a mutable tag pulled from a third-party registry. Based on learnings, this repo's threat model only exempts trusted NVIDIA distroless images from digest-pinning scrutiny; a third-party Temporal image doesn't fall under that carve-out, so pinning by digest here would harden the production build against upstream tag mutation.🔒 Proposed fix
-FROM temporalio/server:1.26.2 +FROM temporalio/server:1.26.2@sha256:<digest>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/docker/production/Dockerfile.nico-temporal-server` at line 21, Update the production Dockerfile’s temporalio/server base image reference to include its immutable registry digest while retaining the 1.26.2 image version. Apply the change to the FROM declaration only, using the verified digest for that exact third-party image.Source: Learnings
4-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal and production Dockerfiles are byte-for-byte identical. Both build stages,
COPY/RUNsteps, and the finaltemporalio/server:1.26.2base are duplicated verbatim across the two files, which will drift silently if one is updated without the other.
rest-api/docker/production/Dockerfile.nico-temporal-server#L4-L23: keep this as the canonical definition (with the digest-pinning fix noted separately) and derive the local variant from it (e.g., via a shared base stage, a build-arg-driven single Dockerfile, or a generation script) rather than maintaining two copies.rest-api/docker/local/Dockerfile.nico-temporal-server#L4-L23: replace with a thin wrapper over the production definition, or document explicitly why an independent copy is required for local dev.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/docker/production/Dockerfile.nico-temporal-server` around lines 4 - 23, Eliminate the duplicated Dockerfile definitions by keeping the production Dockerfile.nico-temporal-server as the canonical build and converting the local Dockerfile.nico-temporal-server into a thin wrapper or generated/shared reference; preserve the existing build stages and base image behavior, and explicitly document the local variant only if an independent definition is required. Apply the corresponding change in rest-api/docker/production/Dockerfile.nico-temporal-server lines 4-23 and rest-api/docker/local/Dockerfile.nico-temporal-server lines 4-23.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/temporal-server/cmd/server/main.go`:
- Around line 33-36: Update main to handle the error returned by
app.Run(os.Args): preserve successful execution, but when a top-level parsing or
dispatch error is returned, report it through the CLI’s established error/exit
mechanism so the process exits non-zero. Use the existing app and buildCLI flow
without changing subcommand behavior.
In `@rest-api/temporal-server/go.mod`:
- Line 14: Update the google.golang.org/grpc dependency in go.mod from v1.67.1
to v1.79.3 or later, ensuring the module metadata and checksums remain
consistent with the selected version.
In `@rest-api/temporal-server/internal/auth/authorizer_test.go`:
- Around line 158-165: Update the Authorize test assertions in
authorizer_test.go to use the project's testify assertion helpers instead of raw
t.Fatalf and t.Errorf calls, preserving the existing error and decision checks
and their failure messages.
In `@rest-api/temporal-server/internal/auth/claim_mapper_test.go`:
- Around line 76-89: The GetClaims test assertions in claim_mapper_test.go
should use testify assertions instead of raw t.Fatalf and t.Errorf calls.
Replace the error, extension type, identity kind, and site namespace checks with
the project’s established testify assertion style, preserving the existing
failure conditions and messages.
---
Outside diff comments:
In @.github/workflows/rest-lint-and-test.yml:
- Around line 265-270: Update the test job’s “Set up Go” step to include
rest-api/temporal-server/go.sum alongside rest-api/go.sum in
cache-dependency-path, matching the existing style and lint-go job configuration
for the temporal-server matrix tests.
- Around line 93-98: Extend the “Run golangci-lint” workflow step to lint both
the existing rest-api module and the new temporal-server module, preserving the
current timeout and exit-code settings. Update the working-directory and target
configuration so ./... is executed for each module, matching the duplicated go
fmt, revive, and go vet coverage.
---
Nitpick comments:
In `@dev/deployment/devspace/bootstrap-prereqs.sh`:
- Around line 556-568: Replace the inline docker build and kind load commands in
the kind-* bootstrap branch with calls to the existing rest-api Makefile targets
docker-build-local and kind-load. Pass the current kind context as required by
kind-load, and preserve the surrounding prerequisite checks and logging.
In `@rest-api/docker/production/Dockerfile.nico-temporal-server`:
- Line 21: Update the production Dockerfile’s temporalio/server base image
reference to include its immutable registry digest while retaining the 1.26.2
image version. Apply the change to the FROM declaration only, using the verified
digest for that exact third-party image.
- Around line 4-23: Eliminate the duplicated Dockerfile definitions by keeping
the production Dockerfile.nico-temporal-server as the canonical build and
converting the local Dockerfile.nico-temporal-server into a thin wrapper or
generated/shared reference; preserve the existing build stages and base image
behavior, and explicitly document the local variant only if an independent
definition is required. Apply the corresponding change in
rest-api/docker/production/Dockerfile.nico-temporal-server lines 4-23 and
rest-api/docker/local/Dockerfile.nico-temporal-server lines 4-23.
In `@rest-api/temporal-server/internal/auth/authorizer.go`:
- Around line 103-106: Consolidate the canonical UUID validation used by
isSiteNamespace and claim_mapper.go’s mapIdentity instead of maintaining
duplicate uuid.Parse plus string round-trip logic. Reuse isSiteNamespace or
extract a shared validation helper, preserving the existing requirement that
parsing succeeds and the canonical string exactly matches the input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80880ee6-0085-49e2-a7f3-d72b5d79315e
⛔ Files ignored due to path filters (1)
rest-api/temporal-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/rest-build-push-docker.yml.github/workflows/rest-lint-and-test.ymldev/deployment/devspace/bootstrap-prereqs.shrest-api/Makefilerest-api/THIRD-PARTY-LICENSESrest-api/docker/local/Dockerfile.nico-temporal-serverrest-api/docker/production/Dockerfile.nico-temporal-serverrest-api/temporal-helm/temporal/values-kind.yamlrest-api/temporal-server/cmd/server/main.gorest-api/temporal-server/go.modrest-api/temporal-server/internal/auth/authorizer.gorest-api/temporal-server/internal/auth/authorizer_test.gorest-api/temporal-server/internal/auth/claim_mapper.gorest-api/temporal-server/internal/auth/claim_mapper_test.go
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/temporal-helm/temporal/values.yaml`:
- Around line 40-43: Replace the image.tag value under the Temporal server image
configuration with an immutable release or CI-generated tag, and ensure release
automation publishes the required architecture-specific image using that tag.
Keep latest only for explicitly local-only configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ae1be6b-8ccb-4d83-968c-4162fc9a0b34
⛔ Files ignored due to path filters (1)
rest-api/temporal-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/rest-lint-and-test.ymlrest-api/docker/production/Dockerfile.nico-temporal-serverrest-api/temporal-helm/temporal/values.yamlrest-api/temporal-server/cmd/server/main.gorest-api/temporal-server/go.modrest-api/temporal-server/internal/auth/authorizer_test.gorest-api/temporal-server/internal/auth/claim_mapper.gorest-api/temporal-server/internal/auth/claim_mapper_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- rest-api/docker/production/Dockerfile.nico-temporal-server
- rest-api/temporal-server/go.mod
- rest-api/temporal-server/internal/auth/claim_mapper.go
- rest-api/temporal-server/internal/auth/authorizer_test.go
- .github/workflows/rest-lint-and-test.yml
- rest-api/temporal-server/internal/auth/claim_mapper_test.go
- rest-api/temporal-server/cmd/server/main.go
| image: | ||
| repository: temporalio/server | ||
| tag: 1.22.6 | ||
| repository: nvcr.io/0837451325059433/carbide-dev/nico-temporal-server | ||
| tag: latest | ||
| pullPolicy: IfNotPresent |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use an immutable image tag instead of latest.
With latest and IfNotPresent, nodes can retain an older cached image while new nodes pull a different image. This makes deployments non-reproducible and can leave Temporal services running mixed server versions. Have release automation publish the required architecture-specific image and set this value to an immutable release/CI tag; reserve latest for explicitly local-only values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/temporal-helm/temporal/values.yaml` around lines 40 - 43, Replace
the image.tag value under the Temporal server image configuration with an
immutable release or CI-generated tag, and ensure release automation publishes
the required architecture-specific image using that tag. Keep latest only for
explicitly local-only configurations.
Source: Path instructions
1 participant
Adds a NICo-specific Temporal server that maps client-certificate identities and restricts workflow service calls to the permitted cluster operations and namespaces. It also builds and packages that server for the REST local-development and production paths.
Related issues
None.
Type of Change
Breaking Changes
Testing
Additional Notes
Draft pending local Kind/DevSpace verification.