fix(kessel): enhance checks with several workspaces#2654
Conversation
Reviewer's GuideImplements an optimized RBAC group filtering path for Kessel by using a temporary table for large group lists, configurable via a new threshold setting, while preserving the existing behavior and adding a safe fallback path. Sequence diagram for optimized Kessel RBAC group filtering in system policysequenceDiagram
actor Request
participant SystemPolicyScope as V2_SystemPolicy_Scope
participant KesselRbac as KesselRbac
participant ARScope as ActiveRecord_Scope
participant DB as Database
Request ->> SystemPolicyScope: resolve_regular
SystemPolicyScope ->> SystemPolicyScope: validate org_id and groups
alt groups equals Rbac_ANY
SystemPolicyScope ->> ARScope: base_scope
SystemPolicyScope -->> Request: base_scope
else groups not Rbac_ANY
SystemPolicyScope ->> ARScope: base_scope
SystemPolicyScope ->> KesselRbac: enabled
KesselRbac -->> SystemPolicyScope: boolean
alt KesselRbac enabled and groups length > groups_temp_table_threshold
SystemPolicyScope ->> KesselRbac: groups_temp_table_threshold
KesselRbac -->> SystemPolicyScope: threshold
SystemPolicyScope ->> SystemPolicyScope: with_groups_via_temp_table(base_scope, groups)
SystemPolicyScope ->> DB: CREATE TEMP TABLE temp_user_groups_...
SystemPolicyScope ->> DB: INSERT group ids into temp table
SystemPolicyScope ->> DB: SELECT with EXISTS using temp table
DB -->> SystemPolicyScope: filtered records
SystemPolicyScope -->> Request: optimized filtered_scope
note over SystemPolicyScope,DB: On ActiveRecord_StatementInvalid, log error and fallback to base_scope.with_groups(groups)
else KesselRbac disabled or groups length <= threshold
SystemPolicyScope ->> ARScope: base_scope.with_groups(groups)
SystemPolicyScope -->> Request: filtered_scope
end
end
Updated class diagram for KesselRbac and system policy scopeclassDiagram
class V2_SystemPolicy_Scope {
- user
- scope
+ resolve_regular()
+ resolve_cert_auth()
+ base_scope()
+ with_groups_via_temp_table(filtered_scope, groups)
}
class KesselRbac {
+ enabled()
+ groups_temp_table_threshold()
+ client()
+ build_client()
}
class SettingsKessel {
+ enabled: boolean
+ principal_domain: string
+ oauth2_token_url: string
+ client_id: string
+ client_secret: string
+ oidc_issuer: string
+ insecure: boolean
+ groups_temp_table_threshold: integer
}
class KesselInitializer {
+ load_from_env()
+ set_defaults()
}
V2_SystemPolicy_Scope ..> KesselRbac : uses
V2_SystemPolicy_Scope ..> SettingsKessel : reads
KesselRbac ..> SettingsKessel : reads
KesselInitializer ..> SettingsKessel : configures
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
josejulio
force-pushed
the
rework-group-query
branch
2 times, most recently
from
77dfc19 to
5991abc
Compare
|
@josejulio what is status of this PR ? |
It needs testing - the idea is there, the original query took long in the planning phase and too little on the execution phase. The hope is that this would alleviate the planning phase by using the temporal table. If it succeeds it might need to port the raw queries to the framework it is being used (if relevant) |
There was a problem hiding this comment.
Potential suggestion to the issue as reviewed by Claude. I personally don't have a strong opinion on this vs the proposed option (especially to test) and would defer to the service SMEs on preference, but wanted to drop it in here anyway:
Consider EXISTS + unnest instead of temp tables
The diagnosis here seems correct: large group lists cause query planning overhead with the current @> ANY(CAST('{...}' AS jsonb[])) pattern, and this can absolutely contribute to pod resource pressure under concurrent load. Nice catch.
The temp table approach may introduce more complexity than needed. A few notes:
Bug: find_in_temp_table arity mismatch
with_groups_via_temp_table calls find_in_temp_table(conn, filtered_scope, table_name) (3 args), but the method is defined as def find_in_temp_table(scope, table_name) (2 params). This will raise ArgumentError at runtime, meaning every request with 50+ groups hits the rescue and falls back to the unoptimized with_groups — making the optimization effectively dead code.
Simpler alternative: EXISTS + unnest
Instead of 3 DB round-trips (CREATE TEMP TABLE, INSERT, SELECT) and connection-level state management, the same result can be achieved in a single query:
WHERE EXISTS (
SELECT 1 FROM unnest(ARRAY['uuid-1','uuid-2',...,'uuid-N']::text[]) AS g(val)
WHERE inventory.hosts.groups @> jsonb_build_array(jsonb_build_object('id', g.val))
)This gives you:
- Single
@>per row — GIN-index friendly (same benefit as the temp table approach) - 1 DB round-trip instead of 3
- No connection-level state — no temp table lifecycle, no cleanup, no concurrency concerns
- No rescue/fallback needed — it's just a WHERE clause
- ~20 lines instead of ~50
Here's what the scope could look like on V2::System:
scope :with_groups_unnest, lambda { |groups, key = :id|
flat = groups.flatten
quoted_groups = flat.map { |g| connection.quote(g) }.join(',')
quoted_key = connection.quote(key.to_s)
exists_sql = <<~SQL.squish
EXISTS (
SELECT 1 FROM unnest(ARRAY[#{quoted_groups}]::text[]) AS g(val)
WHERE #{quoted_table_name}.groups @> jsonb_build_array(jsonb_build_object(#{quoted_key}, g.val))
)
SQL
ungrouped = arel_table[:groups].eq(AN::Quoted.new('[]'))
where(groups.include?([]) ? Arel::Nodes::Grouping.new(Arel.sql(exists_sql)).or(ungrouped) : Arel.sql(exists_sql))
}And the policy becomes:
def resolve_regular
return scope.none if user.org_id.blank? || groups.blank?
return base_scope if groups == Rbac::ANY
return base_scope.with_groups_unnest(groups) if use_unnest?(groups)
base_scope.with_groups(groups)
end
def use_unnest?(groups)
KesselRbac.enabled? && groups.length > KesselRbac.groups_unnest_threshold
endThe config/threshold parts of the PR are fine as-is (just rename to groups_unnest_threshold or similar).
Summary
| Temp table (current PR) | EXISTS + unnest (proposed) | |
|---|---|---|
| DB round-trips | 3 (CREATE, INSERT, SELECT) | 1 |
| Connection-level state | Yes | None |
| GIN index friendly | ✓ | ✓ |
| Error handling needed | Yes (rescue + fallback) | No |
| Lines of code | ~50 | ~20 |
| Bugs | arity mismatch on find_in_temp_table |
N/A |
josejulio
force-pushed
the
rework-group-query
branch
from
5991abc to
36c5fe5
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3 participants
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Optimize Kessel RBAC group-based system scoping for large group lists using a temporary-table approach and make the threshold configurable.
Enhancements: