Skip to content

fix(operator): use per-router names for VLLMRouter pod-viewer RBAC#993

Open
Anai-Guo wants to merge 1 commit into
vllm-project:mainfrom
Anai-Guo:fix/vllmrouter-per-router-rbac
Open

fix(operator): use per-router names for VLLMRouter pod-viewer RBAC#993
Anai-Guo wants to merge 1 commit into
vllm-project:mainfrom
Anai-Guo:fix/vllmrouter-per-router-rbac

Conversation

@Anai-Guo

Copy link
Copy Markdown
Contributor

What

VLLMRouter reconciliation creates a Role and RoleBinding (granting get/list/watch on pods) using fixed names pod-viewer-role / pod-viewer-binding, both owner-referenced to the creating router. This makes it impossible to run more than one VLLMRouter in the same namespace (#900):

  • The second router finds the existing RoleBinding (creation is skipped) whose only subject is the first router's ServiceAccount — so the second router's SA is never bound and its pods cannot list pods.
  • Because the shared Role/RoleBinding are owned by the router that first created them, deleting that router garbage-collects them, breaking the surviving router.

Change

Derive both RBAC object names from the router name via two small helpers:

func podViewerRoleName(router *servingv1alpha1.VLLMRouter) string {
    return fmt.Sprintf("%s-pod-viewer-role", router.Name)
}
func podViewerBindingName(router *servingv1alpha1.VLLMRouter) string {
    return fmt.Sprintf("%s-pod-viewer-binding", router.Name)
}

Each router now gets its own Role and RoleBinding bound to its own ServiceAccount, so multiple routers coexist in one namespace and lifecycle events on one no longer affect the others. The RoleRef in the binding is updated to point at the per-router role name.

No behavior change for single-router namespaces (name is just prefixed). fmt is already imported; no test references the old fixed names.

Fixes #900

🤖 Generated with Claude Code

The Role "pod-viewer-role" and RoleBinding "pod-viewer-binding" created
for a VLLMRouter used fixed names and were owner-referenced to the
creating router. With two VLLMRouters in the same namespace this breaks:

- the second router reuses the existing RoleBinding (creation is skipped),
  so its ServiceAccount is never bound and its pods cannot list pods;
- deleting either router garbage-collects the shared Role/RoleBinding,
  breaking the surviving router.

Derive both names from the router name so each router gets its own Role
and RoleBinding bound to its own ServiceAccount.

Fixes vllm-project#900

Signed-off-by: Anai-Guo <antai12232931@anaiguo.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the VLLM router controller to use dynamic, per-router names for the RBAC Role and RoleBinding instead of static names, allowing multiple VLLMRouters to coexist in the same namespace. The feedback suggests adding nil checks for the router parameter in the helper functions podViewerRoleName and podViewerBindingName to prevent potential nil pointer dereferences.

Comment on lines +479 to +481
func podViewerRoleName(router *servingv1alpha1.VLLMRouter) string {
return fmt.Sprintf("%s-pod-viewer-role", router.Name)
}

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.

medium

For defensive programming, we should ensure that router is not nil before accessing its fields to prevent potential nil pointer dereference panics.

Suggested change
func podViewerRoleName(router *servingv1alpha1.VLLMRouter) string {
return fmt.Sprintf("%s-pod-viewer-role", router.Name)
}
func podViewerRoleName(router *servingv1alpha1.VLLMRouter) string {
if router == nil {
return ""
}
return fmt.Sprintf("%s-pod-viewer-role", router.Name)
}

Comment on lines +485 to +487
func podViewerBindingName(router *servingv1alpha1.VLLMRouter) string {
return fmt.Sprintf("%s-pod-viewer-binding", router.Name)
}

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.

medium

For defensive programming, we should ensure that router is not nil before accessing its fields to prevent potential nil pointer dereference panics.

func podViewerBindingName(router *servingv1alpha1.VLLMRouter) string {
	if router == nil {
		return ""
	}
	return fmt.Sprintf("%s-pod-viewer-binding", router.Name)
}

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.

bug: use multiple crd vllmrouter in the same namespace

1 participant