Skip to content

Commit

Permalink
Merge pull request #646 from l1b0k/fix/policy
Browse files Browse the repository at this point in the history 
fix: ns selector may not work
  • Loading branch information
@BSWANG
BSWANG authored Jul 10, 2024
2 parents 677cd12 + b287173 commit 9b7a870
Show file tree
Hide file tree
Showing 3 changed files with 352 additions and 1 deletion.
2 changes: 1 addition & 1 deletion Dockerfile
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ARG TERWAY_POLICY_IMAGE=registry-cn-zhangjiakou.ack.aliyuncs.com/acs/terway:policy-16a7b819@sha256:e7b74fb86701c9eb49f17996230c5d305a70e0bf5da9780ce10166d8242b9908
ARG TERWAY_POLICY_IMAGE=registry-cn-zhangjiakou.ack.aliyuncs.com/acs/terway:policy-f5386adf@sha256:6d7b22e05891840f797e4ba411fe7872caba30bf3d8a013e3ef60f10da669400
ARG UBUNTU_IMAGE=registry.cn-hangzhou.aliyuncs.com/acs/ubuntu:22.04-update
ARG CILIUM_LLVM_IMAGE=quay.io/cilium/cilium-llvm:547db7ec9a750b8f888a506709adb41f135b952e@sha256:4d6fa0aede3556c5fb5a9c71bc6b9585475ac9b1064f516d4c45c8fb691c9d9e
ARG CILIUM_BPFTOOL_IMAGE=quay.io/cilium/cilium-bpftool:78448c1a37ff2b790d5e25c3d8b8ec3e96e6405f@sha256:99a9453a921a8de99899ef82e0822f0c03f65d97005c064e231c06247ad8597d
Expand Down
193 changes: 193 additions & 0 deletions policy/cilium/0028-k8s-Restrict-configuring-reserved-init-policy-via-CN.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Joe Stringer <[email protected]>
Date: Wed, 6 Sep 2023 05:38:03 +0800
Subject: k8s: Restrict configuring reserved:init policy via CNP

Typically if the policy target EndpointSelector does not include a
namespace, parsing will inject the namespace to ensure that only Pods
within the same namespace can be selected. Furthermore, for peer
EndpointSelectors there is a similar behaviour where ingress or egress
is allowed only to the current namespace unless the user specifies which
namespace the peer exists in. However, these defaults did not previously
apply to policies that select the reserved:init Identity, since this
Identity is not inherently namespaced, and hence automatically injecting
the namespace would cause the policy to no longer match this Identity.

The reserved:init Identity was introduced in order to allow a different
policy to apply to Pods as they start up, iff the full Identity of those
workloads cannot be determined when they are first connecting to the
Cilium network (CNI ADD). In order to support this special reserved:init
Identity, various exceptions were introduced into CiliumNetworkPolicy
parser that removed the automatic insertion of namespace into the
policies when there were label matches for the reserved:init namespace.

The awkward part about this abstraction early on was that there was no
way to directly associate the reserved:init Identity to any specific
Kubernetes namespace, and yet CiliumNetworkPolicy was always inherently
tied to a specific namespace. Since the introduction of the
reserved:init Identity, CiliumClusterwideNetworkPolicy was also
introduced, which is a much better fit for the ability to select
endpoints that are not inherently namespaced.

This patch deprecates support for applying network policies for the
reserved:init Identity via CiliumNetworkPolicy in favour of
CiliumClusterwideNetworkPolicy. As a benefit, this allows us to simplify
the logic that applies the namespaces into the policies and reduce the
likelihood of misconfigurations.

