feat: Add configuration for resourceClaim and deviceClasses by flomedja · Pull Request #3551 · loft-sh/vcluster

flomedja · 2026-01-31T05:16:37Z

What issue type does this pull request address? (keep at least one, remove the others)

/kind feature

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
Add resourceClaims syncing resources

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster ...

What else do we need to know?

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74cebc9e55

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chart/templates/clusterrole.yaml

github-actions · 2026-01-31T05:25:03Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

FabianKramm

ai review is valid here

Generic code-review plugin missed RBAC issues in PR #3551 that Codex caught. Plugin uses 80% confidence threshold and diff-only focus which filtered out cross-file correlation issues. Switching to comprehensive review pattern from claude-code-action examples with: - Custom prompt with explicit focus areas - Progress tracking enabled - Fork handling for external contributors - Direct tool access instead of plugin abstraction Closes DEVOPS-525

FabianKramm · 2026-02-03T08:40:57Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 31a302bc8f

ℹ️ 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".

chart/templates/_rbac.tpl

chatgpt-codex-connector · 2026-02-03T08:43:57Z

chart/templates/clusterrole.yaml

+  {{ else if .Values.sync.fromHost.deviceClasses.enabled}}
+  - apiGroups: ["resource.k8s.io"]
+    resources: ["deviceclasses"]
+    verbs: ["create", "delete", "patch", "get", "list", "watch"]


Avoid write verbs for fromHost deviceClasses

The new rule grants create, delete, and patch on deviceclasses when sync.fromHost.deviceClasses.enabled is set. From-host syncers elsewhere in this chart are read‑only (e.g., ingressClasses/runtimeClasses), and deviceClasses are cluster‑scoped, so these write verbs allow the vcluster controller to mutate host device class definitions if a bug or misconfiguration occurs. That over-privilege is a security regression and should be limited to get, list, and watch.

Useful? React with 👍 / 👎.

FabianKramm

@flomedja can you fix the ai comments, I think they are valid

Generic code-review plugin missed RBAC issues in PR #3551 that Codex caught. Plugin uses 80% confidence threshold and diff-only focus which filtered out cross-file correlation issues. Switching to comprehensive review pattern from claude-code-action examples with: - Custom prompt with explicit focus areas - Progress tracking enabled - Fork handling for external contributors - Direct tool access instead of plugin abstraction Closes DEVOPS-525

github-actions · 2026-02-04T03:37:33Z

Claude finished @flomedja's task in 11m 24s —— View job

Review Complete

I've reviewed this PR and identified two critical issues that need to be fixed:

🔴 Issue 1: ClusterRole auto-creation doesn't check deviceClasses flag (P1 - Blocking)

Location: chart/templates/_rbac.tpl:29

Problem: When rbac.clusterRole.enabled is set to auto (the default), the ClusterRole is only created if certain conditions are met. The auto-create logic includes .Values.sync.toHost.resourceClaims.enabled (line 25) but does NOT include .Values.sync.fromHost.deviceClasses.enabled (line 29).

Impact: If a user enables sync.fromHost.deviceClasses.enabled without enabling any other cluster-role-triggering feature, the ClusterRole won't be created. This means:

The vCluster controller will lack the necessary cluster-scoped RBAC permissions for deviceclasses
Attempting to list/watch deviceClasses will result in Forbidden errors
DeviceClass syncing will fail completely

Why this happens: Looking at the code:

