RHINENG-25147: refactor workspaces#2176
Open
Dugowitch wants to merge 10 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Dugowitch, your pull request is larger than the review limit of 150000 diff characters
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2176 +/- ##
==========================================
- Coverage 59.13% 59.06% -0.07%
==========================================
Files 134 136 +2
Lines 8738 8761 +23
==========================================
+ Hits 5167 5175 +8
- Misses 3028 3040 +12
- Partials 543 546 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MichaelMraka
requested changes
Apr 30, 2026
Reviewer's GuideRefactors workspace handling across the service by replacing JSONB-based File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- ApplyInventoryWorkspaceFilter now always applies
WHERE si.workspace_id IN (?)even whenworkspaceIDsis empty (and only logs a warning); consider short‑circuiting and returning the unmodified tx (or an always‑false predicate) to avoid unexpected SQL/behavior on empty slices. - RBAC still computes and stores inventory groups under its own KeyInventoryGroups, but the rest of the code now reads workspace IDs from utils.KeyInventoryWorkspaces and ApplyInventoryWorkspaceFilter no longer accepts groups; you should either adapt RBAC to emit workspace IDs or add a translation layer, otherwise RBAC group restrictions are effectively ignored in queries.
- The TODOs around caching conditions (e.g., advisoriesCommon and AdvisoriesExportHandler) indicate the cache path is now always disabled because there is always at least the root workspace; it would be good to either remove the dead cache branch or update the conditions to make caching usable again under the new workspace model.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ApplyInventoryWorkspaceFilter now always applies `WHERE si.workspace_id IN (?)` even when `workspaceIDs` is empty (and only logs a warning); consider short‑circuiting and returning the unmodified tx (or an always‑false predicate) to avoid unexpected SQL/behavior on empty slices.
- RBAC still computes and stores inventory groups under its own KeyInventoryGroups, but the rest of the code now reads workspace IDs from utils.KeyInventoryWorkspaces and ApplyInventoryWorkspaceFilter no longer accepts groups; you should either adapt RBAC to emit workspace IDs or add a translation layer, otherwise RBAC group restrictions are effectively ignored in queries.
- The TODOs around caching conditions (e.g., advisoriesCommon and AdvisoriesExportHandler) indicate the cache path is now always disabled because there is always at least the root workspace; it would be good to either remove the dead cache branch or update the conditions to make caching usable again under the new workspace model.
## Individual Comments
### Comment 1
<location path="listener/upload.go" line_range="363-372" />
<code_context>
updatesReqJSONString := string(updatesReqJSON)
- hostWorkspaces := inventory.Groups(host.Groups)
+ var workspaceID uuid.UUID
+ var workspaceName *string
+ if l := len(host.Groups); l >= 1 {
+ if idString := host.Groups[0].ID; idString != "" {
+ workspaceID, err = uuid.Parse(idString)
+ if err != nil {
+ utils.LogError("workspaceID", idString, "invalid workspace UUID")
+ return nil, errors.New("received invalid workspace UUID")
+ }
+ }
+ if host.Groups[0].Name != "" {
+ workspaceName = &host.Groups[0].Name
+ }
+ if l != 1 {
+ utils.LogWarn(
+ "host_id", host.ID, "org_id", host.OrgID, "workspaces", host.Groups,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid persisting a zero UUID when no or invalid workspace ID is present
Because `workspaceID` is always initialized and passed by address, `SystemInventory` will always see a non-nil pointer:
- With no groups, you persist `00000000-0000-0000-0000-000000000000` instead of `NULL`.
- With an empty `host.Groups[0].ID`, you silently use the zero UUID, which is indistinguishable in the DB from a real value and doesn’t clearly mean “no workspace”.
Instead, either:
- Declare `var workspaceID *uuid.UUID` and only set it on successful parse, or
- Keep `workspaceID uuid.UUID` but only set `WorkspaceID` to a non-nil pointer when a valid ID was parsed.
This preserves the intended `NULL` semantics for missing/unknown workspaces and avoids treating the zero UUID as a valid value.
</issue_to_address>
### Comment 2
<location path="manager/middlewares/kessel_test.go" line_range="112" />
<code_context>
- inventoryGroups, found := c.Get(utils.KeyInventoryGroups)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative test for missing or empty workspaces in hasPermissionKessel
The updated `hasPermissionKessel` now aborts with `http.StatusUnauthorized` when the collected workspace IDs slice is empty, but `TestHasPermissionKessel` only covers the success path. Please add a test where the `workspaces` stream is empty (or decoded to an empty slice) and assert that the handler returns 401 and does not set `utils.KeyInventoryWorkspaces` to ensure this failure path is covered and guarded against regressions.
Suggested implementation:
```golang
func TestBuildPermission(t *testing.T) {
w := httptest.NewRecorder()
c, _ := gin.CreateTestContext(w)
c.Request = &http.Request{Method: http.MethodGet, Header: http.Header{}}
permission := buildPermission(c)
require.NotNil(t, permission)
c.Request.Header.Set("x-rh-identity", "ewogICAgImVudGl0bGVtZW50cyI6IHsKICAgICAgICAiaW5zaWdodHMiOiB7CiAgICAgICAgICAgICJpc19lbnRpdGxlZCI6IHRydWUKICAgICAgICB9LAogICAgICAgICJjb3N0X21hbmFnZW1lbnQiOiB7CiAgICAgICAgICAgICJpc19lbnRpdGxlZCI6IHRydWUKICAgICAgICB9LAogICAgICAgICJhbnNpYmxlIjogewogICAgICAgICAgICAiaXNfZW50aXRsZWQiOiB0cnVlCiAgICAgICAgfSwKICAgICAgICAib3BlbnNoaWZ0IjogewogICAgICAgICAgICAiaXNfZW50aXRsZWQiOiB0cnVlCiAgICAgICAgfSwKICAgICAgICAic21hcnRfbWFuYWdlbWVudCI6IHsKICAgICAgICAgICAgImlzX2VudGl0bGVkIjogdHJ1ZQogICAgICAgIH0sCiAgICAgICAgIm1pZ3JhdGlvbnMiOiB7CiAgICAgICAgICAgICJpc19lbnRpdGxlZCI6IHRydWUKICAgICAgICB9CiAgICB9LAogICAgImlkZW50aXR5IjogewogICAgICAgICJpbnRlcm5hbCI6IHsKICAgICAgICAgICAgImF1dGhfdGltZSI6IDI5OSwKICAgICAgICAgICAgImF1dGhfdHlwZSI6ICJiYXNpYy1hdXRoIiwKICAgICAgICAgICAgIm9yZ19pZCI6ICIxMTc4OTc3MiIKICAgICAgICB9LAogICAgICAgICJhY2NvdW50X251bWJlciI6ICI2MDg5NzE5IiwKICAgICAgICAidXNlciI6IHsKICAgICAgICAgICAgImZpcnN0X25hbWUiOiAiSW5zaWdodHMiLAogICAgICAgICAgICAiaXNfYWN0aXZlIjogdHJ1ZSwKICAgICAgICAgICAgImlzX2ludGVybmFsIjogZmFsc2UsCiAgICAgICAgICAgICJsYXN0X25hbWUiOiAiUUEiLAogICAgICAgICAgICAibG9jYWxlIjogImVuX1VTIiwKICAgICAgICAgICAgImlzX29yZ19hZG1pbiI6IHRydWUsCiAgICAgICAgICAgICJ1c2VybmFtZSI6ICJpbnNpZ2h0cy1xYSIsCiAgICAgICAgICAgICJlbWFpbCI6ICJqbmVlZGxlK3FhQHJlZGhhdC5jb20iLAogICAgICAgICAgICAidXNlcl9pZCI6ICI2MDg5NzE5IgogICAgICAgIH0sCiAgICAgICAgInR5cGUiOiAiVXNlciIKICAgIH0KfQ==") //nolint:lll
hasPermissionKessel(c)
assert.Equal(t, http.StatusOK, w.Code)
workspaces, found := c.Get(utils.KeyInventoryWorkspaces)
require.True(t, found)
workspaceIDs, ok := workspaces.([]string)
require.True(t, ok)
require.Greater(t, len(workspaceIDs), 0)
assert.Equal(t, "inventory-group-1", workspaceIDs[0])
}
func TestHasPermissionKessel_NoWorkspaces(t *testing.T) {
w := httptest.NewRecorder()
c, _ := gin.CreateTestContext(w)
c.Request = &http.Request{Method: http.MethodGet, Header: http.Header{}}
// Do not set any identity / workspaces so that hasPermissionKessel
// collects an empty workspace IDs slice and should abort as unauthorized.
hasPermissionKessel(c)
assert.Equal(t, http.StatusUnauthorized, w.Code)
_, found := c.Get(utils.KeyInventoryWorkspaces)
require.False(t, found, "inventory workspaces should not be set when authorization fails")
}
```
To compile these tests, ensure `manager/middlewares/kessel_test.go` imports `net/http/httptest` (and `net/http` if not already imported):
1. Add `httptest` to the import list:
<<<<<<< SEARCH
import (
"net/http"
"github.com/gin-gonic/gin"
=======
import (
"net/http"
"net/http/httptest"
"github.com/gin-gonic/gin"
>>>>>>> REPLACE
If the actual import block differs, adjust the import edits accordingly to include `net/http/httptest`.
</issue_to_address>
### Comment 3
<location path="listener/common_test.go" line_range="89-92" />
<code_context>
// assertSystemInventoryProfileMatchesHost checks host-derived system_inventory columns written by
// storeOrUpdateSysPlatform (must stay in sync on ON CONFLICT DO UPDATE, not only on first insert).
-// nolint: unparam
+// nolint: unparam,funlen
func assertSystemInventoryProfileMatchesHost(t *testing.T, inventoryID string, host *Host) {
t.Helper()
</code_context>
<issue_to_address>
**suggestion:** Test (and make helper robust for) hosts without any workspace groups
The helper currently assumes `host.Groups[0]` exists, but `updateSystemPlatform` now allows hosts with zero groups (nil `WorkspaceID`/`WorkspaceName`). Please guard against `len(host.Groups) == 0` (asserting `inv.WorkspaceID`/`WorkspaceName` are nil in that case), and add a test with `Groups: nil`/`[]` to confirm the values stay nil and no panic occurs.
Suggested implementation:
```golang
// assertSystemInventoryProfileMatchesHost checks host-derived system_inventory columns written by
// storeOrUpdateSysPlatform (must stay in sync on ON CONFLICT DO UPDATE, not only on first insert).
// nolint: unparam,funlen
func assertSystemInventoryProfileMatchesHost(t *testing.T, inventoryID string, host *Host) {
t.Helper()
var inv models.SystemInventory
assert.JSONEq(t, string(utils.MarshalNilToJSONB(host.Tags)), string(inv.Tags))
+ // A host may legitimately have no workspace groups; in that case the inventory workspace
+ // fields should remain nil and we must not index into host.Groups[0].
+ if len(host.Groups) == 0 {
+ assert.Nil(t, inv.WorkspaceID)
+ // NOTE: if inv.WorkspaceName is a *string / sql.NullString or similar, it should also be nil/invalid here.
+ // See additional_changes below for aligning this with the actual type.
+ } else if hostWorkspaceID := host.Groups[0].ID; hostWorkspaceID != "" {
+ require.NotNil(t, inv.WorkspaceID)
+ assert.Equal(t, hostWorkspaceID, inv.WorkspaceID.String())
+ } else {
```
1. In `assertSystemInventoryProfileMatchesHost`, you should also assert the correct behavior for `inv.WorkspaceName` in the `len(host.Groups) == 0` branch, matching its actual type:
- If it's a pointer: `assert.Nil(t, inv.WorkspaceName)`
- If it's an `sql.NullString`: `assert.False(t, inv.WorkspaceName.Valid)`
- Or equivalent for whatever type is used.
2. Anywhere else in this helper where `host.Groups[0]` is accessed (e.g., to derive workspace name), wrap that logic in a `len(host.Groups) > 0` guard in the same pattern.
3. Add tests in `listener/common_test.go` that exercise the helper with no groups, e.g.:
- One test with `host := &Host{Groups: nil, /* other required fields */}` that eventually calls `assertSystemInventoryProfileMatchesHost` and verifies no panic and that `inv.WorkspaceID`/`WorkspaceName` remain nil.
- Another test with `host := &Host{Groups: []*HostGroup{}, /* other required fields */}` doing the same.
These tests should mirror the existing tests that cover the non-empty-groups case, reusing whatever setup helpers you already have for inserting a `Host`, calling `storeOrUpdateSysPlatform`, and retrieving the resulting `models.SystemInventory`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MichaelMraka
approved these changes
May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Refactor workspace handling across the service to replace JSONB-based group/workspace storage with explicit workspace_id and workspace_name columns, and propagate the new workspace-based filtering through queries, middleware, models, and exports.
Enhancements:
Build: