Skip to content

fix(operator): cap k8s discovery RBAC label values to 63 chars#11709

Open
kulkarnisamr wants to merge 1 commit into
ai-dynamo:mainfrom
kulkarnisamr:sk/rbac-label-cap
Open

fix(operator): cap k8s discovery RBAC label values to 63 chars#11709
kulkarnisamr wants to merge 1 commit into
ai-dynamo:mainfrom
kulkarnisamr:sk/rbac-label-cap

Conversation

@kulkarnisamr

@kulkarnisamr kulkarnisamr commented Jul 15, 2026

Copy link
Copy Markdown

Summary

  • keep discovery ServiceAccount/Role/RoleBinding resource names unchanged
  • cap only app.kubernetes.io/name label values to Kubernetes' 63-char limit
  • add deterministic hash-suffix truncation for over-limit label values
  • add boundary tests for unchanged short values and capped over-limit values

Test plan

  • go test ./internal/discovery

Summary by CodeRabbit

  • Bug Fixes

    • Fixed Kubernetes discovery resources created from long service account names so their labels remain within Kubernetes’ 63-character limit.
    • Preserved deterministic label values while preventing invalid resource labels.
  • Tests

    • Added coverage for short, boundary-length, and over-limit label values.
    • Added validation across discovery service accounts, roles, and role bindings.

@kulkarnisamr kulkarnisamr requested a review from a team as a code owner July 15, 2026 14:18
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@kulkarnisamr kulkarnisamr temporarily deployed to external_collaborator July 15, 2026 14:18 — with GitHub Actions Inactive
@kulkarnisamr kulkarnisamr temporarily deployed to external_collaborator July 15, 2026 14:18 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi kulkarnisamr! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions Bot added fix external-contribution Pull request is from an external contributor deployment::k8s Relates to dynamo deployment in kubernetes labels Jul 15, 2026
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Discovery label capping

Layer / File(s) Summary
Label value generation
deploy/operator/internal/discovery/resource.go, deploy/operator/internal/discovery/resource_test.go
Adds Kubernetes label length constants and a helper that preserves short names while hashing and truncating overlong names.
Resource label integration
deploy/operator/internal/discovery/resource.go, deploy/operator/internal/discovery/resource_test.go
Applies capped values to service account, role, and role binding labels, with coverage for generated resource labels.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers summary and tests, but it misses the required Overview, Details, Where should reviewer start, and Related Issues sections. Add the template sections with Overview, Details, reviewer start files, and a Related Issues entry or confirmed no-issue checkbox.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: capping Kubernetes discovery RBAC label values at 63 characters.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
deploy/operator/internal/discovery/resource_test.go (1)

32-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the deterministic label value, not only its shape.

These tests pass for an arbitrary shortened value containing -, or even empty resource labels. Assert the expected SHA-256 suffix and that every resource label equals the capped service-account label.

Proposed test strengthening
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"strings"
 	"testing"
 )
 
 			if tc.wantHashPart && !strings.Contains(got, "-") {
 				t.Fatalf("getK8sDiscoveryLabelValue(%q) = %q, want hash separator", tc.input, got)
 			}
+			if tc.wantHashPart {
+				sum := sha256.Sum256([]byte(tc.input))
+				want := tc.input[:maxLabelValueLen-hashLen-1] + "-" +
+					hex.EncodeToString(sum[:])[:hashLen]
+				if got != want {
+					t.Fatalf("getK8sDiscoveryLabelValue(%q) = %q, want %q", tc.input, got, want)
+				}
+			}
 		})
 	}
 }
 
+	wantLabel := getK8sDiscoveryLabelValue(GetK8sDiscoveryServiceAccountName(dgdName))
 	for _, label := range []string{
 		sa.Labels["app.kubernetes.io/name"],
 		role.Labels["app.kubernetes.io/name"],
 		rb.Labels["app.kubernetes.io/name"],
 	} {
+		if label != wantLabel {
+			t.Fatalf("label = %q, want %q", label, wantLabel)
+		}
 		if len(label) > maxLabelValueLen {

Also applies to: 72-79

🤖 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 `@deploy/operator/internal/discovery/resource_test.go` around lines 32 - 36,
Strengthen the truncation tests around the “over limit truncated with hash”
cases to assert the exact deterministic SHA-256 suffix rather than only checking
its shape. Verify that all generated resource labels equal the capped
service-account label, including the corresponding cases near the referenced
test entries, while preserving the existing length and equality assertions.
🤖 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.

Nitpick comments:
In `@deploy/operator/internal/discovery/resource_test.go`:
- Around line 32-36: Strengthen the truncation tests around the “over limit
truncated with hash” cases to assert the exact deterministic SHA-256 suffix
rather than only checking its shape. Verify that all generated resource labels
equal the capped service-account label, including the corresponding cases near
the referenced test entries, while preserving the existing length and equality
assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 28b9236b-1117-42f5-aa91-ad860a1e9598

📥 Commits

Reviewing files that changed from the base of the PR and between ce068df and 3cd2ec6.

📒 Files selected for processing (2)
  • deploy/operator/internal/discovery/resource.go
  • deploy/operator/internal/discovery/resource_test.go

@kulkarnisamr kulkarnisamr temporarily deployed to external_collaborator July 15, 2026 14:27 — with GitHub Actions Inactive
@kulkarnisamr kulkarnisamr temporarily deployed to external_collaborator July 15, 2026 15:51 — with GitHub Actions Inactive
Keep k8s discovery ServiceAccount, Role, and RoleBinding resource names unchanged while capping only app.kubernetes.io/name label values with a deterministic hash suffix when needed. Add boundary tests to cover unchanged <=63 values and capped values across all three resources.

Signed-off-by: Sameer Kulkarni <skulkarni@coreweave.com>
Signed-off-by: Sameer Kulkarni <kulkarnisameer7@gmail.com>
@kulkarnisamr kulkarnisamr temporarily deployed to external_collaborator July 15, 2026 15:54 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment::k8s Relates to dynamo deployment in kubernetes external-contribution Pull request is from an external contributor fix size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant