From 7a2293ebeed9755c63bb8e3ea042525466876ca3 Mon Sep 17 00:00:00 2001 From: rverdile Date: Tue, 30 Jun 2026 14:02:18 -0400 Subject: [PATCH] RHINENG-26121: update manager to use account_advisory table --- dev/test_data.sql | 2 + manager/config/config.go | 2 + manager/controllers/advisories.go | 98 +++++++++++-- manager/controllers/advisories_export.go | 13 +- manager/controllers/advisories_test.go | 169 +++++++++++++++++++++++ 5 files changed, 267 insertions(+), 17 deletions(-) diff --git a/dev/test_data.sql b/dev/test_data.sql index 386e7d17a..363ae1a95 100644 --- a/dev/test_data.sql +++ b/dev/test_data.sql @@ -7,6 +7,7 @@ DELETE FROM deleted_system; DELETE FROM repo; DELETE FROM timestamp_kv; DELETE FROM advisory_account_data; +DELETE FROM account_advisory; DELETE FROM package_account_data; DELETE FROM package; DELETE FROM package_name; @@ -203,6 +204,7 @@ INSERT INTO timestamp_kv (name, value) VALUES ('last_eval_repo_based', '2018-04-05T01:23:45+02:00'); SELECT refresh_all_cached_counts(); +SELECT refresh_account_advisory_caches_multi(NULL, NULL); ALTER TABLE advisory_metadata ALTER COLUMN id RESTART WITH 100; ALTER TABLE system_inventory ALTER COLUMN id RESTART WITH 100; diff --git a/manager/config/config.go b/manager/config/config.go index 1afc80031..ca019cd2e 100644 --- a/manager/config/config.go +++ b/manager/config/config.go @@ -26,4 +26,6 @@ var ( // Expose templates API (feature flag) EnableTemplates = utils.PodConfig.GetBool("templates_api", true) + // Use precomputed per-workspace advisory counts from account_advisory table + EnableAccountAdvisoryReadPath = utils.PodConfig.GetBool("account_advisory", true) ) diff --git a/manager/controllers/advisories.go b/manager/controllers/advisories.go index ba8e020ab..312315234 100644 --- a/manager/controllers/advisories.go +++ b/manager/controllers/advisories.go @@ -89,14 +89,9 @@ func advisoriesCommon(c *gin.Context) (*gorm.DB, *ListMeta, []string, error) { return nil, nil, nil, err } - // TODO: fix below; the condition is always true since moving from groups to workspaces, - // there will always be at least root workspace - if config.DisableCachedCounts || HasInventoryFilter(filters) || len(workspaceIDs) != 0 { - middlewares.AdvisoryAccountDataCnt.WithLabelValues("miss").Inc() - query = buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs) - } else { - middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc() - query = buildQueryAdvisories(db, account) + query, err = resolveAdvisoriesQuery(db, account, workspaceIDs, filters) + if err != nil { + return nil, nil, nil, err } query, meta, params, err := ListCommon(query, c, filters, AdvisoriesOpts) @@ -218,6 +213,36 @@ func buildQueryAdvisories(db *gorm.DB, account int) *gorm.DB { return query } +func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, filters Filters) (*gorm.DB, error) { + if config.EnableAccountAdvisoryReadPath && !hasNonGroupInventoryFilter(filters) { + effectiveWorkspaceIDs := workspaceIDs + if groupNames := getGroupNameFilterValues(filters); len(groupNames) > 0 { + var err error + effectiveWorkspaceIDs, err = resolveWorkspaceNameToIDs(db, account, groupNames, workspaceIDs) + if err != nil { + return nil, err + } + } + middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc() + if len(effectiveWorkspaceIDs) == 0 { + query := buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs) + return query.Where("FALSE"), nil + } + return buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs), nil + } + // TODO: fix below; + // the condition is always true since moving from groups to workspaces, + // there will always be at least root workspace + // leaving until RBAC cannot be re-enabled + if !config.EnableAccountAdvisoryReadPath && !config.DisableCachedCounts && + !HasInventoryFilter(filters) && len(workspaceIDs) == 0 { + middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc() + return buildQueryAdvisories(db, account), nil + } + middlewares.AdvisoryAccountDataCnt.WithLabelValues("miss").Inc() + return buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs), nil +} + func buildAdvisoryAccountDataQuery(db *gorm.DB, account int, workspaceIDs []string) *gorm.DB { query := database.SystemAdvisories(db, account, workspaceIDs). Select(`sa.advisory_id, si.rh_account_id as rh_account_id, @@ -242,6 +267,63 @@ func buildQueryAdvisoriesTagged(db *gorm.DB, filters map[string]FilterData, acco return query } +func hasNonGroupInventoryFilter(filters Filters) bool { + for key, data := range filters { + if data.Type == TagFilter { + return true + } + if data.Type == InventoryFilter && key != "group_name" { + return true + } + } + return false +} + +func getGroupNameFilterValues(filters Filters) []string { + if data, ok := filters["group_name"]; ok && data.Type == InventoryFilter { + return data.Values + } + return nil +} + +func resolveWorkspaceNameToIDs(db *gorm.DB, + accountID int, + names []string, + allowedWorkspaceIDs []string) ([]string, error) { + var ids []string + query := db.Table("system_inventory si"). + Distinct(). + Where("si.rh_account_id = ?", accountID). + Where("si.workspace_name IN (?)", names) + if len(allowedWorkspaceIDs) > 0 { + query = query.Where("si.workspace_id IN (?)", allowedWorkspaceIDs) + } + err := query.Pluck("workspace_id", &ids).Error + return ids, err +} + +func buildAccountAdvisorySubquery(db *gorm.DB, account int, workspaceIDs []string) *gorm.DB { + query := db.Table("account_advisory aa_inner"). + Select(`aa_inner.advisory_id, + aa_inner.rh_account_id, + SUM(aa_inner.systems_installable) as systems_installable, + SUM(aa_inner.systems_applicable) as systems_applicable`). + Where("aa_inner.rh_account_id = ?", account) + if len(workspaceIDs) > 0 { + query = query.Where("aa_inner.workspace_id IN (?)", workspaceIDs) + } + return query.Group("aa_inner.advisory_id, aa_inner.rh_account_id"). + Having("SUM(aa_inner.systems_applicable) > 0") +} + +func buildQueryAdvisoriesFromAccountAdvisory(db *gorm.DB, account int, workspaceIDs []string) *gorm.DB { + subq := buildAccountAdvisorySubquery(db, account, workspaceIDs) + return database.AdvisoryMetadata(db). + Select(AdvisoriesSelect). + Joins("JOIN (?) aad ON am.id = aad.advisory_id", subq). + Joins("LEFT JOIN advisory_severity sev ON am.severity_id = sev.id") +} + func buildAdvisoriesData(advisories []AdvisoriesDBLookup) ([]AdvisoryItem, int, map[string]int) { var total int subtotals := map[string]int{ diff --git a/manager/controllers/advisories_export.go b/manager/controllers/advisories_export.go index a9a7af66a..3d3da055d 100644 --- a/manager/controllers/advisories_export.go +++ b/manager/controllers/advisories_export.go @@ -2,11 +2,9 @@ package controllers import ( "app/base/utils" - "app/manager/config" "app/manager/middlewares" "github.com/gin-gonic/gin" - "gorm.io/gorm" ) // nolint:lll @@ -37,14 +35,11 @@ func AdvisoriesExportHandler(c *gin.Context) { return } db := middlewares.DBFromContext(c) - var query *gorm.DB - // TODO: fix below; the condition is always true since moving from groups to workspaces, - // there will always be at least root workspace - if config.DisableCachedCounts || HasInventoryFilter(filters) || len(workspaceIDs) != 0 { - query = buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs) - } else { - query = buildQueryAdvisories(db, account) + query, err := resolveAdvisoriesQuery(db, account, workspaceIDs, filters) + if err != nil { + utils.LogAndRespError(c, err, "db error") + return } var advisories []AdvisoriesDBLookup diff --git a/manager/controllers/advisories_test.go b/manager/controllers/advisories_test.go index 4c10882b0..127cb6f48 100644 --- a/manager/controllers/advisories_test.go +++ b/manager/controllers/advisories_test.go @@ -2,7 +2,9 @@ package controllers import ( "app/base/core" + "app/base/database" "app/base/utils" + "app/manager/config" "fmt" "net/http" "testing" @@ -38,6 +40,15 @@ func testAdvisoriesIDs(t *testing.T, url string) IDsPlainResponse { return output } +func TestAdvisoriesLegacyPath(t *testing.T) { + core.SetupTest(t) + config.EnableAccountAdvisoryReadPath = false + defer func() { config.EnableAccountAdvisoryReadPath = true }() + + output := testAdvisories(t, "/") + assert.Equal(t, 12, len(output.Data)) +} + func TestAdvisoriesDefault(t *testing.T) { output := testAdvisories(t, "/") assert.Equal(t, 12, len(output.Data)) @@ -386,3 +397,161 @@ func TestAdvisoryMetadataSums(t *testing.T) { assert.Equal(t, bugfix, st["bugfix"]) assert.Equal(t, security, st["security"]) } + +func TestAdvisoriesGroupNameSingle(t *testing.T) { + output := testAdvisories(t, "/?sort=id&filter[group_name]=group1") + // group1 = workspace 00000000-0000-0000-0000-000000000001 + // Systems 1-6 in group1, advisories 1-8 from system 1, advisory 1 from systems 2-6 + assert.Equal(t, 8, len(output.Data)) + assert.Equal(t, "RH-1", output.Data[0].ID) + assert.Equal(t, 4, output.Data[0].Attributes.InstallableSystems) + assert.Equal(t, 6, output.Data[0].Attributes.ApplicableSystems) +} + +func TestAdvisoriesGroupNameGroup2(t *testing.T) { + output := testAdvisories(t, "/?sort=id&filter[group_name]=group2") + // group2 = workspace 00000000-0000-0000-0000-000000000002 + // System 8 in group2 (system 7 has no last_evaluation), advisories 10-13 + assert.Equal(t, 4, len(output.Data)) + assert.Equal(t, "CUSTOM-12", output.Data[0].ID) + assert.Equal(t, "CUSTOM-13", output.Data[1].ID) + assert.Equal(t, "UNSPEC-10", output.Data[2].ID) + assert.Equal(t, "UNSPEC-11", output.Data[3].ID) +} + +func TestAdvisoriesGroupNameMultiple(t *testing.T) { + output := testAdvisories(t, "/?filter[group_name]=group1,group2") + // group1 + group2 = all advisories for account 1 + assert.Equal(t, 12, len(output.Data)) +} + +func TestAdvisoriesGroupNameNoMatch(t *testing.T) { + output := testAdvisories(t, "/?filter[group_name]=nonexistent") + assert.Equal(t, 0, len(output.Data)) + assert.Equal(t, 0, output.Meta.TotalItems) +} + +func TestAdvisoriesIDsGroupName(t *testing.T) { + output := testAdvisoriesIDs(t, "/?sort=id&filter[group_name]=group2") + assert.Equal(t, 4, len(output.IDs)) +} + +func TestAdvisoriesExportGroupName(t *testing.T) { + core.SetupTest(t) + w := CreateRequest("GET", "/?filter[group_name]=group2", nil, "application/json", AdvisoriesExportHandler) + + var output []AdvisoriesDBLookup + CheckResponse(t, w, http.StatusOK, &output) + assert.Equal(t, 4, len(output)) +} + +func TestHasNonGroupInventoryFilter(t *testing.T) { + tests := []struct { + name string + filters Filters + expected bool + }{ + { + name: "empty filters", + filters: Filters{}, + expected: false, + }, + { + name: "only group_name", + filters: Filters{ + "group_name": {Type: InventoryFilter, Operator: "eq", Values: []string{"group1"}}, + }, + expected: false, + }, + { + name: "group_name and column filter", + filters: Filters{ + "group_name": {Type: InventoryFilter, Operator: "eq", Values: []string{"group1"}}, + "applicable_systems": {Type: ColumnFilter, Operator: "gt", Values: []string{"0"}}, + }, + expected: false, + }, + { + name: "tag filter", + filters: Filters{ + "ns1/k1": {Type: TagFilter, Operator: "eq", Values: []string{"val1"}}, + }, + expected: true, + }, + { + name: "sap_system filter", + filters: Filters{ + "system_profile][sap_system": {Type: InventoryFilter, Operator: "eq", Values: []string{"true"}}, + }, + expected: true, + }, + { + name: "group_name and tag", + filters: Filters{ + "group_name": {Type: InventoryFilter, Operator: "eq", Values: []string{"group1"}}, + "ns1/k1": {Type: TagFilter, Operator: "eq", Values: []string{"val1"}}, + }, + expected: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, hasNonGroupInventoryFilter(tc.filters)) + }) + } +} + +func TestGetGroupNameFilterValues(t *testing.T) { + filters := Filters{ + "group_name": {Type: InventoryFilter, Operator: "eq", Values: []string{"group1", "group2"}}, + } + assert.Equal(t, []string{"group1", "group2"}, getGroupNameFilterValues(filters)) + + assert.Nil(t, getGroupNameFilterValues(Filters{})) + + filtersColumnOnly := Filters{ + "group_name": {Type: ColumnFilter, Operator: "eq", Values: []string{"group1"}}, + } + assert.Nil(t, getGroupNameFilterValues(filtersColumnOnly)) +} + +func TestAdvisoriesTagsFallbackWithAccountAdvisory(t *testing.T) { + output := testAdvisories(t, "/?sort=id&tags=ns1/k2=val2") + assert.Equal(t, 8, len(output.Data)) + assert.Equal(t, 1, output.Data[0].Attributes.InstallableSystems) + assert.Equal(t, 2, output.Data[0].Attributes.ApplicableSystems) +} + +func TestResolveWorkspaceNameToIDs(t *testing.T) { + core.SetupTest(t) + + allowedWorkspaces := []string{ + "00000000-0000-0000-0000-000000000001", + "00000000-0000-0000-0000-000000000002", + "00000000-0000-0000-0000-999999999999", + } + + ids, err := resolveWorkspaceNameToIDs(database.DB, 1, []string{"group1"}, allowedWorkspaces) + assert.Nil(t, err) + assert.Equal(t, 1, len(ids)) + assert.Equal(t, "00000000-0000-0000-0000-000000000001", ids[0]) + + ids, err = resolveWorkspaceNameToIDs(database.DB, 1, []string{"group2"}, allowedWorkspaces) + assert.Nil(t, err) + assert.Equal(t, 1, len(ids)) + assert.Equal(t, "00000000-0000-0000-0000-000000000002", ids[0]) + + ids, err = resolveWorkspaceNameToIDs(database.DB, 1, []string{"group1", "group2"}, allowedWorkspaces) + assert.Nil(t, err) + assert.Equal(t, 2, len(ids)) + + ids, err = resolveWorkspaceNameToIDs(database.DB, 1, []string{"nonexistent"}, allowedWorkspaces) + assert.Nil(t, err) + assert.Equal(t, 0, len(ids)) + + ids, err = resolveWorkspaceNameToIDs(database.DB, + 1, []string{"group1"}, []string{"00000000-0000-0000-0000-000000000002"}) + assert.Nil(t, err) + assert.Equal(t, 0, len(ids)) +}