diff --git a/pkg/apis/sdn/DESIGN.md b/pkg/apis/sdn/DESIGN.md index e7fa62f95..017da9d9a 100644 --- a/pkg/apis/sdn/DESIGN.md +++ b/pkg/apis/sdn/DESIGN.md @@ -130,7 +130,21 @@ spec: 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. -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`. +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: + +```yaml +spec: + endpointSelector: + matchLabels: + securitygroup.sdn.cozystack.io/db: "" + ingress: + - {} + enableDefaultDeny: + ingress: false + egress: false +``` + +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. ## 6. RBAC diff --git a/pkg/registry/sdn/securitygroup/cilium.go b/pkg/registry/sdn/securitygroup/cilium.go index fa0ddb65a..906afcdcd 100644 --- a/pkg/registry/sdn/securitygroup/cilium.go +++ b/pkg/registry/sdn/securitygroup/cilium.go @@ -44,25 +44,24 @@ type CiliumNetworkPolicySpec struct { // it to the SecurityGroup's own membership label. EndpointSelector metav1.LabelSelector `json:"endpointSelector,omitempty"` - // Ingress is the list of allowed inbound traffic rules. It is always - // serialized (an empty list, not omitted) and the projection guarantees it is - // non-nil. The backing CiliumNetworkPolicy CRD requires at least one rule - // section to be present (spec anyOf: ingress|ingressDeny|egress|egressDeny), - // so a rules-less (membership-only) SecurityGroup would otherwise project to a - // selector-only spec the cilium.io/v2 API rejects synchronously. An empty - // ingress 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 always-present list keeps the policy - // schema-valid while leaving the member pods' connectivity untouched. The - // non-nil guarantee matters because the CRD has no nullable fields: a nil - // slice without omitempty would marshal to null and be rejected. - Ingress []CiliumIngressRule `json:"ingress"` + // Ingress is the list of allowed inbound traffic rules. + Ingress []CiliumIngressRule `json:"ingress,omitempty"` - // Egress is the list of allowed outbound traffic rules. Unlike Ingress it - // keeps omitempty: ingress is the guaranteed-present carrier that satisfies the - // CRD anyOf in every case, so egress is emitted only when there are egress - // rules. + // Egress is the list of allowed outbound traffic rules. Egress []CiliumEgressRule `json:"egress,omitempty"` + + // EnableDefaultDeny overrides Cilium's per-direction default-deny posture. + // The projection sets it only for a membership-only SecurityGroup; see + // membershipOnlySpec. + EnableDefaultDeny *CiliumDefaultDeny `json:"enableDefaultDeny,omitempty"` +} + +// CiliumDefaultDeny mirrors the cilium.io/v2 enableDefaultDeny block. A nil +// direction leaves Cilium's default in place: deny for a direction that has +// rules, allow otherwise. +type CiliumDefaultDeny struct { + Ingress *bool `json:"ingress,omitempty"` + Egress *bool `json:"egress,omitempty"` } // CiliumIngressRule mirrors a single cilium.io/v2 ingress rule. fromApp/fromSG @@ -170,6 +169,26 @@ func (in *CiliumNetworkPolicySpec) DeepCopyInto(out *CiliumNetworkPolicySpec) { in.Egress[i].DeepCopyInto(&out.Egress[i]) } } + if in.EnableDefaultDeny != nil { + out.EnableDefaultDeny = in.EnableDefaultDeny.DeepCopy() + } +} + +// DeepCopy returns a deep copy of the receiver. +func (in *CiliumDefaultDeny) DeepCopy() *CiliumDefaultDeny { + if in == nil { + return nil + } + out := new(CiliumDefaultDeny) + if in.Ingress != nil { + v := *in.Ingress + out.Ingress = &v + } + if in.Egress != nil { + v := *in.Egress + out.Egress = &v + } + return out } // DeepCopy returns a deep copy of the receiver. diff --git a/pkg/registry/sdn/securitygroup/rest.go b/pkg/registry/sdn/securitygroup/rest.go index a06439a75..d4a8d223d 100644 --- a/pkg/registry/sdn/securitygroup/rest.go +++ b/pkg/registry/sdn/securitygroup/rest.go @@ -143,11 +143,10 @@ func sgFromSelector(sel metav1.LabelSelector) (string, bool) { // projectIngress turns the SecurityGroup ingress rules into Cilium ingress // rules: fromApp peers become lineage-label endpointSelectors, fromSG peers // become membership-label endpointSelectors, and fromCIDR/toPorts carry over. -// It always returns a non-nil slice (empty when there are no rules): the backing -// CiliumNetworkPolicy's ingress section must be present on the wire to satisfy -// the CRD anyOf, and a non-nil empty slice serializes to an empty list rather -// than null (which the CRD, having no nullable fields, would reject). func projectIngress(rules []sdnv1alpha1.IngressRule) []CiliumIngressRule { + if rules == nil { + return nil + } out := make([]CiliumIngressRule, len(rules)) for i := range rules { var eps []metav1.LabelSelector @@ -195,9 +194,8 @@ func projectEgress(rules []sdnv1alpha1.EgressRule) []CiliumEgressRule { // the lineage-label nor the membership-label shape are ignored, so a // hand-edited backing policy degrades gracefully rather than erroring. func reconstructIngress(rules []CiliumIngressRule) []sdnv1alpha1.IngressRule { - // Collapse an empty list to nil so the always-present empty ingress section the - // projection emits for a rules-less group (to satisfy the CRD anyOf) is - // invisible in the SecurityGroup view, keeping the rules-less round-trip exact. + // Collapse an empty list to nil: policies written before membershipOnlySpec + // carried an always-present empty ingress section. if len(rules) == 0 { return nil } @@ -224,8 +222,8 @@ func reconstructIngress(rules []CiliumIngressRule) []sdnv1alpha1.IngressRule { // reconstructEgress is the egress counterpart of reconstructIngress. func reconstructEgress(rules []CiliumEgressRule) []sdnv1alpha1.EgressRule { - // Collapse an empty list to nil for the same reason as reconstructIngress, so a - // SecurityGroup that carries no egress rules round-trips with egress unset. + // Collapse an empty list to nil, so a SecurityGroup that carries no egress + // rules round-trips with egress unset. if len(rules) == 0 { return nil } @@ -409,8 +407,10 @@ func policyToSecurityGroup(np *CiliumNetworkPolicy) *sdnv1alpha1.SecurityGroup { // Attachments live in a storage-owned annotation, not the // endpointSelector (which is the SecurityGroup's own membership label). Attachments: decodeAttachments(np.Annotations[attachmentsAnnotation]), - Ingress: reconstructIngress(spec.Ingress), - Egress: reconstructEgress(spec.Egress), + } + if !isMembershipOnlySpec(spec) { + sg.Spec.Ingress = reconstructIngress(spec.Ingress) + sg.Spec.Egress = reconstructEgress(spec.Egress) } } return sg @@ -485,6 +485,10 @@ func securityGroupToPolicy(sg *sdnv1alpha1.SecurityGroup, cur *CiliumNetworkPoli // label onto the attached applications' pods. Rules project app/SG peers into // endpoint selectors. spec := sg.Spec.DeepCopy() + if len(spec.Ingress) == 0 && len(spec.Egress) == 0 { + out.Spec = membershipOnlySpec(sg.Name) + return &out + } out.Spec = &CiliumNetworkPolicySpec{ EndpointSelector: buildEndpointSelector(sg.Name), Ingress: projectIngress(spec.Ingress), @@ -497,6 +501,41 @@ func securityGroupToPolicy(sg *sdnv1alpha1.SecurityGroup, cur *CiliumNetworkPoli return &out } +// membershipOnlySpec is the backing spec of a SecurityGroup with no rules. The +// policy must still exist, since it carries the attachments the +// securitygroup-controller reconciles, but Cilium rejects a rule with no +// ingress, ingressDeny, egress or egressDeny entry (api.Rule.Sanitize counts +// entries, so an empty list is as good as absent) and marks the policy +// Valid=False. One empty ingress rule satisfies that check and allows nothing, +// and disabling default deny in both directions keeps that rule from putting +// the member pods into default-deny, so the policy changes nothing in the +// datapath. That relies on enable-non-default-deny-policies, which defaults to +// true; with it off Cilium forces default deny on every policy. +func membershipOnlySpec(name string) *CiliumNetworkPolicySpec { + off := false + return &CiliumNetworkPolicySpec{ + EndpointSelector: buildEndpointSelector(name), + Ingress: []CiliumIngressRule{{}}, + EnableDefaultDeny: &CiliumDefaultDeny{Ingress: &off, Egress: &off}, + } +} + +// isMembershipOnlySpec reports whether spec is what membershipOnlySpec writes, +// so the reverse projection shows no rules for it. enableDefaultDeny tells it +// apart from a SecurityGroup whose single ingress rule is genuinely empty, +// which the projection writes without it, keeping ingress default deny. +func isMembershipOnlySpec(spec *CiliumNetworkPolicySpec) bool { + d := spec.EnableDefaultDeny + if d == nil || d.Ingress == nil || *d.Ingress || d.Egress == nil || *d.Egress { + return false + } + if len(spec.Egress) != 0 || len(spec.Ingress) != 1 { + return false + } + in := spec.Ingress[0] + return len(in.FromEndpoints) == 0 && len(in.FromCIDR) == 0 && len(in.ToPorts) == 0 +} + // normalizePortProtocols upper-cases every port protocol in place so the value // written to the backing CiliumNetworkPolicy matches its case-sensitive enum. func normalizePortProtocols(spec *CiliumNetworkPolicySpec) { diff --git a/pkg/registry/sdn/securitygroup/rest_test.go b/pkg/registry/sdn/securitygroup/rest_test.go index 5473dfd56..f3ad68fc3 100644 --- a/pkg/registry/sdn/securitygroup/rest_test.go +++ b/pkg/registry/sdn/securitygroup/rest_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "reflect" "sort" "strings" @@ -1195,16 +1196,53 @@ func TestCreateAcceptsEmptyAttachments(t *testing.T) { } } +// specWire returns the backing policy's spec as Cilium decodes it: the CRD and +// the agent read the wire form, not the Go struct. +func specWire(t *testing.T, np *CiliumNetworkPolicy) map[string]any { + t.Helper() + raw, err := json.Marshal(np) + if err != nil { + t.Fatalf("marshal backing policy: %v", err) + } + var decoded struct { + Spec map[string]any `json:"spec"` + } + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatalf("unmarshal backing policy: %v", err) + } + return decoded.Spec +} + +// ciliumAcceptsRuleSections mirrors the first check of Cilium's +// api.Rule.Sanitize, which marks a policy Valid=False when none of its rule +// sections has an entry. The CRD anyOf only needs one of the keys to be +// present, so it admits an empty list that the agent then rejects. +func ciliumAcceptsRuleSections(spec map[string]any) bool { + for _, k := range []string{"ingress", "ingressDeny", "egress", "egressDeny"} { + if l, _ := spec[k].([]any); len(l) > 0 { + return true + } + } + return false +} + +func getBackingPolicy(t *testing.T, r *REST, name string) *CiliumNetworkPolicy { + t.Helper() + np := &CiliumNetworkPolicy{} + if err := r.c.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: name}, np); err != nil { + t.Fatalf("backing policy not found: %v", err) + } + return np +} + func TestRulesLessSecurityGroupProjectsValidPolicy(t *testing.T) { r := newTestREST(t) // A membership-only SecurityGroup attaches to an application to stamp the - // membership label but carries no ingress or egress rules. The backing - // CiliumNetworkPolicy CRD still requires at least one rule section to be - // present (spec anyOf: ingress|ingressDeny|egress|egressDeny), so a - // selector-only spec is rejected synchronously by the cilium.io/v2 API. The - // projection must emit an explicit empty ingress list — the CRD's documented - // no-op ("if omitted or empty, this rule does not apply at ingress") — so the - // policy is schema-valid yet leaves the member pods' connectivity untouched. + // membership label but carries no ingress or egress rules. Its backing + // policy must be one Cilium accepts and that changes nothing in the + // datapath: a rule section with an entry that allows nothing, and default + // deny disabled in both directions so that entry does not isolate the + // member pods. createSG(t, r, &sdnv1alpha1.SecurityGroup{ ObjectMeta: metav1.ObjectMeta{Name: "sg-membership", Namespace: testNamespace}, Spec: sdnv1alpha1.SecurityGroupSpec{ @@ -1212,50 +1250,22 @@ func TestRulesLessSecurityGroupProjectsValidPolicy(t *testing.T) { }, }) - np := &CiliumNetworkPolicy{} - if err := r.c.Get(context.Background(), types.NamespacedName{Namespace: testNamespace, Name: "sg-membership"}, np); err != nil { - t.Fatalf("backing policy not found: %v", err) + spec := specWire(t, getBackingPolicy(t, r, "sg-membership")) + if !ciliumAcceptsRuleSections(spec) { + t.Fatalf("rules-less policy has no rule entry, Cilium marks it Valid=False: %v", spec) + } + if got, want := fmt.Sprint(spec["ingress"]), "[map[]]"; got != want { + t.Fatalf("rules-less policy ingress = %s, want %s (one rule that allows nothing)", got, want) + } + if _, ok := spec["egress"]; ok { + t.Fatalf("rules-less policy must not carry an egress key: %v", spec) + } + if got, want := fmt.Sprint(spec["enableDefaultDeny"]), "map[egress:false ingress:false]"; got != want { + t.Fatalf("rules-less policy enableDefaultDeny = %s, want %s", got, want) } - // The CRD validates the wire form, not the Go struct, so assert on the - // marshaled bytes: spec.ingress must be present (an empty list satisfies the - // anyOf), and spec.egress must be absent (ingress is the guaranteed-present - // carrier; egress is emitted only when there are egress rules). - raw, err := json.Marshal(np) - if err != nil { - t.Fatalf("marshal backing policy: %v", err) - } - var decoded struct { - Spec map[string]json.RawMessage `json:"spec"` - } - if err := json.Unmarshal(raw, &decoded); err != nil { - t.Fatalf("unmarshal backing policy: %v", err) - } - ing, ok := decoded.Spec["ingress"] - if !ok { - t.Fatalf("backing policy spec has no ingress key; the CRD anyOf would reject it: %s", raw) - } - if string(ing) != "[]" { - t.Fatalf("rules-less policy ingress = %s, want []", ing) - } - if _, ok := decoded.Spec["egress"]; ok { - t.Fatalf("rules-less policy must not carry an egress key: %s", raw) - } - - // The same key must survive the unstructured conversion path (which honors - // omitempty independently of json.Marshal), so the empty list cannot be - // silently dropped there either. - u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(np) - if err != nil { - t.Fatalf("to unstructured: %v", err) - } - spec, _ := u["spec"].(map[string]any) - if _, ok := spec["ingress"]; !ok { - t.Fatalf("unstructured backing policy spec has no ingress key: %v", spec) - } - - // On read, the synthetic empty ingress list collapses back to no rules, so a - // rules-less SecurityGroup round-trips with neither ingress nor egress. + // On read, the placeholder rule is hidden, so a rules-less SecurityGroup + // round-trips with neither ingress nor egress. out, err := r.Get(ctxNS(), "sg-membership", &metav1.GetOptions{}) if err != nil { t.Fatalf("Get returned error: %v", err) @@ -1264,6 +1274,43 @@ func TestRulesLessSecurityGroupProjectsValidPolicy(t *testing.T) { if got.Spec.Ingress != nil || got.Spec.Egress != nil { t.Fatalf("rules-less round-trip should carry no ingress/egress, got: %+v", got.Spec) } + + // Adding a rule must drop the default-deny override, or the group's + // rules would allow traffic without isolating the member pods. + got.Spec.Egress = []sdnv1alpha1.EgressRule{{ToCIDR: []string{"10.0.0.0/8"}}} + if _, _, err := r.Update(ctxNS(), "sg-membership", rest.DefaultUpdatedObjectInfo(got), nil, nil, false, &metav1.UpdateOptions{}); err != nil { + t.Fatalf("Update returned error: %v", err) + } + spec = specWire(t, getBackingPolicy(t, r, "sg-membership")) + if _, ok := spec["enableDefaultDeny"]; ok { + t.Fatalf("policy with rules must not override default deny: %v", spec) + } + if _, ok := spec["ingress"]; ok { + t.Fatalf("egress-only policy must not carry an ingress key: %v", spec) + } +} + +func TestEmptyIngressRuleIsNotMembershipOnly(t *testing.T) { + r := newTestREST(t) + // An explicit empty ingress rule projects to the same rule as the + // membership-only placeholder, but it is tenant-authored: it keeps Cilium's + // ingress default deny and must stay visible on read. + createSG(t, r, &sdnv1alpha1.SecurityGroup{ + ObjectMeta: metav1.ObjectMeta{Name: "sg-deny-in", Namespace: testNamespace}, + Spec: sdnv1alpha1.SecurityGroupSpec{Ingress: []sdnv1alpha1.IngressRule{{}}}, + }) + + spec := specWire(t, getBackingPolicy(t, r, "sg-deny-in")) + if _, ok := spec["enableDefaultDeny"]; ok { + t.Fatalf("an empty ingress rule must keep default deny: %v", spec) + } + out, err := r.Get(ctxNS(), "sg-deny-in", &metav1.GetOptions{}) + if err != nil { + t.Fatalf("Get returned error: %v", err) + } + if got := out.(*sdnv1alpha1.SecurityGroup).Spec.Ingress; len(got) != 1 { + t.Fatalf("empty ingress rule should round-trip as one rule, got: %+v", got) + } } func TestCreateRejectsInvalidAttachmentLabelValue(t *testing.T) {