fix(securitygroups): project a membership-only group to a policy Cilium accepts (#5133)

## What this PR does Fixes #5129. A SecurityGroup with `attachments` and no rules still projects to a backing CiliumNetworkPolicy, since that policy carries the attachments the securitygroup-controller reconciles. Its spec was `endpointSelector` plus `ingress: []`. The CRD `anyOf` accepts that, but the agent's `api.Rule.Sanitize` counts entries (`len(r.Ingress) == 0 && ...`, the same in v1.19.5 and v1.20.2), so Cilium marks the policy `Valid=False` with "rule must have at least one of Ingress, IngressDeny, Egress, EgressDeny". `pkg/apis/sdn/DESIGN.md` §5 claimed the opposite. The projection now writes one empty ingress rule, which allows nothing, plus `enableDefaultDeny: {ingress: false, egress: false}`, so that rule does not put the member pods into default deny. The override relies on the agent's `enable-non-default-deny-policies`, which defaults to `true` in the vendored chart; DESIGN.md says so. `ingress` is now `omitempty` like `egress`, because an empty list no longer serves any purpose. The reverse projection hides that exact spec, so the group still reads back with no rules. `enableDefaultDeny` is what tells it apart from a tenant-authored empty ingress rule, which keeps its default deny and stays visible. `TestRulesLessSecurityGroupProjectsValidPolicy` now checks the wire form against the Sanitize rule and fails on `main` (`ingress:[]`). `TestEmptyIngressRuleIsNotMembershipOnly` covers the tenant-authored empty rule. `go test ./pkg/registry/sdn/...` passes. Checked on a lab running Cozystack v1.6.5 with Cilium 1.19.5 (`enable-non-default-deny-policies: true`), in a throw-away namespace with no baseline policy. A membership-only group `front` attached to `web`, and a group `db` on `srv` allowing `fromSG: [front]` on 8080. The v1.6.5 projection gives `front` as `VALID=False`. With the spec this branch writes for `front` applied to its CNP, it turns `VALID=True` ("Policy validation succeeded"), and every probe is unchanged: `web -> srv:8080` OK, `web -> srv:9090` and `other -> srv:8080` refused, `other -> web:8080` and `web -> other:8080` OK, so the placeholder isolates nothing. Moving the attachment of `front` from `web` to `other` flips the `fromSG` peer: `other -> srv:8080` OK, `web -> srv:8080` refused. ### Screenshots  ### Downstream repositories The diff touches `pkg/registry/sdn/securitygroup/` and `pkg/apis/sdn/DESIGN.md` only: no package, chart, CRD, `hack/` or convention change. The website mentions SecurityGroups only in the v1.6 blog post and the PCI-DSS page, neither of which describes the backing policy.  - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note  ```release-note fix(securitygroups): a SecurityGroup with attachments and no rules no longer leaves an invalid CiliumNetworkPolicy behind; its backing policy is now accepted by Cilium and still allows or denies nothing. ```   ## Summary by CodeRabbit * **Bug Fixes** * Membership-only SecurityGroups now round-trip without displaying ingress or egress rules, while remaining usable without triggering default-deny behavior. * SecurityGroups with an explicitly configured empty ingress rule continue to retain that rule when read back. * This behavior depends on Cilium’s `enable-non-default-deny-policies` option being enabled; when disabled, Cilium applies default deny to every policy.
2026-11-09 22:13:18 +09:00 · 2026-11-09 12:05:32 +03:00
commit 72ff22dc9e
@@ -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

@@ -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.
@@ -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) {
@@ -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) {