gitPratikSingh commented on code in PR #6926:
URL: https://github.com/apache/camel-k/pull/6926#discussion_r4495389350
##########
pkg/trait/gc.go:
##########
@@ -152,37 +154,9 @@ func (t *gcTrait) Apply(e *Environment) error {
}
func (t *gcTrait) garbageCollectResources(e *Environment) error {
- deletableGVKs, err := t.getDeletableTypes(e)
+ deletableGVKs, err := t.getDeletableTypes()
Review Comment:
Done — the deletable set is now computed once at package initialization,
from the roles and the scheme bundled in the binary, with no lazy computation
on the garbage collection path. As you say, it can only change when the
operator is restarted with a different binary.
##########
pkg/trait/gc.go:
##########
@@ -255,89 +235,238 @@ func canBeDeleted(it *v1.Integration, u
unstructured.Unstructured) bool {
return false
}
-// getDeletableTypes returns the list of deletable types resources, inspecting
the rules for which the operator SA is allowed in the
-// Integration namespace.
-func (t *gcTrait) getDeletableTypes(e *Environment)
(map[schema.GroupVersionKind]struct{}, error) {
- lock.Lock()
- defer lock.Unlock()
-
- // Return a fresh map even when returning cached collectables
+// deletableTypesFromRules builds the in-memory map of deletable types from the
+// rules granted to the operator service account. It is a pure function over
its
+// inputs, so the mapping from RBAC rules to deletable types can be tested
+// without a cluster.
+//
+// A resource is deletable when some rule allows the "delete" verb (or "*") for
+// its API group (or "*") and its resource name (or "*"). Only resources
present
+// in resourceTypes are considered: role rules carry no version or kind
+// information, and subresources cannot be deleted as a whole.
+func deletableTypesFromRules(rules []rbacv1.PolicyRule, resourceTypes
map[schema.GroupResource]schema.GroupVersionKind)
map[schema.GroupVersionKind]struct{} {
GVKs := make(map[schema.GroupVersionKind]struct{})
- // Rate limit to avoid Discovery and SelfSubjectRulesReview requests at
every reconciliation.
- if !rateLimiter.Allow() {
- // Return the cached set of garbage collectable GVKs.
- maps.Copy(GVKs, collectableGVKs)
+ for groupResource, GVK := range resourceTypes {
+ for _, rule := range rules {
+ if !slices.Contains(rule.Verbs, "delete") &&
!slices.Contains(rule.Verbs, "*") {
+ continue
+ }
+ if !slices.Contains(rule.APIGroups,
groupResource.Group) && !slices.Contains(rule.APIGroups, "*") {
+ continue
+ }
+ if !slices.Contains(rule.Resources,
groupResource.Resource) && !slices.Contains(rule.Resources, "*") {
+ continue
+ }
+ GVKs[GVK] = struct{}{}
- return GVKs, nil
+ break
+ }
}
- // We rely on the discovery API to retrieve all the resources GVK,
- // that results in an unbounded set that can impact garbage collection
latency when scaling up.
- resources, err :=
t.Client.Discovery().ServerPreferredNamespacedResources()
- // Swallow group discovery errors, e.g., Knative serving exposes
- // an aggregated API for custom.metrics.k8s.io that requires special
- // authentication scheme while discovering preferred resources.
- if err != nil && !discovery.IsGroupDiscoveryFailedError(err) {
- return nil, err
+ return GVKs
+}
+
+// schemeResourceTypes indexes the types registered in a scheme by the API
+// group and resource name they are served under, as those names appear in
+// role rules. List types and the internal option types registered for every
+// group version are skipped, as they cannot be deleted. When a scheme
+// registers several versions of the same resource, the most stable and
+// recent one wins, so the result does not depend on registration order.
+func schemeResourceTypes(s *runtime.Scheme)
map[schema.GroupResource]schema.GroupVersionKind {
+ known := s.AllKnownTypes()
+ GVKs := make([]schema.GroupVersionKind, 0, len(known))
+ for GVK := range known {
+ if strings.HasSuffix(GVK.Kind, "List") ||
isInternalKind(GVK.Kind) {
+ continue
+ }
+ GVKs = append(GVKs, GVK)
}
+ // Sort for a deterministic result when two versions rank equally.
+ slices.SortFunc(GVKs, func(a, b schema.GroupVersionKind) int {
+ return strings.Compare(a.String(), b.String())
+ })
- // We only take types that support the "delete" verb,
- // to prevents from performing queries that we know are going to return
"MethodNotAllowed".
- APIResourceLists :=
discovery.FilteredBy(discovery.SupportsAllVerbs{Verbs: []string{"delete"}},
resources)
+ resourceTypes := make(map[schema.GroupResource]schema.GroupVersionKind,
len(GVKs))
+ for _, GVK := range GVKs {
+ groupResource := schema.GroupResource{Group: GVK.Group,
Resource: kindToResource(GVK.Kind)}
+ if current, ok := resourceTypes[groupResource]; !ok ||
preferredVersion(GVK, current) {
+ resourceTypes[groupResource] = GVK
+ }
+ }
- // Retrieve the permissions granted to the operator service account.
- // We assume the operator has only to garbage collect the resources it
has created.
- ssrr := &authorization.SelfSubjectRulesReview{
- Spec: authorization.SelfSubjectRulesReviewSpec{
- Namespace: e.Integration.Namespace,
- },
+ return resourceTypes
+}
+
+// isInternalKind reports whether a kind is one of the internal types that
+// metav1.AddToGroupVersion registers for every group version in a scheme.
+// They are API machinery, not resources an operator can delete.
+func isInternalKind(kind string) bool {
+ switch kind {
+ case "CreateOptions", "DeleteOptions", "GetOptions", "ListOptions",
+ "PatchOptions", "UpdateOptions", "WatchEvent", "Status":
+ return true
}
- ssrr, err =
e.Client.AuthorizationV1().SelfSubjectRulesReviews().Create(e.Ctx, ssrr,
metav1.CreateOptions{})
+
+ return false
+}
+
+// kindToResource converts a kind to the resource name it is served under,
+// applying the usual Kubernetes pluralization rules: "Gateway" becomes
+// "gateways", "NetworkPolicy" becomes "networkpolicies" and "Ingress"
+// becomes "ingresses". "Endpoints" is the one irregular case among the
+// registered types: its resource is the kind itself, unmodified.
+func kindToResource(kind string) string {
Review Comment:
This conversion is the one place where a name has to be derived: role rules
carry resource names, the scheme carries kinds, and nothing else in the binary
links the two. What keeps it from being fragile in practice is the coverage
test (TestOperatorRolesDeletableCoverage): it audits every resource the shipped
operator roles grant delete on and fails the build, naming the resource, if its
name is computed wrongly here or it stops resolving. So a pluralization mistake
breaks CI instead of silently narrowing garbage collection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
