From 53c05997753f13a3000814328706a00b496155e1 Mon Sep 17 00:00:00 2001 From: Francesco Torta <62566275+fra98@users.noreply.github.com> Date: Fri, 22 Sep 2023 15:48:49 +0200 Subject: [PATCH] Endpointslices reflection fix --- cmd/virtual-kubelet/root/opts.go | 1 - cmd/virtual-kubelet/root/root.go | 7 ++- deployments/liqo/README.md | 1 - .../liqo-controller-manager-deployment.yaml | 1 - deployments/liqo/values.yaml | 2 - docs/usage/reflection.md | 8 ++- .../reflection/exposition/endpointslice.go | 56 ++++++++++++++++--- .../reflection/generic/reflector.go | 2 +- 8 files changed, 60 insertions(+), 18 deletions(-) diff --git a/cmd/virtual-kubelet/root/opts.go b/cmd/virtual-kubelet/root/opts.go index 5a023322fe..55c0a449b8 100644 --- a/cmd/virtual-kubelet/root/opts.go +++ b/cmd/virtual-kubelet/root/opts.go @@ -63,7 +63,6 @@ var DefaultReflectorsWorkers = map[generic.ResourceReflected]uint{ var DefaultReflectorsTypes = map[generic.ResourceReflected]consts.ReflectionType{ generic.Pod: consts.CustomLiqo, generic.Service: consts.DenyList, - generic.EndpointSlice: consts.DenyList, generic.Ingress: consts.DenyList, generic.ConfigMap: consts.DenyList, generic.Secret: consts.DenyList, diff --git a/cmd/virtual-kubelet/root/root.go b/cmd/virtual-kubelet/root/root.go index 5c79165beb..3ffc67dcc8 100644 --- a/cmd/virtual-kubelet/root/root.go +++ b/cmd/virtual-kubelet/root/root.go @@ -283,7 +283,12 @@ func getReflectorsConfigs(c *Opts) (map[generic.ResourceReflected]*generic.Refle if isReflectionTypeNotCustomizable(*resource) { reflectionType = DefaultReflectorsTypes[*resource] } else { - reflectionType = consts.ReflectionType(*c.ReflectorsType[string(*resource)]) + if *resource == generic.EndpointSlice { + // the endpointslice reflector inherits the reflection type from the service reflector. + reflectionType = consts.ReflectionType(*c.ReflectorsType[string(generic.Service)]) + } else { + reflectionType = consts.ReflectionType(*c.ReflectorsType[string(*resource)]) + } if reflectionType != consts.DenyList && reflectionType != consts.AllowList { return nil, fmt.Errorf("reflection type %q is not valid for resource %s. Ammitted values: %q, %q", reflectionType, *resource, consts.DenyList, consts.AllowList) diff --git a/deployments/liqo/README.md b/deployments/liqo/README.md index 8ea2aeaaad..32d0215070 100644 --- a/deployments/liqo/README.md +++ b/deployments/liqo/README.md @@ -131,7 +131,6 @@ | pullPolicy | string | `"IfNotPresent"` | The pullPolicy for liqo pods. | | reflection.configmap.type | string | `"DenyList"` | The type of reflection used for the configmaps reflector. Ammitted values: "DenyList", "AllowList". | | reflection.configmap.workers | int | `3` | The number of workers used for the configmaps reflector. Set 0 to disable the reflection of configmaps. | -| reflection.endpointslice.type | string | `"DenyList"` | The type of reflection used for the endpointslices reflector Ammitted values: "DenyList", "AllowList". | | reflection.endpointslice.workers | int | `10` | The number of workers used for the endpointslices reflector. Set 0 to disable the reflection of endpointslices. | | reflection.event.type | string | `"DenyList"` | The type of reflection used for the events reflector. Ammitted values: "DenyList", "AllowList". | | reflection.event.workers | int | `3` | The number of workers used for the events reflector. Set 0 to disable the reflection of events. | diff --git a/deployments/liqo/templates/liqo-controller-manager-deployment.yaml b/deployments/liqo/templates/liqo-controller-manager-deployment.yaml index 830438df8c..65e2d11cfc 100644 --- a/deployments/liqo/templates/liqo-controller-manager-deployment.yaml +++ b/deployments/liqo/templates/liqo-controller-manager-deployment.yaml @@ -87,7 +87,6 @@ spec: - --persistentvolumeclaim-reflection-workers={{ .Values.reflection.persistentvolumeclaim.workers }} - --event-reflection-workers={{ .Values.reflection.event.workers }} - --service-reflection-type={{ .Values.reflection.service.type }} - - --endpointslice-reflection-type={{ .Values.reflection.endpointslice.type }} - --ingress-reflection-type={{ .Values.reflection.ingress.type }} - --configmap-reflection-type={{ .Values.reflection.configmap.type }} - --secret-reflection-type={{ .Values.reflection.secret.type }} diff --git a/deployments/liqo/values.yaml b/deployments/liqo/values.yaml index bd0a0b4809..908d90390a 100644 --- a/deployments/liqo/values.yaml +++ b/deployments/liqo/values.yaml @@ -66,8 +66,6 @@ reflection: endpointslice: # -- The number of workers used for the endpointslices reflector. Set 0 to disable the reflection of endpointslices. workers: 10 - # -- The type of reflection used for the endpointslices reflector Ammitted values: "DenyList", "AllowList". - type: DenyList ingress: # -- The number of workers used for the ingresses reflector. Set 0 to disable the reflection of ingresses. workers: 3 diff --git a/docs/usage/reflection.md b/docs/usage/reflection.md index 68ea62189d..da6198f92a 100644 --- a/docs/usage/reflection.md +++ b/docs/usage/reflection.md @@ -24,9 +24,11 @@ liqoctl install ... --set "reflection.secret.type=AllowList" ``` ````{warning} -***DenyList*** is the **default** reflection policy for all resources. - -Only the *Pods*, *PVCs*, and *ServiceAccounts* reflectors follow a **custom** Liqo logic and can't be customized. +* ***DenyList*** is the **default** reflection policy for all resources. +* Only the *Pods*, *PVCs*, and *ServiceAccounts* reflectors follow a **custom** Liqo logic and can't be customized. +* The *EndpointSlice* reflector inherits the reflection policy from the *Service* reflector, and follows the following policy: + * an endpointslice is (not) reflected if the associated service is (not) reflected + * you can bypass the above behavior if you explicitly annotate the endpointslice itself (i.e., reflect the endpointslice using `liqo.io/allow-reflection` annotation, do not reflect using `liqo.io/skip-reflection`) ```` ````{admonition} Note diff --git a/pkg/virtualKubelet/reflection/exposition/endpointslice.go b/pkg/virtualKubelet/reflection/exposition/endpointslice.go index d8c8255dd1..73d866b1d1 100644 --- a/pkg/virtualKubelet/reflection/exposition/endpointslice.go +++ b/pkg/virtualKubelet/reflection/exposition/endpointslice.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "reflect" + "strings" "sync" corev1 "k8s.io/api/core/v1" @@ -31,6 +32,7 @@ import ( corev1listers "k8s.io/client-go/listers/core/v1" discoveryv1listers "k8s.io/client-go/listers/discovery/v1" "k8s.io/klog/v2" + "k8s.io/utils/pointer" "k8s.io/utils/trace" "sigs.k8s.io/controller-runtime/pkg/client" @@ -327,30 +329,68 @@ func (ner *NamespacedEndpointSliceReflector) UnmapEndpointIPs(ctx context.Contex return nil } +// forcedAllowOrSkip checks whether the given object is *explicitly* marked to be allowed or skipped +// (i.e., it has the allow or the deny annotation), independently from the reflection policy. +// If so, it returns whether the object should be skipped, or an error if unable to determine it. +// Otherwise, it return a nil bool as it is undeterminated, since we are not considering the reflection +// policy at this stage. +func forcedAllowOrSkip(obj metav1.Object) (*bool, error) { + allowAnnot, skipAnnot := false, false + + value, ok := obj.GetAnnotations()[consts.AllowReflectionAnnotationKey] + if ok && !strings.EqualFold(value, "false") { + allowAnnot = true + } + value, ok = obj.GetAnnotations()[consts.SkipReflectionAnnotationKey] + if ok && !strings.EqualFold(value, "false") { + skipAnnot = true + } + + if allowAnnot && skipAnnot { + return nil, fmt.Errorf("endpointslice %q can't have both the allow and deny annotations set", klog.KObj(obj)) + } + + switch { + case allowAnnot: + return pointer.Bool(true), nil + case skipAnnot: + return pointer.Bool(false), nil + default: + return nil, nil + } +} + // ShouldSkipReflection returns whether the reflection of the given object should be skipped. func (ner *NamespacedEndpointSliceReflector) ShouldSkipReflection(obj metav1.Object) (bool, error) { - // Check if the endpointslice is marked to be skipped. - skipReflection, err := ner.NamespacedReflector.ShouldSkipReflection(obj) + // Check if the endpointslice is explicitly marked to be skipped or allowed. + // If so, we do not care about the reflection policy, as the annotation takes precedence. + shouldSkip, err := forcedAllowOrSkip(obj) if err != nil { return true, err - } - if skipReflection { - return true, nil + } else if shouldSkip != nil { + return *shouldSkip, nil } + // If no annotation are set, we consider the the standard reflection policy. + // Note: it is inherited from the service reflector. + // Check if a service is associated to the EndpointSlice, and whether it is marked to be skipped. svcname, ok := obj.GetLabels()[discoveryv1.LabelServiceName] if !ok { - return false, nil + // If the endpointslice is not associated to a service, use standard logic to determine whetever or not + // to reflect the endpointslice. + return ner.NamespacedReflector.ShouldSkipReflection(obj) } svc, err := ner.localServices.Get(svcname) - // Continue with the reflection in case the service is not found, as this is likely due to a race conditions + // Continue with the standard logic in case the service is not found, as this is likely due to a race conditions // (i.e., the service has not yet been cached). If necessary, the informer will trigger a re-enqueue, // thus performing once more this check. if err != nil { - return false, nil + return ner.NamespacedReflector.ShouldSkipReflection(obj) } + + // The associated service exists, hence check if it is marked to be skipped. return ner.NamespacedReflector.ShouldSkipReflection(svc) } diff --git a/pkg/virtualKubelet/reflection/generic/reflector.go b/pkg/virtualKubelet/reflection/generic/reflector.go index 2a865f2e95..3fa7476c42 100644 --- a/pkg/virtualKubelet/reflection/generic/reflector.go +++ b/pkg/virtualKubelet/reflection/generic/reflector.go @@ -105,7 +105,7 @@ const ( var Reflectors = []ResourceReflected{Pod, Service, EndpointSlice, Ingress, ConfigMap, Secret, ServiceAccount, PersistentVolumeClaim, Event} // ReflectorsCustomizableType is the list of resources for which the reflection type can be customized. -var ReflectorsCustomizableType = []ResourceReflected{Service, EndpointSlice, Ingress, ConfigMap, Secret, Event} +var ReflectorsCustomizableType = []ResourceReflected{Service, Ingress, ConfigMap, Secret, Event} // ReflectorConfig contains configuration parameters of the reflector. type ReflectorConfig struct {