Skip to content

Commit dde255b

Browse files
mattia-eleuterimattia-eleuteri
committed
fix(securitygroups): project a membership-only group to a policy Cilium accepts
A SecurityGroup with attachments and no rules still needs its backing CiliumNetworkPolicy, which carries the attachments the controller reconciles. Its spec held only an empty ingress list: the CRD anyOf accepts that, but the agent's api.Rule.Sanitize counts entries, so Cilium 1.19 marks the policy Valid=False. Write one empty ingress rule, which allows nothing, and disable default deny in both directions so the rule does not isolate the member pods. The policy is then valid and still changes nothing in the datapath. The reverse projection recognises that exact spec, enableDefaultDeny included, so the group still reads back with no rules while a tenant-authored empty ingress rule keeps its default deny and stays visible. Fixes #5129 Assisted-by: LLM Signed-off-by: Mattia Eleuteri <[email protected]>
1 parent b366286 commit dde255b

4 files changed

Lines changed: 194 additions & 75 deletions

File tree

‎pkg/apis/sdn/DESIGN.md‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,21 @@ spec:
130130

131131
The securitygroup-controller stamps `securitygroup.sdn.cozystack.io/db: ""` onto the pods of the attached `Postgres/db` application, which is what makes the policy's `endpointSelector` select them.
132132

133-
A **membership-only SecurityGroup** — one with `attachments` but no `ingress`/`egress` rules — still projects to a backing CiliumNetworkPolicy, because the controller reconciles the policy (not the SecurityGroup) and the attachments live on it. The CiliumNetworkPolicy CRD requires at least one rule section to be present (its `spec` `anyOf` is `ingress | ingressDeny | egress | egressDeny`), so the projection always emits the `ingress` section, an empty list `ingress: []` when the group has no ingress rules. An empty list is the CRD's documented no-op ("if omitted or empty, this rule does not apply at ingress") and `enableDefaultDeny` defaults to `false` for a direction with no rules, so the policy is schema-valid yet datapath-inert: it neither allows nor denies anything, consistent with §7. The empty section is hidden from the SecurityGroup view on read, so a membership-only group round-trips with no `ingress`/`egress`.
133+
A **membership-only SecurityGroup** — one with `attachments` but no `ingress`/`egress` rules — still projects to a backing CiliumNetworkPolicy, because the controller reconciles the policy (not the SecurityGroup) and the attachments live on it. Cilium rejects a policy none of whose rule sections has an entry: the CRD `anyOf` (`ingress | ingressDeny | egress | egressDeny`) is satisfied by an empty list, but the agent's `api.Rule.Sanitize` counts entries and marks such a policy `Valid=False`. The projection therefore writes one empty ingress rule, which allows nothing, together with `enableDefaultDeny: {ingress: false, egress: false}`, so that the rule does not put the member pods into default-deny either:
134+
135+
```yaml
136+
spec:
137+
endpointSelector:
138+
matchLabels:
139+
securitygroup.sdn.cozystack.io/db: ""
140+
ingress:
141+
- {}
142+
enableDefaultDeny:
143+
ingress: false
144+
egress: false
145+
```
146+
147+
The policy is valid yet datapath-inert: it neither allows nor denies anything, consistent with §7. The override depends on the Cilium agent's `enable-non-default-deny-policies` option, which defaults to `true`; with it off, Cilium forces default deny on every policy. The placeholder is hidden from the SecurityGroup view on read, so a membership-only group round-trips with no `ingress`/`egress`. `enableDefaultDeny` is what tells it apart from a group whose single ingress rule is genuinely empty, which projects without the override and so keeps Cilium's ingress default deny.
134148

135149
## 6. RBAC
136150

