Skip to content

Commit

Permalink
Endpointslices reflection fix
Browse files Browse the repository at this point in the history
  • Loading branch information
fra98 authored and adamjensenbot committed Sep 25, 2023
1 parent 5278146 commit 4edf128
Show file tree
Hide file tree
Showing 9 changed files with 112 additions and 19 deletions.
1 change: 0 additions & 1 deletion cmd/virtual-kubelet/root/opts.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 6 additions & 1 deletion cmd/virtual-kubelet/root/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion deployments/liqo/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
2 changes: 0 additions & 2 deletions deployments/liqo/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions docs/usage/reflection.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
56 changes: 48 additions & 8 deletions pkg/virtualKubelet/reflection/exposition/endpointslice.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"fmt"
"reflect"
"strings"
"sync"

corev1 "k8s.io/api/core/v1"
Expand All @@ -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"

Expand Down Expand Up @@ -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)
}

Expand Down
53 changes: 52 additions & 1 deletion pkg/virtualKubelet/reflection/exposition/endpointslice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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
Expand All @@ -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() {
Expand Down
2 changes: 1 addition & 1 deletion pkg/virtualKubelet/reflection/generic/reflector.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 4edf128

Please sign in to comment.