Skip to content

fix: proper construction of SA groups for tier lookup #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jland-redhat merged 1 commit into from
Oct 3, 2025
Merged

fix: proper construction of SA groups for tier lookup #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maas-api/internal/tier/mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func (m *Mapper) loadTierConfig(ctx context.Context) ([]Tier, error) {

for i := range tiers {
tier := &tiers[i]
tier.Groups = append(tier.Groups, fmt.Sprintf("system:serviceaccount:%s", m.projectedNsName(tier)))
tier.Groups = append(tier.Groups, fmt.Sprintf("system:serviceaccounts:%s", m.projectedNsName(tier)))
}

return tiers, nil
Expand Down
14 changes: 13 additions & 1 deletion maas-api/internal/tier/mapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,16 @@ func TestMapper_GetTierForGroups(t *testing.T) {
},
{
name: "inferred SA group - free tier",
groups: []string{"system:serviceaccount:test-tenant-tier-free"},
groups: []string{"system:serviceaccounts:test-tenant-tier-free"},
expectedTier: "free",
description: "User belongs to only free tier group",
},
{
name: "inferred SA group - premium tier",
groups: []string{"system:serviceaccounts:test-tenant-tier-premium"},
expectedTier: "premium",
description: "User belongs to only premium tier group",
},
{
name: "single group - premium tier",
groups: []string{"premium-users"},
Expand Down Expand Up @@ -87,6 +93,12 @@ func TestMapper_GetTierForGroups(t *testing.T) {
expectedTier: "developer",
description: "User belongs to both premium and developer - developer has higher level (15 > 10)",
},
{
name: "multiple groups - service account groups",
groups: []string{"system:serviceaccounts", "system:serviceaccounts:test-tenant-tier-premium", "system:authenticated"},
expectedTier: "premium",
description: "User belongs to both premium and developer - developer has higher level (15 > 10)",
},
Comment on lines +96 to +101
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update the test description to match the test logic.

The test case correctly verifies that premium tier wins among mixed groups including service accounts. However, the description still mentions "developer" tier, which doesn't match the test logic.

Apply this diff to fix the description:

 		{
 			name:         "multiple groups - service account groups",
 			groups:       []string{"system:serviceaccounts", "system:serviceaccounts:test-tenant-tier-premium", "system:authenticated"},
 			expectedTier: "premium",
-			description:  "User belongs to both premium and developer - developer has higher level (15 > 10)",
+			description:  "Service account with premium namespace group should resolve to premium tier",
 		},
🤖 Prompt for AI Agents 
In maas-api/internal/tier/mapper_test.go around lines 96 to 101, the test case
description incorrectly mentions "developer" even though the test data checks
that the "premium" tier wins among mixed service account groups; update the
description string to accurately state that premium wins (e.g., "User belongs to
multiple groups including premium - premium has higher level and should win") so
the description matches the test logic.

{
name: "three groups - enterprise wins",
groups: []string{"free-users", "premium-users", "enterprise-users"},
Expand Down