‎pkg/registry/sdn/securitygroup/cilium.go‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -44,25 +44,24 @@ type CiliumNetworkPolicySpec struct {
4444
// it to the SecurityGroup's own membership label.
4545
EndpointSelector metav1.LabelSelector `json:"endpointSelector,omitempty"`
4646

47-
// Ingress is the list of allowed inbound traffic rules. It is always
48-
// serialized (an empty list, not omitted) and the projection guarantees it is
49-
// non-nil. The backing CiliumNetworkPolicy CRD requires at least one rule
50-
// section to be present (spec anyOf: ingress|ingressDeny|egress|egressDeny),
51-
// so a rules-less (membership-only) SecurityGroup would otherwise project to a
52-
// selector-only spec the cilium.io/v2 API rejects synchronously. An empty
53-
// ingress list is the CRD's documented no-op ("if omitted or empty, this rule
54-
// does not apply at ingress"), and enableDefaultDeny defaults to false for a
55-
// direction with no rules, so the always-present list keeps the policy
56-
// schema-valid while leaving the member pods' connectivity untouched. The
57-
// non-nil guarantee matters because the CRD has no nullable fields: a nil
58-
// slice without omitempty would marshal to null and be rejected.
59-
Ingress []CiliumIngressRule `json:"ingress"`
60-
61-
// Egress is the list of allowed outbound traffic rules. Unlike Ingress it
62-
// keeps omitempty: ingress is the guaranteed-present carrier that satisfies the
63-
// CRD anyOf in every case, so egress is emitted only when there are egress
64-
// rules.
47+
// Ingress is the list of allowed inbound traffic rules.
48+
Ingress []CiliumIngressRule `json:"ingress,omitempty"`
49+
50+
// Egress is the list of allowed outbound traffic rules.
6551
Egress []CiliumEgressRule `json:"egress,omitempty"`
52+
53+
// EnableDefaultDeny overrides Cilium's per-direction default-deny posture.
54+
// The projection sets it only for a membership-only SecurityGroup; see
55+
// membershipOnlySpec.
56+
EnableDefaultDeny *CiliumDefaultDeny `json:"enableDefaultDeny,omitempty"`
57+
}
58+
59+
// CiliumDefaultDeny mirrors the cilium.io/v2 enableDefaultDeny block. A nil
60+
// direction leaves Cilium's default in place: deny for a direction that has
61+
// rules, allow otherwise.
62+
type CiliumDefaultDeny struct {
63+
Ingress *bool `json:"ingress,omitempty"`
64+
Egress *bool `json:"egress,omitempty"`
6665
}
6766

6867
// CiliumIngressRule mirrors a single cilium.io/v2 ingress rule. fromApp/fromSG
@@ -170,6 +169,26 @@ func (in *CiliumNetworkPolicySpec) DeepCopyInto(out *CiliumNetworkPolicySpec) {
170169
in.Egress[i].DeepCopyInto(&out.Egress[i])
171170
}
172171
}
172+
if in.EnableDefaultDeny != nil {
173+
out.EnableDefaultDeny = in.EnableDefaultDeny.DeepCopy()
174+
}
175+
}
176+
177+
// DeepCopy returns a deep copy of the receiver.
178+
func (in *CiliumDefaultDeny) DeepCopy() *CiliumDefaultDeny {
179+
if in == nil {
180+
return nil
181+
}
182+
out := new(CiliumDefaultDeny)
183+
if in.Ingress != nil {
184+
v := *in.Ingress
185+
out.Ingress = &v
186+
}
187+
if in.Egress != nil {
188+
v := *in.Egress
189+
out.Egress = &v
190+
}
191+
return out
173192
}
174193

175194
// DeepCopy returns a deep copy of the receiver.

