Skip to content

Add hash collision test for NodeType ID generation#4815

Open
tmchow wants to merge 3 commits intoarmadaproject:masterfrom
tmchow:test/node-type-hash-collision
Open

Add hash collision test for NodeType ID generation#4815
tmchow wants to merge 3 commits intoarmadaproject:masterfrom
tmchow:test/node-type-hash-collision

Conversation

@tmchow
Copy link
Copy Markdown

@tmchow tmchow commented Apr 4, 2026

Adds tests for nodeTypeIdFromTaintsAndLabels that verify:

  1. No hash collisions across realistic Kubernetes taint/label combinations (taint keys x effects, label keys x values, mixed taint+label inputs)
  2. The hash is never zero, even for empty inputs

Tests 60+ unique inputs with zero collisions observed. Removes the TODO comment that requested this coverage.

Closes #4778

This PR was authored by a human with AI coding assistance.

Compound Engineering

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 4, 2026

Greptile Summary

This PR adds hash collision tests for nodeTypeIdFromTaintsAndLabels (closing #4778) and bundles a bonus refactoring that replaces all plain-string context keys ("principal", "user", "requestId") with a new typed ctxkeys.ContextKey type, preventing accidental collisions between packages. All writers and readers of those context values have been updated consistently, and a grep across the repository confirms no stale plain-string lookups remain.

Confidence Score: 5/5

This PR is safe to merge; all changes are either test additions or a consistent typed-key refactor with no remaining plain-string callers.

The collision tests are correct and cover the stated combinations. The ctxkeys refactoring is complete — every writer and reader was updated together, the grep finds no leftover plain-string lookups, and the test files were updated in lockstep. No logic was changed in production code outside of the key type.

No files require special attention.

Important Files Changed

Filename Overview
internal/scheduler/internaltypes/node_type_test.go Adds two new tests: collision detection across 60+ realistic taint/label combinations and a zero-hash guard; logic is correct and the TODO is removed.
internal/common/ctxkeys/ctxkeys.go New package that introduces a typed ContextKey string type and three constants (PrincipalKey, UserKey, RequestIDKey) to replace plain string context keys across the codebase.
internal/common/auth/common.go Migrates principalKey from a plain string literal to ctxkeys.PrincipalKey typed constant; all readers (GetPrincipal) and writers (WithPrincipal) are updated consistently.
internal/common/armadacontext/armada_context.go FromGrpcCtx now reads "user" and "requestId" via typed ctxkeys constants instead of plain strings, consistent with the writers in auth and requestid packages.
internal/common/requestid/interceptors.go AddToIncomingContext now stores the request ID using ctxkeys.RequestIDKey instead of the plain string "requestId".
internal/common/grpc/grpc.go InterceptorLogger reads user and requestId from context via typed ctxkeys constants, completing the migration away from plain string keys.
internal/server/submit/submit_test.go Test setup updated to store the principal using ctxkeys.PrincipalKey so it is found by GetPrincipal after the typed-key migration.
internal/scheduler/internaltypes/node_type.go Removed the TODO comment requesting hash-collision test coverage; no logic changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Incoming gRPC request] --> B[requestid.UnaryServerInterceptor]
    B --> C["context.WithValue(ctx, ctxkeys.RequestIDKey, id)"]
    C --> D[auth.CreateGrpcMiddlewareAuthFunction]
    D --> E["context.WithValue(ctx, ctxkeys.UserKey, name)"]
    E --> F["WithPrincipal → context.WithValue(ctx, ctxkeys.PrincipalKey, principal)"]
    F --> G[Handler / interceptors read context]
    G --> H["grpc.InterceptorLogger\nctx.Value(ctxkeys.UserKey)\nctx.Value(ctxkeys.RequestIDKey)"]
    G --> I["auth.GetPrincipal\nctx.Value(ctxkeys.PrincipalKey)"]
    G --> J["armadacontext.FromGrpcCtx\nctx.Value(ctxkeys.UserKey)\nctx.Value(ctxkeys.RequestIDKey)"]
Loading

Reviews (3): Last reviewed commit: "Fix submit_test.go to use typed context ..." | Re-trigger Greptile

@tmchow tmchow force-pushed the test/node-type-hash-collision branch from e73d5c1 to b6801d3 Compare April 5, 2026 03:56
tmchow added 3 commits April 7, 2026 14:08
Introduce a ctxkeys package with a custom ContextKey type to prevent
context value collisions from bare string keys. Replace all plain
string context keys ("principal", "user", "requestId") with typed
constants from the new package.

The ctxkeys package is a leaf dependency with no imports, avoiding
circular dependencies between auth, requestid, and armadacontext.

Closes armadaproject#4780

Signed-off-by: Trevin Chow <trevin@trevinchow.com>
Add tests verifying that nodeTypeIdFromTaintsAndLabels produces unique
hashes for distinct combinations of taints, labels, and unset indexed
labels using realistic Kubernetes inputs. Also verify the hash is
never zero, even for empty inputs.

Removes the TODO comment that requested these tests.

Closes armadaproject#4778

Signed-off-by: Trevin Chow <trevin@trevinchow.com>
Update four test call-sites to use ctxkeys.PrincipalKey instead of the
plain string key "principal". Without this, auth.GetPrincipal silently
falls back to anonymousPrincipal since Go context lookups are type-sensitive.

Signed-off-by: Trevin Chow <trevin@trevinchow.com>
@tmchow tmchow force-pushed the test/node-type-hash-collision branch from b6801d3 to 0df0735 Compare April 7, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add hash collision test for NodeType ID generation

1 participant