Endpointslices reflection fix

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,

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)

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. |

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 }}

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

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

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(false), nil
+	case skipAnnot:
+		return pointer.Bool(true), 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)
 }
 

pkg/virtualKubelet/reflection/exposition/endpointslice_test.go

@@ -119,7 +119,7 @@ var _ = Describe("EndpointSlice Reflection Tests", func() {
 			liqoClient = liqoclientfake.NewSimpleClientset()
 			local = discoveryv1.EndpointSlice{ObjectMeta: metav1.ObjectMeta{Name: EndpointSliceName, Namespace: LocalNamespace}}
 			remote = vkv1alpha1.ShadowEndpointSlice{ObjectMeta: metav1.ObjectMeta{Name: EndpointSliceName, Namespace: RemoteNamespace}}
-			reflectionType = root.DefaultReflectorsTypes[generic.EndpointSlice]
+			reflectionType = root.DefaultReflectorsTypes[generic.Service] // reflection type inherited from the service reflector
 		})
 
 		AfterEach(func() {
@@ -276,6 +276,28 @@ var _ = Describe("EndpointSlice Reflection Tests", func() {
 					reflectionType = consts.AllowList
 				})
 
+				When("the local object does exist, and the associated service has the allow annotation", func() {
+					BeforeEach(func() {
+						local.Labels = map[string]string{discoveryv1.LabelServiceName: ServiceName}
+						local.AddressType = discoveryv1.AddressTypeIPv4
+						CreateEndpointSlice(&local)
+
+						CreateService(&corev1.Service{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: ServiceName, Namespace: LocalNamespace,
+								Annotations: map[string]string{consts.AllowReflectionAnnotationKey: "whatever"},
+							},
+							Spec: corev1.ServiceSpec{Ports: []corev1.ServicePort{{Name: "http", Port: 80, TargetPort: intstr.FromInt(8080)}}},
+						})
+					})
+
+					It("should succeed", func() { Expect(err).ToNot(HaveOccurred()) })
+					It("the remote object should be present", func() {
+						remoteAfter := GetShadowEndpointSlice(RemoteNamespace)
+						Expect(remoteAfter).ToNot(BeNil())
+					})
+				})
+
 				When("the local object does exist, but does not have the allow annotation", func() {
 					BeforeEach(func() {
 						local.AddressType = discoveryv1.AddressTypeIPv4
@@ -300,6 +322,35 @@ var _ = Describe("EndpointSlice Reflection Tests", func() {
 					})
 				})
 			})
+
+			When("the reflection is forced with the allow or skip annotation", func() {
+				When("the reflection is deny, but the object has the allow annotation", func() {
+					BeforeEach(func() {
+						reflectionType = consts.DenyList
+						local.SetAnnotations(map[string]string{consts.AllowReflectionAnnotationKey: "whatever"})
+						local.AddressType = discoveryv1.AddressTypeIPv4
+						CreateEndpointSlice(&local)
+					})
+
+					It("should succeed", func() { Expect(err).ToNot(HaveOccurred()) })
+					It("the remote object should be present", func() {
+						remoteAfter := GetShadowEndpointSlice(RemoteNamespace)
+						Expect(remoteAfter).ToNot(BeNil())
+					})
+				})
+
+				When("the reflection is allow, but the object has the skip annotation", func() {
+					BeforeEach(func() {
+						reflectionType = consts.AllowList
+						local.SetAnnotations(map[string]string{consts.SkipReflectionAnnotationKey: "whatever"})
+						local.AddressType = discoveryv1.AddressTypeIPv4
+						CreateEndpointSlice(&local)
+					})
+
+					When("the remote object does not exist", WhenBodyRemoteShouldNotExist(false))
+					When("the remote object does exist", WhenBodyRemoteShouldNotExist(true))
+				})
+			})
 		})
 
 		Context("address translation", func() {

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 {