Signed-off-by: Joe Stringer <[email protected]>
Signed-off-by: l1b0k <[email protected]>
---
examples/policies/l4/init.yaml | 2 +-
pkg/k8s/apis/cilium.io/utils/utils.go | 16 +++--
pkg/k8s/apis/cilium.io/utils/utils_test.go | 83 ++++++++++++++++++++++
3 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/examples/policies/l4/init.yaml b/examples/policies/l4/init.yaml
index 038524feb7..30c8633674 100644
--- a/examples/policies/l4/init.yaml
+++ b/examples/policies/l4/init.yaml
@@ -1,5 +1,5 @@
apiVersion: "cilium.io/v2"
-kind: CiliumNetworkPolicy
+kind: CiliumClusterwideNetworkPolicy
metadata:
name: init
specs:
diff --git a/pkg/k8s/apis/cilium.io/utils/utils.go b/pkg/k8s/apis/cilium.io/utils/utils.go
index badcdfbdf4..365621fe63 100644
--- a/pkg/k8s/apis/cilium.io/utils/utils.go
+++ b/pkg/k8s/apis/cilium.io/utils/utils.go
@@ -82,14 +82,16 @@ func getEndpointSelector(namespace string, labelSelector *slim_metav1.LabelSelec
// Those pods don't have any labels, so they don't have a namespace label either.
// Don't add a namespace label to those endpoint selectors, or we wouldn't be
// able to match on those pods.
- if !matchesInit && !es.HasKey(podPrefixLbl) && !es.HasKey(podAnyPrefixLbl) {
+ if !es.HasKey(podPrefixLbl) && !es.HasKey(podAnyPrefixLbl) {
if namespace == "" {
// For a clusterwide policy if a namespace is not specified in the labels we add
// a selector to only match endpoints that contains a namespace label.
// This is to make sure that we are only allowing traffic for cilium managed k8s endpoints
// and even if a wildcard is provided in the selector we don't proceed with a truly
// empty(allow all) endpoint selector for the policy.
- es.AddMatchExpression(podPrefixLbl, slim_metav1.LabelSelectorOpExists, []string{})
+ if !matchesInit {
+ es.AddMatchExpression(podPrefixLbl, slim_metav1.LabelSelectorOpExists, []string{})
+ }
} else {
es.AddMatch(podPrefixLbl, namespace)
}
@@ -299,11 +301,11 @@ func ParseToCiliumRule(namespace, name string, uid types.UID, r *api.Rule) *api.
// the policy is being stored, thus we add the namespace to
// the MatchLabels map.
//
- // Policies applying on initializing pods are a special case.
- // Those pods don't have any labels, so they don't have a namespace label either.
- // Don't add a namespace label to those endpoint selectors, or we wouldn't be
- // able to match on those pods.
- if !retRule.EndpointSelector.HasKey(podInitLbl) && namespace != "" {
+ // Policies applying to all namespaces are a special case.
+ // Such policies can match on any traffic from Pods or Nodes,
+ // so it wouldn't make sense to inject a namespace match for
+ // those policies.
+ if namespace != "" {
userNamespace, present := r.EndpointSelector.GetMatch(podPrefixLbl)
if present && !namespacesAreValid(namespace, userNamespace) {
log.WithFields(logrus.Fields{
diff --git a/pkg/k8s/apis/cilium.io/utils/utils_test.go b/pkg/k8s/apis/cilium.io/utils/utils_test.go
index 88fdcdad3a..060cc3a593 100644
--- a/pkg/k8s/apis/cilium.io/utils/utils_test.go
+++ b/pkg/k8s/apis/cilium.io/utils/utils_test.go
@@ -201,6 +201,89 @@ func Test_ParseToCiliumRule(t *testing.T) {
},
),
},
+ {
+ // CNP with endpoint selectors should always select the
+ // current namespace
+ name: "parse-init-policy-namespaced",
+ args: args{
+ namespace: slim_metav1.NamespaceDefault,
+ uid: uuid,
+ rule: &api.Rule{
+ EndpointSelector: api.NewESFromMatchRequirements(
+ nil,
+ []slim_metav1.LabelSelectorRequirement{
+ {
+ Key: "reserved.init",
+ Operator: slim_metav1.LabelSelectorOpDoesNotExist,
+ },
+ },
+ ),
+ Ingress: []api.IngressRule{
+ {
+ IngressCommonRule: api.IngressCommonRule{
+ FromEndpoints: []api.EndpointSelector{
+ {
+ LabelSelector: &slim_metav1.LabelSelector{},
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ want: api.NewRule().WithEndpointSelector(
+ api.NewESFromMatchRequirements(
+ map[string]string{
+ namespace: "default",
+ },
+ []slim_metav1.LabelSelectorRequirement{
+ {
+ Key: "reserved.init",
+ Operator: slim_metav1.LabelSelectorOpDoesNotExist,
+ },
+ },
+ ),
+ ).WithIngressRules(
+ []api.IngressRule{
+ {
+ IngressCommonRule: api.IngressCommonRule{
+ FromEndpoints: []api.EndpointSelector{
+ api.NewESFromK8sLabelSelector(
+ labels.LabelSourceK8sKeyPrefix,
+ &slim_metav1.LabelSelector{
+ MatchLabels: map[string]string{
+ k8sConst.PodNamespaceLabel: "default",
+ },
+ }),
+ },
+ },
+ },
+ },
+ ).WithLabels(
+ labels.LabelArray{
+ {
+ Key: "io.cilium.k8s.policy.derived-from",
+ Value: "CiliumNetworkPolicy",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.name",
+ Value: "parse-init-policy-namespaced",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.namespace",
+ Value: "default",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.uid",
+ Value: string(uuid),
+ Source: labels.LabelSourceK8s,
+ },
+ },
+ ),
+ },
{
name: "set-any-source-for-namespace",
args: args{
--
2.45.2

158 changes: 158 additions & 0 deletions policy/cilium/0029-policy-Fix-selecting-endpoints-by-namespace-label.patch
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ilia Chernov <[email protected]>
Date: Wed, 15 May 2024 11:30:27 +0300
Subject: policy: Fix selecting endpoints by namespace label

It was not possible to match endpoints by namespace labels
even though web policy editor suggested it.
Label matching namespace by name was added implicitly to the policy
which makes selector by labels invalid.
After fix label matching namespace by name is not added in case user specified matching by namespace labels.

Fixes: #30149

Signed-off-by: Ilia Chernov <[email protected]>
---
pkg/k8s/apis/cilium.io/const.go | 5 +-
pkg/k8s/apis/cilium.io/utils/utils.go | 8 ++-
pkg/k8s/apis/cilium.io/utils/utils_test.go | 80 ++++++++++++++++++++++
3 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/pkg/k8s/apis/cilium.io/const.go b/pkg/k8s/apis/cilium.io/const.go
index 0b9e7bd2c3..d3cea253c1 100644
--- a/pkg/k8s/apis/cilium.io/const.go
+++ b/pkg/k8s/apis/cilium.io/const.go
@@ -40,9 +40,12 @@ const (
// kubernetes namespace's labels.
PodNamespaceMetaLabels = LabelPrefix + ".namespace.labels"

+ // PodNamespaceMetaLabelsPrefix is the prefix used for kubernetes namespace's labels
+ PodNamespaceMetaLabelsPrefix = PodNamespaceMetaLabels + "."
+
// PodNamespaceMetaNameLabel is the label that Kubernetes automatically adds
// to namespaces.
- PodNamespaceMetaNameLabel = PodNamespaceMetaLabels + "." + LabelMetadataName
+ PodNamespaceMetaNameLabel = PodNamespaceMetaLabelsPrefix + LabelMetadataName

// LabelMetadataName is the label name which, in-tree, is used to
// automatically label namespaces, so they can be selected easily by tools
diff --git a/pkg/k8s/apis/cilium.io/utils/utils.go b/pkg/k8s/apis/cilium.io/utils/utils.go
index 365621fe63..d775aa0e0e 100644
--- a/pkg/k8s/apis/cilium.io/utils/utils.go
+++ b/pkg/k8s/apis/cilium.io/utils/utils.go
@@ -26,6 +26,12 @@ const (
// represent pods in the default namespace for any source type.
podAnyPrefixLbl = labels.LabelSourceAnyKeyPrefix + k8sConst.PodNamespaceLabel

+ // podK8SNamespaceLabelsPrefix is the prefix use in the label selector for namespace labels.
+ podK8SNamespaceLabelsPrefix = labels.LabelSourceK8sKeyPrefix + k8sConst.PodNamespaceMetaLabelsPrefix
+ // podAnyNamespaceLabelsPrefix is the prefix use in the label selector for namespace labels
+ // for any source type.
+ podAnyNamespaceLabelsPrefix = labels.LabelSourceAnyKeyPrefix + k8sConst.PodNamespaceMetaLabelsPrefix
+
// podInitLbl is the label used in a label selector to match on
// initializing pods.
podInitLbl = labels.LabelSourceReservedKeyPrefix + labels.IDNameInit
@@ -92,7 +98,7 @@ func getEndpointSelector(namespace string, labelSelector *slim_metav1.LabelSelec
if !matchesInit {
es.AddMatchExpression(podPrefixLbl, slim_metav1.LabelSelectorOpExists, []string{})
}
- } else {
+ } else if !es.HasKeyPrefix(podK8SNamespaceLabelsPrefix) && !es.HasKeyPrefix(podAnyNamespaceLabelsPrefix) {
es.AddMatch(podPrefixLbl, namespace)
}
}
diff --git a/pkg/k8s/apis/cilium.io/utils/utils_test.go b/pkg/k8s/apis/cilium.io/utils/utils_test.go
index 060cc3a593..22b15db6c3 100644
--- a/pkg/k8s/apis/cilium.io/utils/utils_test.go
+++ b/pkg/k8s/apis/cilium.io/utils/utils_test.go
@@ -362,6 +362,86 @@ func Test_ParseToCiliumRule(t *testing.T) {
},
),
},
+ {
+ // When the rule specifies namespace labels, namespace label is not added
+ // by the namespace where the rule was inserted.
+ name: "parse-in-namespace-with-ns-labels-selector",
+ args: args{
+ namespace: slim_metav1.NamespaceDefault,
+ uid: uuid,
+ rule: &api.Rule{
+ EndpointSelector: api.NewESFromMatchRequirements(
+ map[string]string{
+ role: "backend",
+ },
+ nil,
+ ),
+ Ingress: []api.IngressRule{
+ {
+ IngressCommonRule: api.IngressCommonRule{
+ FromEndpoints: []api.EndpointSelector{
+ {
+ LabelSelector: &slim_metav1.LabelSelector{
+ MatchLabels: map[string]string{
+ podAnyNamespaceLabelsPrefix + "team": "team-a",
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ },
+ want: api.NewRule().WithEndpointSelector(
+ api.NewESFromMatchRequirements(
+ map[string]string{
+ role: "backend",
+ namespace: "default",
+ },
+ nil,
+ ),
+ ).WithIngressRules(
+ []api.IngressRule{
+ {
+ IngressCommonRule: api.IngressCommonRule{
+ FromEndpoints: []api.EndpointSelector{
+ api.NewESFromK8sLabelSelector(
+ labels.LabelSourceAnyKeyPrefix,
+ &slim_metav1.LabelSelector{
+ MatchLabels: map[string]string{
+ k8sConst.PodNamespaceMetaLabelsPrefix + "team": "team-a",
+ },
+ }),
+ },
+ },
+ },
+ },
+ ).WithLabels(
+ labels.LabelArray{
+ {
+ Key: "io.cilium.k8s.policy.derived-from",
+ Value: "CiliumNetworkPolicy",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.name",
+ Value: "parse-in-namespace-with-ns-labels-selector",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.namespace",
+ Value: "default",
+ Source: labels.LabelSourceK8s,
+ },
+ {
+ Key: "io.cilium.k8s.policy.uid",
+ Value: string(uuid),
+ Source: labels.LabelSourceK8s,
+ },
+ },
+ ),
+ },
{
// For a clusterwide policy the namespace is empty but when a to/fromEndpoint
// rule is added that represents a wildcard we add a match expression
--
2.45.2

0 comments on commit 9b7a870

Please sign in to comment.