‎pkg/registry/sdn/securitygroup/rest.go‎

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -143,11 +143,10 @@ func sgFromSelector(sel metav1.LabelSelector) (string, bool) {
143143
// projectIngress turns the SecurityGroup ingress rules into Cilium ingress
144144
// rules: fromApp peers become lineage-label endpointSelectors, fromSG peers
145145
// become membership-label endpointSelectors, and fromCIDR/toPorts carry over.
146-
// It always returns a non-nil slice (empty when there are no rules): the backing
147-
// CiliumNetworkPolicy's ingress section must be present on the wire to satisfy
148-
// the CRD anyOf, and a non-nil empty slice serializes to an empty list rather
149-
// than null (which the CRD, having no nullable fields, would reject).
150146
func projectIngress(rules []sdnv1alpha1.IngressRule) []CiliumIngressRule {
147+
if rules == nil {
148+
return nil
149+
}
151150
out := make([]CiliumIngressRule, len(rules))
152151
for i := range rules {
153152
var eps []metav1.LabelSelector
@@ -195,9 +194,8 @@ func projectEgress(rules []sdnv1alpha1.EgressRule) []CiliumEgressRule {
195194
// the lineage-label nor the membership-label shape are ignored, so a
196195
// hand-edited backing policy degrades gracefully rather than erroring.
197196
func reconstructIngress(rules []CiliumIngressRule) []sdnv1alpha1.IngressRule {
198-
// Collapse an empty list to nil so the always-present empty ingress section the
199-
// projection emits for a rules-less group (to satisfy the CRD anyOf) is
200-
// invisible in the SecurityGroup view, keeping the rules-less round-trip exact.
197+
// Collapse an empty list to nil: policies written before membershipOnlySpec
198+
// carried an always-present empty ingress section.
201199
if len(rules) == 0 {
202200
return nil
203201
}
@@ -224,8 +222,8 @@ func reconstructIngress(rules []CiliumIngressRule) []sdnv1alpha1.IngressRule {
224222

225223
// reconstructEgress is the egress counterpart of reconstructIngress.
226224
func reconstructEgress(rules []CiliumEgressRule) []sdnv1alpha1.EgressRule {
227-
// Collapse an empty list to nil for the same reason as reconstructIngress, so a
228-
// SecurityGroup that carries no egress rules round-trips with egress unset.
225+
// Collapse an empty list to nil, so a SecurityGroup that carries no egress
226+
// rules round-trips with egress unset.
229227
if len(rules) == 0 {
230228
return nil
231229
}
@@ -409,8 +407,10 @@ func policyToSecurityGroup(np *CiliumNetworkPolicy) *sdnv1alpha1.SecurityGroup {
409407
// Attachments live in a storage-owned annotation, not the
410408
// endpointSelector (which is the SecurityGroup's own membership label).
411409
Attachments: decodeAttachments(np.Annotations[attachmentsAnnotation]),
412-
Ingress: reconstructIngress(spec.Ingress),
413-
Egress: reconstructEgress(spec.Egress),
410+
}
411+
if !isMembershipOnlySpec(spec) {
412+
sg.Spec.Ingress = reconstructIngress(spec.Ingress)
413+
sg.Spec.Egress = reconstructEgress(spec.Egress)
414414
}
415415
}
416416
return sg
@@ -485,6 +485,10 @@ func securityGroupToPolicy(sg *sdnv1alpha1.SecurityGroup, cur *CiliumNetworkPoli
485485
// label onto the attached applications' pods. Rules project app/SG peers into
486486
// endpoint selectors.
487487
spec := sg.Spec.DeepCopy()
488+
if len(spec.Ingress) == 0 && len(spec.Egress) == 0 {
489+
out.Spec = membershipOnlySpec(sg.Name)
490+
return &out
491+
}
488492
out.Spec = &CiliumNetworkPolicySpec{
489493
EndpointSelector: buildEndpointSelector(sg.Name),
490494
Ingress: projectIngress(spec.Ingress),
@@ -497,6 +501,41 @@ func securityGroupToPolicy(sg *sdnv1alpha1.SecurityGroup, cur *CiliumNetworkPoli
497501
return &out
498502
}
499503

504+
// membershipOnlySpec is the backing spec of a SecurityGroup with no rules. The
505+
// policy must still exist, since it carries the attachments the
506+
// securitygroup-controller reconciles, but Cilium rejects a rule with no
507+
// ingress, ingressDeny, egress or egressDeny entry (api.Rule.Sanitize counts
508+
// entries, so an empty list is as good as absent) and marks the policy
509+
// Valid=False. One empty ingress rule satisfies that check and allows nothing,
510+
// and disabling default deny in both directions keeps that rule from putting
511+
// the member pods into default-deny, so the policy changes nothing in the
512+
// datapath. That relies on enable-non-default-deny-policies, which defaults to
513+
// true; with it off Cilium forces default deny on every policy.
514+
func membershipOnlySpec(name string) *CiliumNetworkPolicySpec {
515+
off := false
516+
return &CiliumNetworkPolicySpec{
517+
EndpointSelector: buildEndpointSelector(name),
518+
Ingress: []CiliumIngressRule{{}},
519+
EnableDefaultDeny: &CiliumDefaultDeny{Ingress: &off, Egress: &off},
520+
}
521+
}
522+
523+
// isMembershipOnlySpec reports whether spec is what membershipOnlySpec writes,
524+
// so the reverse projection shows no rules for it. enableDefaultDeny tells it
525+
// apart from a SecurityGroup whose single ingress rule is genuinely empty,
526+
// which the projection writes without it, keeping ingress default deny.
527+
func isMembershipOnlySpec(spec *CiliumNetworkPolicySpec) bool {
528+
d := spec.EnableDefaultDeny
529+
if d == nil || d.Ingress == nil || *d.Ingress || d.Egress == nil || *d.Egress {
530+
return false
531+
}
532+
if len(spec.Egress) != 0 || len(spec.Ingress) != 1 {
533+
return false
534+
}
535+
in := spec.Ingress[0]
536+
return len(in.FromEndpoints) == 0 && len(in.FromCIDR) == 0 && len(in.ToPorts) == 0
537+
}
538+
500539
// normalizePortProtocols upper-cases every port protocol in place so the value
501540
// written to the backing CiliumNetworkPolicy matches its case-sensitive enum.
502541
func normalizePortProtocols(spec *CiliumNetworkPolicySpec) {

‎pkg/registry/sdn/securitygroup/rest_test.go‎

Lines changed: 92 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"context"
88
"encoding/json"
99
"errors"
10+
"fmt"
1011
"reflect"
1112
"sort"
1213
"strings"
@@ -1195,74 +1196,120 @@ func TestCreateAcceptsEmptyAttachments(t *testing.T) {
11951196
}
11961197
}
11971198

1199+
// specWire returns the backing policy's spec as Cilium decodes it: the CRD and
1200+
// the agent read the wire form, not the Go struct.
1201+
func specWire(t *testing.T, np *CiliumNetworkPolicy) map[string]any {
1202+
t.Helper()
1203+
raw, err := json.Marshal(np)
1204+
if err != nil {
1205+
t.Fatalf("marshal backing policy: %v", err)
1206+
}
1207+
var decoded struct {
1208+
Spec map[string]any `json:"spec"`
1209+
}
1210+
if err := json.Unmarshal(raw, &decoded); err != nil {
1211+
t.Fatalf("unmarshal backing policy: %v", err)
1212+
}
1213+
return decoded.Spec
1214+
}
1215+
1216+
// ciliumAcceptsRuleSections mirrors the first check of Cilium's
1217+
// api.Rule.Sanitize, which marks a policy Valid=False when none of its rule
1218+
// sections has an entry. The CRD anyOf only needs one of the keys to be
1219+
// present, so it admits an empty list that the agent then rejects.
1220+
func ciliumAcceptsRuleSections(spec map[string]any) bool {
1221+
for _, k := range []string{"ingress", "ingressDeny", "egress", "egressDeny"} {
1222+
if l, _ := spec[k].([]any); len(l) > 0 {
1223+
return true
1224+
}
1225+
}
1226+
return false
1227+
}
1228+
1229+
func getBackingPolicy(t *testing.T, r *REST, name string) *CiliumNetworkPolicy {
1230+
t.Helper()
1231+
np := &CiliumNetworkPolicy{}
1232+
if err := r.c.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: name}, np); err != nil {
1233+
t.Fatalf("backing policy not found: %v", err)
1234+
}
1235+
return np
1236+
}
1237+
11981238
func TestRulesLessSecurityGroupProjectsValidPolicy(t *testing.T) {
11991239
r := newTestREST(t)
12001240
// A membership-only SecurityGroup attaches to an application to stamp the
1201-
// membership label but carries no ingress or egress rules. The backing
1202-
// CiliumNetworkPolicy CRD still requires at least one rule section to be
1203-
// present (spec anyOf: ingress|ingressDeny|egress|egressDeny), so a
1204-
// selector-only spec is rejected synchronously by the cilium.io/v2 API. The
1205-
// projection must emit an explicit empty ingress list — the CRD's documented
1206-
// no-op ("if omitted or empty, this rule does not apply at ingress") — so the
1207-
// policy is schema-valid yet leaves the member pods' connectivity untouched.
1241+
// membership label but carries no ingress or egress rules. Its backing
1242+
// policy must be one Cilium accepts and that changes nothing in the
1243+
// datapath: a rule section with an entry that allows nothing, and default
1244+
// deny disabled in both directions so that entry does not isolate the
1245+
// member pods.
12081246
createSG(t, r, &sdnv1alpha1.SecurityGroup{
12091247
ObjectMeta: metav1.ObjectMeta{Name: "sg-membership", Namespace: testNamespace},
12101248
Spec: sdnv1alpha1.SecurityGroupSpec{
12111249
Attachments: []sdnv1alpha1.ApplicationReference{{Kind: "Postgres", Name: "db"}},
12121250
},
12131251
})
12141252

1215-
np := &CiliumNetworkPolicy{}
1216-
if err := r.c.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: "sg-membership"}, np); err != nil {
1217-
t.Fatalf("backing policy not found: %v", err)
1218-
}
1219-
1220-
// The CRD validates the wire form, not the Go struct, so assert on the
1221-
// marshaled bytes: spec.ingress must be present (an empty list satisfies the
1222-
// anyOf), and spec.egress must be absent (ingress is the guaranteed-present
1223-
// carrier; egress is emitted only when there are egress rules).
1224-
raw, err := json.Marshal(np)
1225-
if err != nil {
1226-
t.Fatalf("marshal backing policy: %v", err)
1253+
spec := specWire(t, getBackingPolicy(t, r, "sg-membership"))
1254+
if !ciliumAcceptsRuleSections(spec) {
1255+
t.Fatalf("rules-less policy has no rule entry, Cilium marks it Valid=False: %v", spec)
12271256
}
1228-
var decoded struct {
1229-
Spec map[string]json.RawMessage `json:"spec"`
1257+
if got, want := fmt.Sprint(spec["ingress"]), "[map[]]"; got != want {
1258+
t.Fatalf("rules-less policy ingress = %s, want %s (one rule that allows nothing)", got, want)
12301259
}
1231-
if err := json.Unmarshal(raw, &decoded); err != nil {
1232-
t.Fatalf("unmarshal backing policy: %v", err)
1260+
if _, ok := spec["egress"]; ok {
1261+
t.Fatalf("rules-less policy must not carry an egress key: %v", spec)
12331262
}
1234-
ing, ok := decoded.Spec["ingress"]
1235-
if !ok {
1236-
t.Fatalf("backing policy spec has no ingress key; the CRD anyOf would reject it: %s", raw)
1263+
if got, want := fmt.Sprint(spec["enableDefaultDeny"]), "map[egress:false ingress:false]"; got != want {
1264+
t.Fatalf("rules-less policy enableDefaultDeny = %s, want %s", got, want)
12371265
}
1238-
if string(ing) != "[]" {
1239-
t.Fatalf("rules-less policy ingress = %s, want []", ing)
1266+
1267+
// On read, the placeholder rule is hidden, so a rules-less SecurityGroup
1268+
// round-trips with neither ingress nor egress.
1269+
out, err := r.Get(ctxNS(), "sg-membership", &metav1.GetOptions{})
1270+
if err != nil {
1271+
t.Fatalf("Get returned error: %v", err)
12401272
}
1241-
if _, ok := decoded.Spec["egress"]; ok {
1242-
t.Fatalf("rules-less policy must not carry an egress key: %s", raw)
1273+
got := out.(*sdnv1alpha1.SecurityGroup)
1274+
if got.Spec.Ingress != nil || got.Spec.Egress != nil {
1275+
t.Fatalf("rules-less round-trip should carry no ingress/egress, got: %+v", got.Spec)
12431276
}
12441277

1245-
// The same key must survive the unstructured conversion path (which honors
1246-
// omitempty independently of json.Marshal), so the empty list cannot be
1247-
// silently dropped there either.
1248-
u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(np)
1249-
if err != nil {
1250-
t.Fatalf("to unstructured: %v", err)
1278+
// Adding a rule must drop the default-deny override, or the group's
1279+
// rules would allow traffic without isolating the member pods.
1280+
got.Spec.Egress = []sdnv1alpha1.EgressRule{{ToCIDR: []string{"10.0.0.0/8"}}}
1281+
if _, _, err := r.Update(ctxNS(), "sg-membership", rest.DefaultUpdatedObjectInfo(got), nil, nil, false, &metav1.UpdateOptions{}); err != nil {
1282+
t.Fatalf("Update returned error: %v", err)
12511283
}
1252-
spec, _ := u["spec"].(map[string]any)
1253-
if _, ok := spec["ingress"]; !ok {
1254-
t.Fatalf("unstructured backing policy spec has no ingress key: %v", spec)
1284+
spec = specWire(t, getBackingPolicy(t, r, "sg-membership"))
1285+
if _, ok := spec["enableDefaultDeny"]; ok {
1286+
t.Fatalf("policy with rules must not override default deny: %v", spec)
12551287
}
1288+
if _, ok := spec["ingress"]; ok {
1289+
t.Fatalf("egress-only policy must not carry an ingress key: %v", spec)
1290+
}
1291+
}
12561292

1257-
// On read, the synthetic empty ingress list collapses back to no rules, so a
1258-
// rules-less SecurityGroup round-trips with neither ingress nor egress.
1259-
out, err := r.Get(ctxNS(), "sg-membership", &metav1.GetOptions{})
1293+
func TestEmptyIngressRuleIsNotMembershipOnly(t *testing.T) {
1294+
r := newTestREST(t)
1295+
// An explicit empty ingress rule projects to the same rule as the
1296+
// membership-only placeholder, but it is tenant-authored: it keeps Cilium's
1297+
// ingress default deny and must stay visible on read.
1298+
createSG(t, r, &sdnv1alpha1.SecurityGroup{
1299+
ObjectMeta: metav1.ObjectMeta{Name: "sg-deny-in", Namespace: testNamespace},
1300+
Spec: sdnv1alpha1.SecurityGroupSpec{Ingress: []sdnv1alpha1.IngressRule{{}}},
1301+
})
1302+
1303+
spec := specWire(t, getBackingPolicy(t, r, "sg-deny-in"))
1304+
if _, ok := spec["enableDefaultDeny"]; ok {
1305+
t.Fatalf("an empty ingress rule must keep default deny: %v", spec)
1306+
}
1307+
out, err := r.Get(ctxNS(), "sg-deny-in", &metav1.GetOptions{})
12601308
if err != nil {
12611309
t.Fatalf("Get returned error: %v", err)
12621310
}
1263-
got := out.(*sdnv1alpha1.SecurityGroup)
1264-
if got.Spec.Ingress != nil || got.Spec.Egress != nil {
1265-
t.Fatalf("rules-less round-trip should carry no ingress/egress, got: %+v", got.Spec)
1311+
if got := out.(*sdnv1alpha1.SecurityGroup).Spec.Ingress; len(got) != 1 {
1312+
t.Fatalf("empty ingress rule should round-trip as one rule, got: %+v", got)
12661313
}
12671314
}
12681315

0 commit comments

Comments
 (0)