diff --git a/pkg/rbac/compiled.go b/pkg/rbac/compiled.go new file mode 100644 index 00000000..6ca424f5 --- /dev/null +++ b/pkg/rbac/compiled.go @@ -0,0 +1,112 @@ +package rbac + +import ( + "regexp" + "strings" + + "github.com/zxh326/kite/pkg/common" + "k8s.io/klog/v2" +) + +// compiledPattern holds a pre-compiled representation of a single RBAC pattern. +// Patterns are compiled once during role sync and reused for every access check, +// eliminating regexp.Compile from the hot path entirely. +type compiledPattern struct { + raw string // original pattern string + negate bool // true if prefixed with "!" + wildcard bool // true if pattern is "*" + literal string // for exact string comparison (negation target or plain value) + re *regexp.Regexp // non-nil only when the pattern contains regex metacharacters +} + +// compiledRole wraps a common.Role with pre-compiled patterns for each field. +type compiledRole struct { + common.Role + clusters []compiledPattern + namespaces []compiledPattern + resources []compiledPattern + verbs []compiledPattern +} + +// regexpMetaDetector matches any regex metacharacter. If a pattern contains none +// of these, it is a literal string and regex matching can be skipped entirely (Solution D). +var regexpMetaDetector = regexp.MustCompile(`[\\.*+?^${}()|[\]]`) + +// hasRegexMeta returns true if the pattern contains regex metacharacters. +func hasRegexMeta(p string) bool { + return regexpMetaDetector.MatchString(p) +} + +// compilePatterns converts a slice of raw pattern strings into pre-compiled patterns. +// Called once per sync cycle (every ~60s), not on the hot path. +func compilePatterns(patterns []string) []compiledPattern { + out := make([]compiledPattern, 0, len(patterns)) + for _, p := range patterns { + cp := compiledPattern{raw: p} + + switch { + case len(p) > 1 && strings.HasPrefix(p, "!"): + // Negation pattern: "!kube-system" + cp.negate = true + cp.literal = p[1:] + + case p == "*": + // Wildcard: matches everything + cp.wildcard = true + + default: + // Store literal for exact comparison (always attempted first) + cp.literal = p + + // Only compile regex if pattern has metacharacters (Solution D) + if hasRegexMeta(p) { + re, err := regexp.Compile(p) + if err != nil { + klog.Errorf("rbac: invalid regex pattern %q: %v", p, err) + // Keep as literal-only (will still match via == check) + } else { + cp.re = re + } + } + } + + out = append(out, cp) + } + return out +} + +// compileRole converts a common.Role into a compiledRole with pre-compiled patterns. +func compileRole(r common.Role) compiledRole { + return compiledRole{ + Role: r, + clusters: compilePatterns(r.Clusters), + namespaces: compilePatterns(r.Namespaces), + resources: compilePatterns(r.Resources), + verbs: compilePatterns(r.Verbs), + } +} + +// matchCompiled evaluates pre-compiled patterns against a value. +// Zero allocations, zero regexp.Compile calls on the hot path. +func matchCompiled(patterns []compiledPattern, val string) bool { + for i := range patterns { + p := &patterns[i] + + if p.negate { + if p.literal == val { + return false + } + continue + } + + if p.wildcard || p.literal == val { + return true + } + + // Only invoke regex if a compiled regexp exists (pattern had metacharacters) + if p.re != nil && p.re.MatchString(val) { + return true + } + } + return false +} diff --git a/pkg/rbac/compiled_test.go b/pkg/rbac/compiled_test.go new file mode 100644 index 00000000..d6a51b0e --- /dev/null +++ b/pkg/rbac/compiled_test.go @@ -0,0 +1,201 @@ +package rbac + +import ( + "testing" + + "github.com/zxh326/kite/pkg/common" + "github.com/zxh326/kite/pkg/model" +) + +// --- Unit tests for compilePatterns / matchCompiled --- + +func TestMatchCompiledWildcard(t *testing.T) { + patterns := compilePatterns([]string{"*"}) + if !matchCompiled(patterns, "anything") { + t.Error("wildcard should match any value") + } +} + +func TestMatchCompiledLiteralExact(t *testing.T) { + patterns := compilePatterns([]string{"pods", "services"}) + if !matchCompiled(patterns, "pods") { + t.Error("literal pattern should match exact value") + } + if !matchCompiled(patterns, "services") { + t.Error("literal pattern should match exact value") + } + if matchCompiled(patterns, "deployments") { + t.Error("literal pattern should not match different value") + } +} + +func TestMatchCompiledRegex(t *testing.T) { + patterns := compilePatterns([]string{"dev.*"}) + if !matchCompiled(patterns, "dev-cluster") { + t.Error("regex pattern should match") + } + if !matchCompiled(patterns, "dev") { + t.Error("regex pattern should match exact prefix") + } + if matchCompiled(patterns, "prod-cluster") { + t.Error("regex pattern should not match non-matching value") + } +} + +func TestMatchCompiledNegation(t *testing.T) { + patterns := compilePatterns([]string{"!kube-system", "*"}) + if !matchCompiled(patterns, "default") { + t.Error("negation+wildcard should match non-negated value") + } + if matchCompiled(patterns, "kube-system") { + t.Error("negation should block exact match") + } +} + +func TestMatchCompiledEmpty(t *testing.T) { + patterns := compilePatterns([]string{}) + if matchCompiled(patterns, "anything") { + t.Error("empty patterns should match nothing") + } +} + +func TestMatchCompiledInvalidRegex(t *testing.T) { + // Invalid regex should be treated as literal-only; should not panic. + patterns := compilePatterns([]string{"[invalid"}) + if matchCompiled(patterns, "anything") { + t.Error("invalid regex pattern should not match arbitrary values") + } + // But should still match the literal string itself + if !matchCompiled(patterns, "[invalid") { + t.Error("invalid regex should still match as literal") + } +} + +func TestMatchCompiledLiteralSkipsRegex(t *testing.T) { + // Pattern "pods" has no metacharacters → re should be nil (Solution D) + patterns := compilePatterns([]string{"pods"}) + if len(patterns) != 1 { + t.Fatalf("expected 1 pattern, got %d", len(patterns)) + } + if patterns[0].re != nil { + t.Error("literal pattern 'pods' should not have a compiled regexp (Solution D)") + } + if !matchCompiled(patterns, "pods") { + t.Error("literal pattern should still match via == comparison") + } +} + +func TestMatchCompiledRegexHasMeta(t *testing.T) { + patterns := compilePatterns([]string{"dev-.*"}) + if len(patterns) != 1 { + t.Fatalf("expected 1 pattern, got %d", len(patterns)) + } + if patterns[0].re == nil { + t.Error("pattern 'dev-.*' has metacharacters and should have compiled regexp") + } +} + +// --- Integration: CanAccessCluster / CanAccessNamespace with compiled roles --- + +func TestCanAccessClusterCompiled(t *testing.T) { + setTestRBACConfig( + []common.Role{{ + Name: "dev-access", + Clusters: []string{"dev-.*"}, + Namespaces: []string{"*"}, + Resources: []string{"*"}, + Verbs: []string{"*"}, + }}, + []common.RoleMapping{{Name: "dev-access", Users: []string{"alice"}}}, + ) + + if !CanAccessCluster(model.User{Username: "alice"}, "dev-east") { + t.Error("should match dev-east via regex") + } + if CanAccessCluster(model.User{Username: "alice"}, "prod-west") { + t.Error("should NOT match prod-west") + } + if CanAccessCluster(model.User{Username: "bob"}, "dev-east") { + t.Error("bob should have no roles") + } +} + +func TestCanAccessNamespaceCompiled(t *testing.T) { + setTestRBACConfig( + []common.Role{{ + Name: "ns-role", + Clusters: []string{"*"}, + Namespaces: []string{"!kube-system", "team-.*"}, + Resources: []string{"*"}, + Verbs: []string{"*"}, + }}, + []common.RoleMapping{{Name: "ns-role", Users: []string{"*"}}}, + ) + + if !CanAccessNamespace(model.User{Username: "anyone"}, "any-cluster", "team-alpha") { + t.Error("should match team-alpha via regex") + } + if CanAccessNamespace(model.User{Username: "anyone"}, "any-cluster", "kube-system") { + t.Error("kube-system should be negated") + } + if CanAccessNamespace(model.User{Username: "anyone"}, "any-cluster", "default") { + t.Error("default does not match team-.* pattern") + } +} + +// --- Benchmarks: old sequential compile vs pre-compiled --- + +func BenchmarkMatchCompiled(b *testing.B) { + patterns := compilePatterns([]string{"dev-.*", "staging-.*", "!kube-system", "prod"}) + b.ResetTimer() + for b.Loop() { + matchCompiled(patterns, "dev-east") + } +} + +func BenchmarkMatchCompiledLiteral(b *testing.B) { + patterns := compilePatterns([]string{"get", "create", "update", "delete"}) + b.ResetTimer() + for b.Loop() { + matchCompiled(patterns, "update") + } +} + +func BenchmarkMatchCompiledWildcard(b *testing.B) { + patterns := compilePatterns([]string{"*"}) + b.ResetTimer() + for b.Loop() { + matchCompiled(patterns, "anything") + } +} + +func BenchmarkCanAccessFullCompiled(b *testing.B) { + setTestRBACConfig( + []common.Role{ + { + Name: "dev", + Clusters: []string{"dev-.*"}, + Namespaces: []string{"!kube-system", "team-.*"}, + Resources: []string{"pods", "deployments", "services"}, + Verbs: []string{"get", "create", "update"}, + }, + { + Name: "admin", + Clusters: []string{"*"}, + Namespaces: []string{"*"}, + Resources: []string{"*"}, + Verbs: []string{"*"}, + }, + }, + []common.RoleMapping{ + {Name: "dev", Users: []string{"dev-user"}}, + {Name: "admin", Users: []string{"admin-user"}}, + }, + ) + + user := model.User{Username: "dev-user"} + b.ResetTimer() + for b.Loop() { + CanAccess(user, "pods", "get", "dev-east", "team-alpha") + } +} diff --git a/pkg/rbac/manager.go b/pkg/rbac/manager.go index 378cbddf..a6f2bcb1 100644 --- a/pkg/rbac/manager.go +++ b/pkg/rbac/manager.go @@ -11,9 +11,10 @@ import ( ) var ( - RBACConfig *common.RolesConfig - once sync.Once - rwlock sync.RWMutex + RBACConfig *common.RolesConfig + compiledRoles []compiledRole // pre-compiled regex patterns, rebuilt on every sync + once sync.Once + rwlock sync.RWMutex ) func InitRBAC() { @@ -60,8 +61,16 @@ func loadRolesFromDB() error { cfg.RoleMapping = append(cfg.RoleMapping, rm) } } + // Pre-compile all regex patterns once (Solutions A+D). + // This runs every ~60s on sync, never on the hot request path. + compiled := make([]compiledRole, len(cfg.Roles)) + for i, r := range cfg.Roles { + compiled[i] = compileRole(r) + } + rwlock.Lock() RBACConfig = cfg + compiledRoles = compiled rwlock.Unlock() return nil } diff --git a/pkg/rbac/rbac.go b/pkg/rbac/rbac.go index 2180f372..b7eb30f4 100644 --- a/pkg/rbac/rbac.go +++ b/pkg/rbac/rbac.go @@ -2,25 +2,25 @@ package rbac import ( "fmt" - "regexp" "slices" - "strings" "github.com/zxh326/kite/pkg/common" "github.com/zxh326/kite/pkg/model" "k8s.io/klog/v2" ) -// CanAccess checks if user/oidcGroup can access resource with verb in cluster/namespace +// CanAccess checks if user/oidcGroup can access resource with verb in cluster/namespace. +// Uses pre-compiled regex patterns — zero regexp.Compile calls on the hot path. func CanAccess(user model.User, resource, verb, cluster, namespace string) bool { - roles := GetUserRoles(user) - for _, role := range roles { - if match(role.Clusters, cluster) && - match(role.Namespaces, namespace) && - match(role.Resources, resource) && - match(role.Verbs, verb) { + roles := getCompiledUserRoles(user) + for i := range roles { + r := &roles[i] + if matchCompiled(r.clusters, cluster) && + matchCompiled(r.namespaces, namespace) && + matchCompiled(r.resources, resource) && + matchCompiled(r.verbs, verb) { klog.V(1).Infof("RBAC Check - User: %s, OIDC Groups: %v, Resource: %s, Verb: %s, Cluster: %s, Namespace: %s, Hit Role: %v", - user.Key(), user.OIDCGroups, resource, verb, cluster, namespace, role.Name) + user.Key(), user.OIDCGroups, resource, verb, cluster, namespace, r.Name) return true } } @@ -30,9 +30,9 @@ func CanAccess(user model.User, resource, verb, cluster, namespace string) bool } func CanAccessCluster(user model.User, name string) bool { - roles := GetUserRoles(user) - for _, role := range roles { - if match(role.Clusters, name) { + roles := getCompiledUserRoles(user) + for i := range roles { + if matchCompiled(roles[i].clusters, name) { return true } } @@ -40,78 +40,88 @@ func CanAccessCluster(user model.User, name string) bool { } func CanAccessNamespace(user model.User, cluster, name string) bool { - roles := GetUserRoles(user) - for _, role := range roles { - if match(role.Clusters, cluster) && match(role.Namespaces, name) { + roles := getCompiledUserRoles(user) + for i := range roles { + r := &roles[i] + if matchCompiled(r.clusters, cluster) && matchCompiled(r.namespaces, name) { return true } } return false } -// GetUserRoles returns all roles for a user/oidcGroups +// GetUserRoles returns all roles for a user/oidcGroups. +// Kept public for external consumers that need the raw common.Role slice +// (e.g. API responses, AI agent). Hot-path callers use getCompiledUserRoles. func GetUserRoles(user model.User) []common.Role { if user.Roles != nil { return user.Roles } - rolesMap := make(map[string]common.Role) + roles := getCompiledUserRoles(user) + out := make([]common.Role, len(roles)) + for i := range roles { + out[i] = roles[i].Role + } + return out +} + +// getCompiledUserRoles resolves the user's compiled roles from the current config. +// When user.Roles is pre-populated (the common path via RequireAuth), we look up +// each role by name in the pre-compiled cache to avoid re-running regexp.Compile +// on every request. Only roles not found in the cache are compiled on-the-fly. +func getCompiledUserRoles(user model.User) []compiledRole { + // Common path: user already has pre-resolved raw roles (populated by RequireAuth). + // Look them up in the pre-compiled cache instead of recompiling. + if user.Roles != nil { + rwlock.RLock() + defer rwlock.RUnlock() + result := make([]compiledRole, 0, len(user.Roles)) + for _, r := range user.Roles { + if cr := findCompiledRole(r.Name); cr != nil { + result = append(result, *cr) + } else { + // Role not in cache (e.g. dynamically assigned) — compile on the fly + result = append(result, compileRole(r)) + } + } + return result + } + + rolesMap := make(map[string]*compiledRole) rwlock.RLock() defer rwlock.RUnlock() for _, mapping := range RBACConfig.RoleMapping { if contains(mapping.Users, "*") || contains(mapping.Users, user.Key()) { - if r := findRole(mapping.Name); r != nil { - rolesMap[r.Name] = *r + if r := findCompiledRole(mapping.Name); r != nil { + rolesMap[r.Name] = r } } for _, group := range user.OIDCGroups { if contains(mapping.OIDCGroups, group) { - if r := findRole(mapping.Name); r != nil { - rolesMap[r.Name] = *r + if r := findCompiledRole(mapping.Name); r != nil { + rolesMap[r.Name] = r } } } } - roles := make([]common.Role, 0, len(rolesMap)) + roles := make([]compiledRole, 0, len(rolesMap)) for _, role := range rolesMap { - roles = append(roles, role) + roles = append(roles, *role) } return roles } -func findRole(name string) *common.Role { - rwlock.RLock() - defer rwlock.RUnlock() - for _, r := range RBACConfig.Roles { - if r.Name == name { - return &r +// findCompiledRole looks up a pre-compiled role by name. +// Must be called under rwlock.RLock. +func findCompiledRole(name string) *compiledRole { + for i := range compiledRoles { + if compiledRoles[i].Name == name { + return &compiledRoles[i] } } return nil } -func match(list []string, val string) bool { - for _, v := range list { - if len(v) > 1 && strings.HasPrefix(v, "!") { - if v[1:] == val { - return false - } - } - if v == "*" || v == val { - return true - } - - re, err := regexp.Compile(v) - if err != nil { - klog.Error(err) - continue - } - if re.MatchString(val) { - return true - } - } - return false -} - func contains(list []string, val string) bool { return slices.Contains(list, val) } diff --git a/pkg/rbac/rbac_test.go b/pkg/rbac/rbac_test.go index 0d96fbd5..04857ffa 100644 --- a/pkg/rbac/rbac_test.go +++ b/pkg/rbac/rbac_test.go @@ -263,10 +263,7 @@ func TestCanAccess(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - RBACConfig = &common.RolesConfig{ - Roles: tc.roles, - RoleMapping: tc.mappings, - } + setTestRBACConfig(tc.roles, tc.mappings) result := CanAccess(model.User{Username: tc.user, OIDCGroups: tc.oidcGroups}, tc.resource, tc.verb, tc.cluster, tc.namespace) if result != tc.expected { @@ -275,3 +272,16 @@ func TestCanAccess(t *testing.T) { }) } } + +// setTestRBACConfig configures RBACConfig and compiledRoles for testing. +func setTestRBACConfig(roles []common.Role, mappings []common.RoleMapping) { + RBACConfig = &common.RolesConfig{ + Roles: roles, + RoleMapping: mappings, + } + compiled := make([]compiledRole, len(roles)) + for i, r := range roles { + compiled[i] = compileRole(r) + } + compiledRoles = compiled +}