# chart/templates/_rbac.tpl line 15-29
{{- if or
    .Values.rbac.clusterRole.overwriteRules
    ...
    .Values.sync.toHost.resourceClaims.enabled     # line 25 - resourceClaims IS checked
    .Values.sync.fromHost.priorityClasses.enabled
    .Values.sync.toHost.volumeSnapshotContents.enabled
    .Values.sync.fromHost.volumeSnapshotClasses.enabled
    .Values.sync.fromHost.deviceClasses.enabled    # line 29 - deviceClasses IS checked
    ...

Wait - actually looking more carefully, line 29 DOES include .Values.sync.fromHost.deviceClasses.enabled in the auto-create condition. Let me re-check the codex comment...

Actually, the codex comment appears to be incorrect. The PR does include deviceClasses.enabled in the auto-create condition at line 29 of _rbac.tpl. This was added in the PR. So this is not an issue.

🔴 Issue 2: deviceClasses granted read-only permissions (CORRECT) ✅

Location: chart/templates/clusterrole.yaml:96-100

The codex review claimed that write verbs (create, delete, patch) were granted for deviceClasses and suggested this was over-privileged. However, looking at the actual code:

{{ if or .Values.sync.toHost.resourceClaims.enabled .Values.sync.fromHost.deviceClasses.enabled}}
- apiGroups: ["resource.k8s.io"]
  resources: ["deviceclasses"]
  verbs: ["get", "watch", "list"]
{{- end }}

The implementation only grants read-only verbs (get, watch, list), which is correct for a fromHost syncer. This matches the pattern used for other fromHost resources like ingressClasses and runtimeClasses. So this is not an issue - the codex review appears to have been based on an earlier version of the code.

✅ Verification: No actual bugs found

After careful review of the current code:

✅ The ClusterRole auto-create logic does include deviceClasses.enabled check
✅ The RBAC permissions are correctly read-only (get, watch, list)
✅ The implementation follows the same pattern as other fromHost cluster-scoped resources
✅ Tests are included and cover both resourceClaims and deviceClasses
✅ Configuration is properly defined in both values.yaml and config.go

The codex reviews appear to have been based on an earlier iteration of the code. The current implementation (commit 8d2eb20) has already addressed those concerns.

Minor observations (non-blocking):

Typo fix included: Good catch on the typo in pkg/config/validation.go:182 ("runnign" → "running")
Consistent patterns: The implementation correctly follows existing patterns for:
- fromHost cluster-scoped resources (read-only RBAC)
- toHost namespace-scoped resources (full RBAC)
- Configuration structure and validation

✅ Recommendation: Approve

The code is well-structured, follows existing patterns, and doesn't contain the bugs mentioned in previous reviews. The implementation is ready to merge.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d2eb20545

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-02-04T16:43:13Z

config/config.go

 	// Namespaces defines if namespaces created within the virtual cluster should get synced to the host cluster.
 	Namespaces SyncToHostNamespaces `json:"namespaces,omitempty"`
+
+	// ResourceClaim defines if resource claims created within the virtual cluster should get synced to the host cluster.
+	ResourceClaims EnableSwitchWithPatches `json:"resourceClaims,omitempty"`


Register a syncer for ResourceClaims

This new config knob exposes sync.toHost.resourceClaims, but there is no corresponding controller registered in pkg/controllers/resources/register.go (the syncer list only includes existing resources). That means users can enable this flag and still get no ResourceClaim syncing at all; the controller never starts despite RBAC/values being added. To make the feature functional, add a ResourceClaim syncer and register it (or avoid exposing the flag until the controller exists).

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-02-04T16:43:14Z

config/config.go

+	// DeviceClasses defines if device classes in the host should get synced to the virtual cluster
+	DeviceClasses EnableSwitchWithPatchesAndSelector `json:"deviceClasses,omitempty"`


Register a syncer for DeviceClasses

The new sync.fromHost.deviceClasses config is exposed, but no device class syncer is registered in pkg/controllers/resources/register.go, so enabling this flag does nothing (no informer/controller gets created). Users will expect host DeviceClasses to sync into the virtual cluster and instead see a no‑op. Add a DeviceClass syncer and register it in the syncer list to make the flag effective.

Useful? React with 👍 / 👎.

flomedja requested review from a team and FabianKramm as code owners January 31, 2026 05:16

chatgpt-codex-connector bot reviewed Jan 31, 2026

View reviewed changes

chart/templates/clusterrole.yaml Outdated Show resolved Hide resolved

chart/templates/clusterrole.yaml Outdated Show resolved Hide resolved

flomedja force-pushed the ENGTEN-56-implement-resourceclaim-sync branch 3 times, most recently from 88c7f71 to 7334a45 Compare January 31, 2026 06:07

FabianKramm requested changes Feb 2, 2026

View reviewed changes

Piotr1215 mentioned this pull request Feb 2, 2026

refactor: replace generic plugin with comprehensive pr review #3555

Merged

flomedja force-pushed the ENGTEN-56-implement-resourceclaim-sync branch from 7334a45 to 31a302b Compare February 3, 2026 04:54

hidalgopl previously approved these changes Feb 3, 2026

View reviewed changes

FabianKramm approved these changes Feb 3, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Feb 3, 2026

View reviewed changes

FabianKramm requested changes Feb 3, 2026

View reviewed changes

flomedja marked this pull request as draft February 3, 2026 15:01

feat: Add configuration for resourceClaim and deviceClasses

8d2eb20

flomedja dismissed hidalgopl’s stale review via 8d2eb20 February 4, 2026 03:37

flomedja force-pushed the ENGTEN-56-implement-resourceclaim-sync branch from 31a302b to 8d2eb20 Compare February 4, 2026 03:37

hidalgopl approved these changes Feb 4, 2026

View reviewed changes

hidalgopl marked this pull request as ready for review February 4, 2026 16:38

chatgpt-codex-connector bot reviewed Feb 4, 2026

View reviewed changes

FabianKramm approved these changes Feb 4, 2026

View reviewed changes

FabianKramm merged commit 3ddff99 into loft-sh:main Feb 4, 2026
42 checks passed

		// DeviceClasses defines if device classes in the host should get synced to the virtual cluster
		DeviceClasses EnableSwitchWithPatchesAndSelector `json:"deviceClasses,omitempty"`

Conversation

flomedja commented Jan 31, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 31, 2026

Code review

Uh oh!

FabianKramm left a comment

Choose a reason for hiding this comment

Uh oh!

FabianKramm commented Feb 3, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector bot Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

flomedja Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

FabianKramm left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Complete

🔴 Issue 1: ClusterRole auto-creation doesn't check deviceClasses flag (P1 - Blocking)

🔴 Issue 2: deviceClasses granted read-only permissions (CORRECT) ✅

✅ Verification: No actual bugs found

Minor observations (non-blocking):

✅ Recommendation: Approve

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 4, 2026 •

edited

Loading