From a699475f441072cc5ee559f748bec381021e68e9 Mon Sep 17 00:00:00 2001 From: Francesco Torta <62566275+fra98@users.noreply.github.com> Date: Fri, 22 Sep 2023 17:25:44 +0200 Subject: [PATCH] Always reflect kube-root-ca.crt --- .../reflection/configuration/configmap.go | 21 +++++++++++ .../configuration/configmap_test.go | 22 +++++++++--- .../reflection/exposition/endpointslice.go | 35 +------------------ .../reflection/generic/namespaced.go | 32 +++++++++++++++++ 4 files changed, 72 insertions(+), 38 deletions(-) diff --git a/pkg/virtualKubelet/reflection/configuration/configmap.go b/pkg/virtualKubelet/reflection/configuration/configmap.go index 2ac0fbc9e7..70ff4bd332 100644 --- a/pkg/virtualKubelet/reflection/configuration/configmap.go +++ b/pkg/virtualKubelet/reflection/configuration/configmap.go @@ -162,6 +162,27 @@ func (ncr *NamespacedConfigMapReflector) Handle(ctx context.Context, name string return nil } +// ShouldSkipReflection returns whether the reflection of the given object should be skipped. +func (ncr *NamespacedConfigMapReflector) ShouldSkipReflection(obj metav1.Object) (bool, error) { + // If the object is the root CA configmap, the object must always be reflected independently + // from the reflection policy, unless the object is specifically marked with the allow or + // deny annotations. + if obj.GetName() == forge.RootCAConfigMapName { + shouldSkip, err := ncr.ForcedAllowOrSkip(obj) + switch { + case err != nil: + return false, err + case shouldSkip != nil: + return *shouldSkip, nil + default: + return false, nil + } + } + + // Otherwise, the standard reflection policy is applied. + return ncr.NamespacedReflector.ShouldSkipReflection(obj) +} + // List returns the list of objects. func (ncr *NamespacedConfigMapReflector) List() ([]interface{}, error) { return virtualkubelet.List[virtualkubelet.Lister[*corev1.ConfigMap], *corev1.ConfigMap]( diff --git a/pkg/virtualKubelet/reflection/configuration/configmap_test.go b/pkg/virtualKubelet/reflection/configuration/configmap_test.go index a0283fe38e..e8ec11d979 100644 --- a/pkg/virtualKubelet/reflection/configuration/configmap_test.go +++ b/pkg/virtualKubelet/reflection/configuration/configmap_test.go @@ -240,10 +240,24 @@ var _ = Describe("ConfigMap Reflection", func() { CreateConfigMap(&local) }) - It("should succeed", func() { Expect(err).ToNot(HaveOccurred()) }) - It("the remapped remote object should be created", func() { - _, err = client.CoreV1().ConfigMaps(RemoteNamespace).Get(ctx, forge.RemoteConfigMapName(name), metav1.GetOptions{}) - Expect(err).ToNot(HaveOccurred()) + When("the reflection type is DenyList", func() { + It("should succeed", func() { Expect(err).ToNot(HaveOccurred()) }) + It("the remapped remote object should be created", func() { + _, err = client.CoreV1().ConfigMaps(RemoteNamespace).Get(ctx, forge.RemoteConfigMapName(name), metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + When("the reflection type is AllowList", func() { + BeforeEach(func() { + reflectionType = consts.AllowList + }) + + It("should succeed", func() { Expect(err).ToNot(HaveOccurred()) }) + It("the remapped remote object should be created", func() { + _, err = client.CoreV1().ConfigMaps(RemoteNamespace).Get(ctx, forge.RemoteConfigMapName(name), metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + }) }) }) }) diff --git a/pkg/virtualKubelet/reflection/exposition/endpointslice.go b/pkg/virtualKubelet/reflection/exposition/endpointslice.go index ece6f2a0a0..a34d276807 100644 --- a/pkg/virtualKubelet/reflection/exposition/endpointslice.go +++ b/pkg/virtualKubelet/reflection/exposition/endpointslice.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "reflect" - "strings" "sync" corev1 "k8s.io/api/core/v1" @@ -32,7 +31,6 @@ 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" @@ -329,42 +327,11 @@ 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 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) + shouldSkip, err := ner.ForcedAllowOrSkip(obj) if err != nil { return true, err } else if shouldSkip != nil { diff --git a/pkg/virtualKubelet/reflection/generic/namespaced.go b/pkg/virtualKubelet/reflection/generic/namespaced.go index 8fd8bacf1a..bd568ed42d 100644 --- a/pkg/virtualKubelet/reflection/generic/namespaced.go +++ b/pkg/virtualKubelet/reflection/generic/namespaced.go @@ -26,6 +26,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/record" "k8s.io/klog/v2" + "k8s.io/utils/pointer" "github.com/liqotech/liqo/pkg/consts" "github.com/liqotech/liqo/pkg/virtualKubelet/forge" @@ -133,3 +134,34 @@ func (gnr *NamespacedReflector) ShouldSkipReflection(obj metav1.Object) (bool, e func (gnr *NamespacedReflector) GetReflectionType() consts.ReflectionType { return gnr.reflectionType } + +// 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 (gnr *NamespacedReflector) 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("object %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 + } +}