Repository navigation
RHINENG-30223: scope Satellite hosts and separate ID sources - #2362
Open
dominikvagner wants to merge 3 commits into
Open
dominikvagner wants to merge 3 commits into
dominikvagner wants to merge 3 commits into
Conversation
The cyndi role creation is often failing in the local dev setup. This changes the script to only create it, if it doesn't exist, making the local dev setup less bothersome.
Owner IDs and subscription-manager IDs have different purposes. Treating them as interchangeable can resolve the wrong host or send the wrong consumer identity to Candlepin. Read each ID from its matching Inventory field. Use owner IDs for cert-auth lookup and subscription-manager IDs for Candlepin.
Satellite certificates can represent multiple hosts sharing an owner. Owner-only lookup cannot distinguish which host a request targets. Validate optional forwarded subscription-manager IDs for Satellite certificates and match them alongside the owner and account. Reject malformed IDs with 403 while preserving owner-only lookup when the header is absent. Other certificate types ignore the header.
Reviewer's GuideThe PR separates owner IDs from subscription-manager IDs across inventory ingestion, persistence, Candlepin calls, and template lookups, while adding Satellite-specific forwarded-identity scoping and validation; it also makes local database role creation safe to rerun. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="manager/controllers/template_subscribed_systems_update.go" line_range="73" />
<code_context>
+func systemIdentityQuery(c *gin.Context, tx *gorm.DB) *gorm.DB {
+ query := tx.Table("system_inventory").
+ Where("owner_id = ? AND rh_account_id = ?", c.GetString(utils.KeySystem), c.GetInt(utils.KeyAccount))
+ if forwardedID := c.GetString(utils.KeySubscriptionManagerID); forwardedID != "" {
+ query = query.Where("subscription_manager_id = ?", forwardedID)
</code_context>
<issue_to_address>
**Template changes target wrong host**
When a Satellite owner matches multiple hosts in the account and the request omits `X-Forwarded-Identity`, `systemIdentityQuery` omits the subscription-manager filter, and `getSubscribedSystem` uses `LIMIT 1` without ordering or checking for multiple matches. The handler can assign the template to an arbitrary host.
Require a forwarded identity, or reject requests when the owner-and-account query matches multiple hosts.
Also at `manager/controllers/template_subscribed_systems_update.go:74-75`.
</issue_to_address>
### Comment 2
<location path="listener/upload.go" line_range="400" />
<code_context>
OSMinor: &host.SystemProfile.OperatingSystem.Minor,
RhsmVersion: utils.EmptyToNil(&host.SystemProfile.Rhsm.Version),
- SubscriptionManagerID: host.SystemProfile.OwnerID,
+ SubscriptionManagerID: host.SubscriptionManagerID,
OwnerID: host.SystemProfile.OwnerID,
SapWorkload: host.SystemProfile.Workloads.Sap.SapSystem,
</code_context>
<issue_to_address>
**Older upload overwrites host identities**
When an older or redelivered host event is processed after a newer event for the same inventory ID, `updateSystemPlatform` writes the event's subscription-manager ID on every upsert, but its row lock only serializes writes and does not reject stale events. An older event processed last replaces the current ID, so later host lookup and Candlepin requests use stale identity data.
Reject stale identity updates using a reliable event version or timestamp before applying the upsert.
</issue_to_address>
### Comment 3
<location path="listener/upload.go" line_range="400" />
<code_context>
OSMinor: &host.SystemProfile.OperatingSystem.Minor,
RhsmVersion: utils.EmptyToNil(&host.SystemProfile.Rhsm.Version),
- SubscriptionManagerID: host.SystemProfile.OwnerID,
+ SubscriptionManagerID: host.SubscriptionManagerID,
OwnerID: host.SystemProfile.OwnerID,
SapWorkload: host.SystemProfile.Workloads.Sap.SapSystem,
</code_context>
<issue_to_address>
**Existing hosts retain owner IDs**
When a host already in the database has not been processed by the new listener since the ID source changed, the backfill job copies the old `subscription_manager_id` into `owner_id` but leaves the old value in `subscription_manager_id`; the new listener only writes the actual subscription-manager ID when it processes a host event. Until then, forwarded-ID lookups return 404 and Candlepin requests use the owner ID as the consumer ID.
Backfill or replay events to populate actual subscription-manager IDs before enabling the new lookup and Candlepin behavior, or make the rollout handle legacy values until they are refreshed.
Also at `manager/controllers/template_subscribed_systems_update.go:75`.
</issue_to_address>
### Comment 4
<location path="dev/create_inventory_hosts.sql" line_range="45" />
<code_context>
+ CREATE USER cyndi;
+ EXCEPTION WHEN DUPLICATE_OBJECT THEN
+ RAISE NOTICE 'cyndi already exists';
+END
+$$;
</code_context>
<issue_to_address>
**Local setup script fails**
When the local database setup executes this script, the `DO` block's `END` lacks a terminating semicolon, so PostgreSQL raises a PL/pgSQL syntax error and the script cannot create or skip the `cyndi` role.
Terminate `END` with a semicolon.
</issue_to_address>
### Comment 5
<location path="manager/controllers/template_subscribed_systems_update.go" line_range="73" />
<code_context>
+func systemIdentityQuery(c *gin.Context, tx *gorm.DB) *gorm.DB {
+ query := tx.Table("system_inventory").
+ Where("owner_id = ? AND rh_account_id = ?", c.GetString(utils.KeySystem), c.GetInt(utils.KeyAccount))
+ if forwardedID := c.GetString(utils.KeySubscriptionManagerID); forwardedID != "" {
+ query = query.Where("subscription_manager_id = ?", forwardedID)
</code_context>
<issue_to_address>
**Satellite lookups scan host partitions**
When a Satellite request reaches an account with a large host partition, the owner-and-account filter has no matching index, so each subscribed-system request scans the account’s `system_inventory` partition; this makes lookup latency grow with that account’s host count.
Add an index supporting the owner/account lookup and the optional subscription-manager ID filter, with a migration for existing databases.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and the system-certificate identity and optional forwarded UUID now determine which inventory host is resolved for template subscription updates. If that matching selects the wrong host, the template association persists; reverting stops future lookups but does not undo associations already written.
Blocking findings: manager/controllers/template_subscribed_systems_update.go:73, listener/upload.go:400, listener/upload.go:400, dev/create_inventory_hosts.sql:45, manager/controllers/template_subscribed_systems_update.go:73
| CREATE USER cyndi; | ||
| EXCEPTION WHEN DUPLICATE_OBJECT THEN | ||
| RAISE NOTICE 'cyndi already exists'; | ||
| END |
There was a problem hiding this comment.
🟡 Medium · Local setup script fails
When the local database setup executes this script, the DO block's END lacks a terminating semicolon, so PostgreSQL raises a PL/pgSQL syntax error and the script cannot create or skip the cyndi role.
Terminate END with a semicolon.
Prompt for AI agents
In `dev/create_inventory_hosts.sql` at line 45:
**Local setup script fails**
When the local database setup executes this script, the `DO` block's `END` lacks a terminating semicolon, so PostgreSQL raises a PL/pgSQL syntax error and the script cannot create or skip the `cyndi` role.
Terminate `END` with a semicolon.|
|
||
| func systemIdentityQuery(c *gin.Context, tx *gorm.DB) *gorm.DB { | ||
| query := tx.Table("system_inventory"). | ||
| Where("owner_id = ? AND rh_account_id = ?", c.GetString(utils.KeySystem), c.GetInt(utils.KeyAccount)) |
There was a problem hiding this comment.
🟡 Medium · Satellite lookups scan host partitions
When a Satellite request reaches an account with a large host partition, the owner-and-account filter has no matching index, so each subscribed-system request scans the account’s system_inventory partition; this makes lookup latency grow with that account’s host count.
Add an index supporting the owner/account lookup and the optional subscription-manager ID filter, with a migration for existing databases.
Prompt for AI agents
In `manager/controllers/template_subscribed_systems_update.go` at line 73:
**Satellite lookups scan host partitions**
When a Satellite request reaches an account with a large host partition, the owner-and-account filter has no matching index, so each subscribed-system request scans the account’s `system_inventory` partition; this makes lookup latency grow with that account’s host count.
Add an index supporting the owner/account lookup and the optional subscription-manager ID filter, with a migration for existing databases.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2362 +/- ##
==========================================
+ Coverage 58.89% 58.98% +0.09%
==========================================
Files 151 151
Lines 9706 9730 +24
==========================================
+ Hits 5716 5739 +23
- Misses 3386 3387 +1
Partials 604 604
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MichaelMraka
approved these changes
Oct 14, 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
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.
Summary
This PR makes follow-up changes to #2356.
Switches the code to use subscription manager IDs and owner IDs correctly.
And changes the auth to work correctly for satellite systems using cert auth.
Also makes one small tweak to the local DB setup to not fail on existing roles.
This PR will need to be merged after the owner ID backfill job will be finished
and the prod release for this will also need to be done in order.
RHINENG-30223: separate host identity sources
RHINENG-30223: scope Satellite host requests
Summary by Sourcery
Correct host identity handling and scope Satellite-authenticated requests to the intended host.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: