feat(api): add SecurityGroup network policy resource (sdn.cozystack.io) - #2922
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces ChangesSDN SecurityGroup API
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIServer as Aggregated APIServer
participant REST
participant Cilium as CiliumNetworkPolicy (backing)
rect rgba(100, 149, 237, 0.5)
Note over Client,Cilium: Create: SecurityGroup → CiliumNetworkPolicy + marker
Client->>APIServer: POST /securitygroups (no marker in labels)
APIServer->>REST: Create(SecurityGroup)
REST->>REST: toPolicy() — inject sdn.cozystack.io/securitygroup=true
REST->>Cilium: Create CiliumNetworkPolicy with marker label
Cilium-->>REST: created policy
REST->>REST: toSecurityGroup() — strip marker from view
REST-->>APIServer: SecurityGroup (no marker exposed)
APIServer-->>Client: 201 Created
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Cilium: Get & List: marker-enforced selector filtering
Client->>APIServer: GET /securitygroups
APIServer->>REST: List(opts)
REST->>REST: Require marker label in selector
REST->>Cilium: List (enforce marker-labeled selector)
Cilium-->>REST: Marked policy only
REST->>REST: Project to SecurityGroup
REST-->>APIServer: SecurityGroupList
APIServer-->>Client: items (marker stripped)
end
rect rgba(255, 165, 0, 0.5)
Note over Client,Cilium: Update: reassert marker label on every write
Client->>APIServer: PATCH /securitygroups/sg1 (user labels)
APIServer->>REST: Update(sg1, spec)
REST->>Cilium: Get current policy
Cilium-->>REST: existing policy with marker
REST->>REST: toPolicy() — copy user fields, reassert marker=true
REST->>Cilium: Update CiliumNetworkPolicy
Cilium-->>REST: updated
REST-->>APIServer: SecurityGroup
APIServer-->>Client: 200 OK (marker preserved)
end
rect rgba(173, 216, 230, 0.5)
Note over Client,Cilium: Watch: filter by marker, project events
Client->>APIServer: GET /securitygroups?watch=true
APIServer->>REST: Watch(opts)
REST->>Cilium: Watch CiliumNetworkPolicies (marker selector required)
Cilium-->>REST: Added (unmarked platform policy)
REST->>REST: isSecurityGroup()? → false, filter out
Cilium-->>REST: Added (marked user policy)
REST->>REST: isSecurityGroup()? → true, project to SecurityGroup
REST-->>APIServer: watch.Added SecurityGroup
APIServer-->>Client: watch event (no marker)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/apis/sdn/v1alpha1/register.go (1)
16-21: ⚡ Quick winAvoid duplicating API group literals across packages.
GroupNameis duplicated here and inpkg/apis/sdn/register.go; centralizing it to the parent package avoids future contract drift between scheme registration layers.Proposed refactor
package v1alpha1 import ( + sdn "github.com/cozystack/cozystack/pkg/apis/sdn" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" ) @@ -const GroupName = "sdn.cozystack.io" +const GroupName = sdn.GroupName🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/sdn/v1alpha1/register.go` around lines 16 - 21, The GroupName constant is duplicated between the v1alpha1 subpackage and its parent package, which can lead to drift between the two definitions. Remove the GroupName constant definition from this location (in the v1alpha1/register.go file) and instead import GroupName from the parent sdn package. Then update the SchemeGroupVersion variable to reference the imported GroupName from the parent package to maintain a single source of truth for the API group identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/sdn/securitygroup/rest.go`:
- Around line 501-520: The Watch method in the REST struct does not apply field
selector filtering from opts.FieldSelector, causing watch requests with field
selectors (like fieldSelector=metadata.name=sg-a) to stream all matching objects
instead of filtering by the specified field. Update the Watch method to parse
and apply opts.FieldSelector similar to how the List method handles it. Pass the
field selector to the watch operation by including it in the client.ListOptions
(specifically in the Raw metav1.ListOptions field) or by filtering the streamed
events appropriately to respect the field selector criteria.
- Around line 389-390: The CreateOptions at line 389 and UpdateOptions at line
488 are being created fresh, which drops important caller intent such as dryRun,
fieldManager, and fieldValidation. This causes semantic issues where dry-run
requests can become real writes. Instead of creating fresh options objects,
preserve the original request options in both locations—examine how the correct
pattern is implemented at lines 401 and 477, and apply that same approach to
both the Create call at line 389 and the Update call at line 488 to maintain
fieldManager attribution and respect dry-run semantics.
- Around line 514-524: The AllowWatchBookmarks field in the metav1.ListOptions
(within the Watch method call) is incorrectly being set to the sendInitialEvents
value, which ties these two independent Kubernetes API features together.
Instead, extract AllowWatchBookmarks independently from opts similar to how
SendInitialEvents is extracted at the beginning (check if
opts.AllowWatchBookmarks is not nil and dereference it), and use that separate
boolean value when setting AllowWatchBookmarks in the metav1.ListOptions
structure rather than reusing the sendInitialEvents variable.
---
Nitpick comments:
In `@pkg/apis/sdn/v1alpha1/register.go`:
- Around line 16-21: The GroupName constant is duplicated between the v1alpha1
subpackage and its parent package, which can lead to drift between the two
definitions. Remove the GroupName constant definition from this location (in the
v1alpha1/register.go file) and instead import GroupName from the parent sdn
package. Then update the SchemeGroupVersion variable to reference the imported
GroupName from the parent package to maintain a single source of truth for the
API group identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f0630d3-9a26-46c9-815d-04af3a715ed8
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (24)
api/api-rules/cozystack_api_violation_exceptions.listpackages/system/cozystack-api/templates/apiservice.yamlpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-api/tests/rbac_test.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/cozystack-basics/tests/clusterroles-options_test.yamlpkg/apis/sdn/DESIGN.mdpkg/apis/sdn/fuzzer/fuzzer.gopkg/apis/sdn/install/install.gopkg/apis/sdn/install/roundtrip_test.gopkg/apis/sdn/register.gopkg/apis/sdn/v1alpha1/doc.gopkg/apis/sdn/v1alpha1/register.gopkg/apis/sdn/v1alpha1/securitygroup_types.gopkg/apis/sdn/v1alpha1/zz_generated.conversion.gopkg/apis/sdn/v1alpha1/zz_generated.deepcopy.gopkg/apis/sdn/v1alpha1/zz_generated.defaults.gopkg/apiserver/apiserver.gopkg/apiserver/scheme_test.gopkg/cmd/server/start.gopkg/registry/sdn/securitygroup/cilium.gopkg/registry/sdn/securitygroup/rest.gopkg/registry/sdn/securitygroup/rest_test.gopkg/registry/sdn/securitygroup/rest_watch_test.go
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new SecurityGroup API resource that allows tenants to manage their own network policies in a namespace-scoped manner. By projecting these resources 1:1 onto CiliumNetworkPolicy objects via the aggregated API server, it provides a secure, self-service firewall interface without requiring tenants to have direct access to the underlying cilium.io API group. The implementation is stateless and synchronous, ensuring efficient policy management while maintaining strict isolation through marker-label scoping. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the SecurityGroup resource under the new sdn.cozystack.io/v1alpha1 API group, implementing a namespace-scoped projection over CiliumNetworkPolicies. Feedback focuses on critical API server integration issues: using nsFrom(ctx) in listing and watching methods breaks cluster-wide operations, which can be resolved by using request.NamespaceValue(ctx). Additionally, OwnerReferences and Finalizers should be projected to preserve Kubernetes garbage collection and finalizer behaviors. The Port field should use intstr.IntOrString instead of string to support unquoted port numbers, and the custom Patch method can be removed as it is dead code under the generic apiserver PATCH handler. Finally, adding a unit test for cluster-wide listing is recommended.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ns, err := nsFrom(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Using nsFrom(ctx) in the List method forces a namespace to be present in the context. This completely breaks cluster-wide listing (e.g., kubectl get securitygroups -A), returning a 400 Bad Request instead. To support cluster-wide listing, retrieve the namespace using request.NamespaceValue(ctx), which correctly returns an empty string when listing across all namespaces.
| ns, err := nsFrom(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ns := request.NamespaceValue(ctx) |
There was a problem hiding this comment.
Fixed in 43e079e66 — List now uses request.NamespaceValue(ctx), which returns an empty namespace for a cluster-wide request, so kubectl get securitygroups -A lists across namespaces. Added TestListClusterWide.
| ns, err := nsFrom(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Similar to the List method, using nsFrom(ctx) in Watch breaks cluster-wide watching (e.g., kubectl get securitygroups -A -w). Use request.NamespaceValue(ctx) instead to allow watching across all namespaces.
| ns, err := nsFrom(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ns := request.NamespaceValue(ctx) |
There was a problem hiding this comment.
Fixed in 43e079e66 — Watch now uses request.NamespaceValue(ctx) for the same reason, enabling cluster-wide watch. Added TestWatchClusterWide.
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: np.Name, | ||
| Namespace: np.Namespace, | ||
| UID: np.UID, | ||
| ResourceVersion: np.ResourceVersion, | ||
| CreationTimestamp: np.CreationTimestamp, | ||
| Labels: stripInternal(np.Labels), | ||
| Annotations: np.Annotations, | ||
| }, |
There was a problem hiding this comment.
Currently, OwnerReferences and Finalizers are not projected from the backing CiliumNetworkPolicy to the SecurityGroup resource. This breaks Kubernetes garbage collection and controller finalizers for SecurityGroup resources. Project them 1:1 to ensure correct behavior.
ObjectMeta: metav1.ObjectMeta{
Name: np.Name,
Namespace: np.Namespace,
UID: np.UID,
ResourceVersion: np.ResourceVersion,
CreationTimestamp: np.CreationTimestamp,
Labels: stripInternal(np.Labels),
Annotations: np.Annotations,
OwnerReferences: np.OwnerReferences,
Finalizers: np.Finalizers,
},There was a problem hiding this comment.
Fixed in 43e079e66 — policyToSecurityGroup now projects OwnerReferences and Finalizers from the backing CiliumNetworkPolicy. Covered by TestOwnerReferencesAndFinalizersRoundTrip.
| if len(sg.Annotations) > 0 { | ||
| out.Annotations = make(map[string]string, len(sg.Annotations)) | ||
| for k, v := range sg.Annotations { | ||
| out.Annotations[k] = v | ||
| } | ||
| } else { | ||
| out.Annotations = nil | ||
| } | ||
|
|
||
| out.Spec = sg.Spec.DeepCopy() | ||
| return &out |
There was a problem hiding this comment.
Project OwnerReferences and Finalizers from the SecurityGroup resource to the backing CiliumNetworkPolicy during creation and updates to ensure garbage collection and finalizers are correctly propagated.
if len(sg.Annotations) > 0 {
out.Annotations = make(map[string]string, len(sg.Annotations))
for k, v := range sg.Annotations {
out.Annotations[k] = v
}
} else {
out.Annotations = nil
}
out.OwnerReferences = sg.OwnerReferences
out.Finalizers = sg.Finalizers
out.Spec = sg.Spec.DeepCopy()There was a problem hiding this comment.
Fixed in 43e079e66 — securityGroupToPolicy now projects OwnerReferences and Finalizers onto the backing policy with replace semantics (like labels/annotations), so garbage collection and finalizers work. Covered by TestOwnerReferencesAndFinalizersRoundTrip.
|
|
||
| package v1alpha1 | ||
|
|
||
| import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
There was a problem hiding this comment.
Import k8s.io/apimachinery/pkg/util/intstr to support the intstr.IntOrString type for the Port field.
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)There was a problem hiding this comment.
Not adding this import — Port stays a string (see the PortProtocol thread), so intstr is not needed.
| // PortProtocol is a single port and protocol pair. | ||
| type PortProtocol struct { | ||
| // Port is the L4 port number as a string, or a named port. An empty value | ||
| // matches all ports. | ||
| Port string `json:"port,omitempty"` | ||
|
|
||
| // Protocol is the L4 protocol. One of TCP, UDP, SCTP or ANY. Defaults to ANY | ||
| // when empty. | ||
| Protocol string `json:"protocol,omitempty"` | ||
| } |
There was a problem hiding this comment.
Using string for the Port field means that any unquoted port numbers (e.g., port: 5432) in the YAML input will fail to unmarshal, forcing users to always quote port numbers (e.g., port: "5432"). To align with standard Kubernetes conventions and prevent unmarshaling errors, consider using intstr.IntOrString instead.
| // PortProtocol is a single port and protocol pair. | |
| type PortProtocol struct { | |
| // Port is the L4 port number as a string, or a named port. An empty value | |
| // matches all ports. | |
| Port string `json:"port,omitempty"` | |
| // Protocol is the L4 protocol. One of TCP, UDP, SCTP or ANY. Defaults to ANY | |
| // when empty. | |
| Protocol string `json:"protocol,omitempty"` | |
| } | |
| // PortProtocol is a single port and protocol pair. | |
| type PortProtocol struct { | |
| // Port is the L4 port number as an integer or a named port. An empty value | |
| // matches all ports. | |
| Port intstr.IntOrString `json:"port,omitempty"` | |
| // Protocol is the L4 protocol. One of TCP, UDP, SCTP or ANY. Defaults to ANY | |
| // when empty. | |
| Protocol string `json:"protocol,omitempty"` | |
| } |
There was a problem hiding this comment.
Keeping Port as a string. The backing CiliumNetworkPolicy CRD defines toPorts.ports.port as a string, and this resource is a lossless 1:1 projection onto it — SecurityGroupSpec is reused verbatim as the backing policy's spec body. Switching to intstr.IntOrString would serialize an integer port into the CNP, which its schema rejects, so the projection would stop round-tripping. Quoting port numbers matches how Cilium policies are written.
| } | ||
|
|
||
| // Patch applies a patch to the CiliumNetworkPolicy backing the SecurityGroup. | ||
| func (r *REST) Patch( |
There was a problem hiding this comment.
In Kubernetes apiserver, there is no rest.Patcher interface. PATCH requests are handled by the API server's generic handler by first calling Get to retrieve the resource, applying the patch in-memory, and then calling Update to persist the changes. Therefore, the custom Patch method on REST is dead code and will never be invoked by the API server at runtime. The marker label preservation is already fully handled by the Update path (via securityGroupToPolicy). You can safely remove the Patch method and its associated tests to simplify the codebase.
There was a problem hiding this comment.
Agreed — removed in 43e079e66. PATCH is served by the generic handler via Get+Update (rest.Patcher = Getter + Updater), and the Update path already re-asserts the marker label, so behavior is unchanged and the method was dead code.
| if !sort.StringsAreSorted(names) || !reflect.DeepEqual(names, want) { | ||
| t.Fatalf("List names = %v, want sorted %v", names, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a unit test to verify that cluster-wide listing works correctly and does not regress.
}
func TestListClusterWide(t *testing.T) {
r := newTestREST(t,
&CiliumNetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "sg-a",
Namespace: "tenant-a",
Labels: map[string]string{sgLabelKey: sgLabelValue},
},
},
&CiliumNetworkPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "sg-b",
Namespace: "tenant-b",
Labels: map[string]string{sgLabelKey: sgLabelValue},
},
},
)
ctx := request.WithNamespace(context.Background(), metav1.NamespaceAll)
out, err := r.List(ctx, &metainternal.ListOptions{})
if err != nil {
t.Fatalf("List returned error: %v", err)
}
list := out.(*sdnv1alpha1.SecurityGroupList)
if len(list.Items) != 2 {
t.Fatalf("expected 2 items, got %d", len(list.Items))
}
}There was a problem hiding this comment.
Added in 43e079e66 — TestListClusterWide and TestWatchClusterWide verify cross-namespace list/watch.
8ec416f to
c595f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/apis/sdn/DESIGN.md (1)
29-29: 💤 Low valueUse hyphenation for the compound adjective.
LanguageTool flagged: "project marked" should be hyphenated as "project-marked" when used as a compound adjective modifying CiliumNetworkPolicies.
✏️ Proposed fix
-3. Reads (`get`/`list`/`watch`) project marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view. +3. Reads (`get`/`list`/`watch`) project-marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/sdn/DESIGN.md` at line 29, The compound adjective "project marked" in the line describing Reads operations should be hyphenated since it is used to modify "CiliumNetworkPolicies". Change "project marked CiliumNetworkPolicies" to "project-marked CiliumNetworkPolicies" to follow proper hyphenation rules for compound adjectives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/sdn/securitygroup/rest.go`:
- Around line 504-516: In the watch call where metav1.ListOptions is being
constructed with the Raw field, the SendInitialEvents option from opts is not
being forwarded to the backing watch. Add the line SendInitialEvents:
opts.SendInitialEvents, to the Raw metav1.ListOptions struct (alongside the
existing AllowWatchBookmarks, Watch, and ResourceVersion fields) to ensure that
when initial events are requested by the client, the backing watch is properly
configured to handle them according to Kubernetes watch semantics.
---
Nitpick comments:
In `@pkg/apis/sdn/DESIGN.md`:
- Line 29: The compound adjective "project marked" in the line describing Reads
operations should be hyphenated since it is used to modify
"CiliumNetworkPolicies". Change "project marked CiliumNetworkPolicies" to
"project-marked CiliumNetworkPolicies" to follow proper hyphenation rules for
compound adjectives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95e1e375-8e2e-4e8c-ad59-8aa4c0e1cefc
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (24)
api/api-rules/cozystack_api_violation_exceptions.listpackages/system/cozystack-api/templates/apiservice.yamlpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-api/tests/rbac_test.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/cozystack-basics/tests/clusterroles-options_test.yamlpkg/apis/sdn/DESIGN.mdpkg/apis/sdn/fuzzer/fuzzer.gopkg/apis/sdn/install/install.gopkg/apis/sdn/install/roundtrip_test.gopkg/apis/sdn/register.gopkg/apis/sdn/v1alpha1/doc.gopkg/apis/sdn/v1alpha1/register.gopkg/apis/sdn/v1alpha1/securitygroup_types.gopkg/apis/sdn/v1alpha1/zz_generated.conversion.gopkg/apis/sdn/v1alpha1/zz_generated.deepcopy.gopkg/apis/sdn/v1alpha1/zz_generated.defaults.gopkg/apiserver/apiserver.gopkg/apiserver/scheme_test.gopkg/cmd/server/start.gopkg/registry/sdn/securitygroup/cilium.gopkg/registry/sdn/securitygroup/rest.gopkg/registry/sdn/securitygroup/rest_test.gopkg/registry/sdn/securitygroup/rest_watch_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/apis/sdn/install/roundtrip_test.go
- pkg/apis/sdn/v1alpha1/doc.go
- pkg/apis/sdn/v1alpha1/zz_generated.defaults.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/apis/sdn/fuzzer/fuzzer.go
- packages/system/cozystack-api/templates/rbac.yaml
- pkg/apis/sdn/register.go
- packages/system/cozystack-api/templates/apiservice.yaml
- packages/system/cozystack-basics/tests/clusterroles-options_test.yaml
- pkg/cmd/server/start.go
- pkg/apis/sdn/v1alpha1/securitygroup_types.go
- pkg/apis/sdn/v1alpha1/zz_generated.conversion.go
- pkg/registry/sdn/securitygroup/cilium.go
- api/api-rules/cozystack_api_violation_exceptions.list
- pkg/apiserver/scheme_test.go
- pkg/apis/sdn/v1alpha1/register.go
- pkg/apiserver/apiserver.go
- pkg/apis/sdn/v1alpha1/zz_generated.deepcopy.go
- pkg/registry/sdn/securitygroup/rest_test.go
c595f22 to
2234a3f
Compare
36cf376 to
20e3676
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/apis/sdn/DESIGN.md (1)
29-29: 💤 Low valueImprove sentence structure for clarity.
The phrase "Reads (
get/list/watch) project marked CiliumNetworkPolicies back into SecurityGroups" could be clearer. Consider restructuring to explicitly connect the read operations with the projection action.💭 Proposed revision
-3. Reads (`get`/`list`/`watch`) project marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view. +3. Reads (`get`, `list`, `watch`) project marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view.or alternatively:
-3. Reads (`get`/`list`/`watch`) project marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view. +3. Read operations (`get`, `list`, `watch`) project marked CiliumNetworkPolicies back into SecurityGroups. The marker label is hidden from the SecurityGroup view.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/sdn/DESIGN.md` at line 29, The sentence beginning with "Reads (`get`/`list`/`watch`) project marked CiliumNetworkPolicies back into SecurityGroups" is unclear because it doesn't explicitly connect the read operations with the projection action. Restructure this sentence to make the relationship between the read operations (get, list, watch) and the projection of marked CiliumNetworkPolicies into SecurityGroups more explicit and grammatically clear. Ensure the revised sentence flows logically and maintains the context that the marker label is hidden from the SecurityGroup view.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/sdn/securitygroup/rest_test.go`:
- Around line 124-233: Add two new regression test functions to validate
namespace and name identity boundaries. First, add a test function (e.g.,
TestCreateRejectsNamespaceMismatch) that verifies the Create method rejects a
SecurityGroup when metadata.namespace differs from the request namespace
(testNamespace). Second, add a test function (e.g.,
TestUpdateRejectsNameMismatch) that verifies the Update method rejects an update
when the SecurityGroup metadata.name differs from the URL name parameter passed
to the Update call. Both tests should assert that appropriate validation errors
are returned to prevent security boundary violations. Place these new test
functions after the existing test functions in the file to complete the
regression test coverage for these critical security contracts.
In `@pkg/registry/sdn/securitygroup/rest.go`:
- Around line 251-280: The Create method does not enforce that the SecurityGroup
being created belongs to the request's namespace, allowing a caller to post to
one namespace endpoint and persist to a different namespace. After the type
assertion check for the SecurityGroup object (the `in` variable), explicitly
bind the SecurityGroup's namespace to the namespace extracted from the request
context before any validation or persistence occurs. This ensures the object can
only be created in the intended namespace regardless of what namespace value was
provided in the request body.
- Around line 371-426: The Update function does not validate that the
SecurityGroup object's metadata.name matches the URL name parameter, which could
allow writing unintended objects in force-create flows. After casting the object
to sdnv1alpha1.SecurityGroup in the `in` variable and before calling
validateSecurityGroup, add a validation check that compares the SecurityGroup's
name field with the name parameter passed to the Update function, returning a
NewBadRequest or appropriate error if they do not match.
- Around line 608-613: The RV filtering logic at lines 608-613 in the watch
handler is incorrectly dropping ADDED events that are part of the initial replay
when sendInitialEvents=true. These replayed ADDED events intentionally have
ResourceVersion values less than or equal to the starting RV. Modify the
condition that guards the RV filtering check to only apply this filter to live
ADDED events that arrive after the initial replay completes, not to the replayed
events during the initial phase. You will need to track or detect when the
initial replay phase ends (typically marked by receiving a Bookmark event or
reaching the end of the backlog) and only apply the parseErr and objRV
comparison when processing post-replay events.
---
Nitpick comments:
In `@pkg/apis/sdn/DESIGN.md`:
- Line 29: The sentence beginning with "Reads (`get`/`list`/`watch`) project
marked CiliumNetworkPolicies back into SecurityGroups" is unclear because it
doesn't explicitly connect the read operations with the projection action.
Restructure this sentence to make the relationship between the read operations
(get, list, watch) and the projection of marked CiliumNetworkPolicies into
SecurityGroups more explicit and grammatically clear. Ensure the revised
sentence flows logically and maintains the context that the marker label is
hidden from the SecurityGroup view.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 728e225d-6c8d-475e-a65e-9ff0f2d83379
📒 Files selected for processing (14)
packages/system/cozystack-api/templates/apiservice.yamlpackages/system/cozystack-api/templates/rbac.yamlpackages/system/cozystack-api/tests/rbac_test.yamlpackages/system/cozystack-basics/templates/clusterroles.yamlpackages/system/cozystack-basics/tests/clusterroles-options_test.yamlpkg/apis/sdn/DESIGN.mdpkg/apiserver/apiserver.gopkg/apiserver/scheme_test.gopkg/cmd/server/start.gopkg/registry/sdn/securitygroup/cilium.gopkg/registry/sdn/securitygroup/rest.gopkg/registry/sdn/securitygroup/rest_test.gopkg/registry/sdn/securitygroup/rest_watch_test.gopkg/registry/sdn/securitygroup/validate.go
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/system/cozystack-basics/tests/clusterroles-options_test.yaml
- pkg/apiserver/scheme_test.go
- packages/system/cozystack-api/tests/rbac_test.yaml
- packages/system/cozystack-api/templates/apiservice.yaml
- packages/system/cozystack-api/templates/rbac.yaml
- pkg/apiserver/apiserver.go
- pkg/registry/sdn/securitygroup/cilium.go
- packages/system/cozystack-basics/templates/clusterroles.yaml
- pkg/registry/sdn/securitygroup/rest_watch_test.go
832a1e6 to
d1c0bf8
Compare
Introduce the sdn.cozystack.io/v1alpha1 API group with the SecurityGroup resource. SecurityGroup is a namespace-scoped, tenant-facing firewall object whose spec mirrors the subset of the CiliumNetworkPolicy rule that the abstraction exposes (endpointSelector, ingress, egress, ports, CIDR, FQDN), so the resource can be projected 1:1 onto a CiliumNetworkPolicy without granting tenants direct access to the cilium.io API group. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
The aggregated API server translates each SecurityGroup into a single CiliumNetworkPolicy in the same namespace and back. Policies owned by the SecurityGroup API are marked with the sdn.cozystack.io/securitygroup label, which the storage always re-asserts on every write and hides from the SecurityGroup view, so it only ever surfaces or mutates its own policies, leaves platform-managed policies untouched, and cannot be orphaned by a tenant overwriting the marker through spec labels. Mutating verbs run the genericapiserver admission hooks, and updates replace labels/annotations rather than merging them, matching Kubernetes PUT semantics. To avoid importing the full Cilium module — which pins a Kubernetes version incompatible with this project's apimachinery fork — the backing object is a minimal in-tree CiliumNetworkPolicy mirror that reuses SecurityGroupSpec as its spec body, making the projection a near-identity translation in both directions. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
Install the sdn.cozystack.io API group and serve the securitygroups resource from the SecurityGroup REST storage, register the static SecurityGroup kinds, add the group to the server codec, and register the in-tree CiliumNetworkPolicy mirror with the client scheme. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
Register the v1alpha1.sdn.cozystack.io aggregated APIService, grant the cozystack-api ServiceAccount full access to CiliumNetworkPolicy (the backing object for SecurityGroup), and let tenants self-service their SecurityGroups: full access in the base tenant role and read access in the view role, mirroring how apps.cozystack.io resources are exposed. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la> Co-authored-by: Timofei Larkin <lllamnyp@gmail.com>
Where this should head: security groups as attachable membership groupsFirst — the targetRef rework is a real improvement and the reasoning behind it is sound. Deriving the backing I want to use this PR to align on the destination, because one API decision here — The destination: a configurable firewall, not a fixed oneThe end state we're building toward is a tenant-configurable firewall. Today the per-tenant baseline ( That is what gives this feature a reason to exist. Under today's allow-all baseline a SecurityGroup can only add allowances on top of an already-open namespace, so it cannot actually restrict anything — Why attachment + membership beats a targetRef fieldIn that world the rules tenants reach for are app-to-app, and quickly after, group-to-group. The vocabulary should be: a SecurityGroup targets applications, and its peers are applications, other SecurityGroups, CIDRs, and FQDNs. Entities ( The cleanest way to deliver app-to-app and group-to-group references is to make a SecurityGroup a real membership group rather than a pointer to one application:
That last point is the payoff. With a real membership label, The cost, and why it is smaller than it looksThe honest trade is that membership requires a stateful piece: a controller that maintains the labels as applications and attachments come and go, with finalizers for clean detach and a short eventual-consistency window. That is the cost #1614 weighed when it specced this same model (membership label per group, a relabeling controller, What makes that controller cheap to build is that the identity it needs already lands on every pod for free. The lineage webhook ( One thing this also makes clean: because tenants have no direct access to Kubernetes primitives — only What I'm askingLet's converge on the membership-group model — SecurityGroups attached to applications, peers expressed as |
Give each SecurityGroup its own membership label (securitygroup.sdn.cozystack.io/<name>) and select it from the backing CiliumNetworkPolicy, instead of deriving the endpointSelector from a single app's lineage labels. spec.targetRef becomes spec.attachments, so one SecurityGroup can apply to several applications at once. Peers move from free-form fromEndpoints/toEndpoints selectors to fromApp/toApp (resolved to lineage labels) and fromSG/toSG (resolved to another group's membership label, a reference the Cilium dataplane resolves live). Attachments are persisted in a storage-owned CiliumNetworkPolicy annotation, round-tripped and hidden from the SecurityGroup view like the marker label. Validation now covers attachments and app/SG peers and rejects names that collide with reserved Cilium entities. The membership labels are maintained by a separate controller added in a follow-up commit; this change covers the API types and the aggregated-apiserver projection only. BREAKING CHANGE: SecurityGroup spec.targetRef is replaced by spec.attachments, and ingress/egress peers use fromApp/fromSG/toApp/toSG instead of fromEndpoints/toEndpoints. The feature is unreleased, so no migration is required. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
278e6ab to
fe6e28d
Compare
…tyGroups Add a controller that keeps each SecurityGroup's membership label (securitygroup.sdn.cozystack.io/<name>) in sync with its attachments: it stamps the label onto the pods of every attached application and removes it when an application is detached or the SecurityGroup is deleted. The backing CiliumNetworkPolicy's endpointSelector matches that label, so this is what makes a SecurityGroup apply to its members. The controller watches the marked backing policies and managed-app pods. It resolves each attachment through the lineage labels the lineage webhook already stamps — never the attachment list directly — and only ever labels pods in the SecurityGroup's own namespace, using single-key merge patches so it cannot clobber another group's label or a pod's lineage labels. A finalizer on the backing policy guarantees the labels are stripped before the policy is removed. Informer caches are bounded to managed pods and SecurityGroup-owned policies. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Add the securitygroup-controller Helm package (Deployment, ServiceAccount, ClusterRole and binding, image build) and register it as a default system package so it ships with the platform. The ClusterRole grants cluster-wide pod patch — the controller stamps the SecurityGroup membership label on managed-app pods across dynamically-created tenant namespaces, so the grant cannot be namespace-scoped — plus read and finalizer patch on the backing CiliumNetworkPolicies, and leader-election leases. Tenants gain no new permissions; the membership label is written exclusively by this platform controller. helm-unittest pins the pod-patch and ciliumnetworkpolicies grants so a dropped verb fails the suite rather than silently failing closed at runtime. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Rewrite the SecurityGroup design for the membership model: each group's own identity label, attachments stored in a backing-policy annotation, the REST/controller split, live fromSG/toSG references, the controller's hardening invariants and the eventual-consistency window contract. Spell out that the default-deny baseline flip and the membership admission webhook are out of scope and why, and that membership relocates rather than fixes the "tenant firewalls its own managed application" concern. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Rewrite the SecurityGroup e2e for the membership model: assert the backing CiliumNetworkPolicy's endpointSelector is the group's own membership label, that attachments ride a storage-owned annotation hidden from the view, and that fromApp/fromSG peers project to lineage- and membership-label endpoint selectors the real cilium.io/v2 CRD accepts. A second test creates a managed-labeled pod and asserts the securitygroup-controller stamps the membership label on attach and clears it on detach. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
fe6e28d to
98e60d0
Compare
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Approve — the membership-group model is the right shape, and it's well built
This is exactly the direction we wanted, implemented carefully. A few things I want to call out as done right:
- The API reads the way tenants think.
spec.attachmentslets one SecurityGroup apply to several applications, and peers arefromApp/toAppandfromSG/toSGinstead of raw selectors. App-to-app and group-to-group are now first-class. fromSG/toSGare live. Because each SecurityGroup owns a membership label (securitygroup.sdn.cozystack.io/<name>) and references resolve to the other group's membership key, a group-to-group reference follows membership as attachments change — no stale, frozen selectors. That liveness is the whole reason to carry the membership label, and it works.- The controller's containment is right. It resolves each attachment through the lineage labels rather than trusting the attachment list to pick pods directly, lists pods only within the SecurityGroup's own namespace, and writes single-key merge patches. So a tenant-driven, cluster-wide label writer can't reach pods outside the tenant's namespace or clobber another group's label. The finalizer strips membership before the backing policy is removed, caches are bounded to managed pods and marked policies, and the periodic resync backstops a missed pod event.
- Validation closes the projection edge cases. Rejecting a SecurityGroup name that can't form a valid label key — with a clean
metadata.nameerror instead of a confusing backing-policy write failure — and rejecting peer names that collide with reserved Cilium entities are both the right calls.
One small thing I'd like fixed in this PR, then two notes for later.
Please fix before merge
securitygroup_types.go still documents an empty ingress/egress list as denying all traffic to the group's member pods:
An empty list denies all ingress to the group's member pods.
Under the current per-tenant baseline (allow-internal-communication and friends blanket-allow the namespace) that isn't true yet — an empty rule list adds no allowance but denies nothing, because the policy can only widen on top of the baseline. Until the baseline moves to default-deny, this comment promises enforcement the feature doesn't provide. Please qualify it (e.g. "once the namespace baseline is default-deny, an empty list denies all ingress to the member pods; under today's allow-all baseline it has no effect"). Same for the egress comment.
Notes for follow-up (not blocking)
- Membership is applied after a pod starts, not at admission. That's the deliberate consequence of using a controller rather than extending the lineage webhook, and it's harmless today because the model is allow-only over an allow-all baseline. Once the baseline goes default-deny, a freshly-scaled pod will have a brief window before the controller stamps its membership label and its SecurityGroup-granted allows take effect — long enough to blip a pod's own startup egress. Worth documenting as a known property, and revisiting (admission-time stamping) only if it actually bites.
- The feature stays inert until the baseline tightens. This PR is the mechanism; the separate work of shrinking the per-tenant baseline to the minimum the platform needs and making tenant namespaces default-deny is what gives these rules teeth. That's the expected sequence, just naming it so the inertness isn't mistaken for a defect in this PR.
Approving on the strength of the design and implementation; the only in-tree ask is the doc qualification above.
There was a problem hiding this comment.
Code Review
This pull request introduces the SecurityGroup API, a namespaced projection over CiliumNetworkPolicy, and the securitygroup-controller to manage pod membership labels. The implementation includes the necessary API types, REST storage, and controller logic to ensure that pods of attached applications are correctly labeled for network policy enforcement. I have identified a high-severity issue regarding an invalid Go version in the Dockerfile and provided feedback on improving observability by logging errors in the reconciler.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -0,0 +1,24 @@ | |||
| FROM golang:1.26-alpine AS builder | |||
There was a problem hiding this comment.
The Go version 1.26 specified in the FROM instruction does not exist yet. This will cause the Docker build to fail. Please use a recent, valid Go version, for example 1.22-alpine.
FROM golang:1.22-alpine AS builder
There was a problem hiding this comment.
golang:1.26 is a real, published image — including the -alpine variant used here (both are on Docker Hub, and the cozystack builder images already build on golang:1.26). The build pulls it successfully; the "does not exist" note is stale training data. Keeping golang:1.26-alpine.
| func decodeAttachments(s string) []sdnv1alpha1.ApplicationReference { | ||
| if s == "" { | ||
| return nil | ||
| } | ||
| var refs []sdnv1alpha1.ApplicationReference | ||
| if err := json.Unmarshal([]byte(s), &refs); err != nil { | ||
| return nil | ||
| } | ||
| return refs | ||
| } |
There was a problem hiding this comment.
While it's reasonable to treat a malformed attachments annotation as empty to prevent the reconciler from crashing, swallowing the error silently can make debugging difficult. It would be beneficial for observability to log this error.
Consider changing the function signature to decodeAttachments(ctx context.Context, s string) and logging the error. You will also need to update the call sites in Reconcile and mapPodToSGs to pass the context.
// decodeAttachments parses the attachments annotation. A missing or malformed
// value yields nil.
func decodeAttachments(ctx context.Context, s string) []sdnv1alpha1.ApplicationReference {
if s == "" {
return nil
}
var refs []sdnv1alpha1.ApplicationReference
if err := json.Unmarshal([]byte(s), &refs); err != nil {
log.FromContext(ctx).Error(err, "failed to decode attachments annotation", "value", s)
return nil
}
return refs
}References
- Handle errors explicitly. Discarding meaningful errors with
_is a bug.
There was a problem hiding this comment.
Addressed in 881591d. decodeAttachments now takes a context and logs the error via log.FromContext before falling back to empty, so a malformed annotation is still tolerated (the reconciler stays resilient) but no longer fails silently. Covered by TestDecodeAttachments (empty / malformed / valid).
| if err := r.List(ctx, cnps, client.InNamespace(pod.Namespace), client.MatchingLabels{sgLabelKey: sgLabelValue}); err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
In mapPodToSGs, an error from r.List is swallowed. While returning nil is the correct way to signal no reconciliation requests, logging the error would improve observability, especially for transient API errors that might cause a pod update to be missed until the next periodic resync.
| if err := r.List(ctx, cnps, client.InNamespace(pod.Namespace), client.MatchingLabels{sgLabelKey: sgLabelValue}); err != nil { | |
| return nil | |
| } | |
| if err := r.List(ctx, cnps, client.InNamespace(pod.Namespace), client.MatchingLabels{sgLabelKey: sgLabelValue}); err != nil { | |
| log.FromContext(ctx).Error(err, "failed to list CiliumNetworkPolicies for pod mapping") | |
| return nil | |
| } |
References
- Handle errors explicitly. Discarding meaningful errors with
_is a bug.
There was a problem hiding this comment.
Addressed in 881591d. The swallowed List error in mapPodToSGs is now logged via log.FromContext before returning no requests, so a transient API error during pod→SecurityGroup mapping is visible instead of silently deferring the pod until the next resync.
…d target The securitygroup-controller package has a working `make image` target, but it was never wired into the top-level Makefile `build:` list, so CI never builds or pushes the controller image. The shipped values.yaml therefore keeps its `securitygroup-controller:v0.0.0` placeholder and the controller Deployment lands in ImagePullBackOff, since that image tag does not exist in the registry. Add the missing build line next to the other first-party controllers so CI builds, pushes, and digest-pins the image like every other controller. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…n the API Two watch-proxy defects in the SecurityGroup REST storage: - watch.Error events from the backing CiliumNetworkPolicy watch (e.g. a 410 Gone for an expired resourceVersion) carry a *metav1.Status, not a CiliumNetworkPolicy. The CNP type assertion dropped them, so the client saw a cleanly closed stream and never performed the required relist. Forward error events verbatim before the type assertion. - The DELETED-event bypass the label selector needs (a policy whose labels mutate out of the selector is deleted carrying non-matching labels, so dropping it would strand cached clients) was wrongly applied to the field selector too. Name and namespace are immutable, so a field-selected watch (metadata.name=sg-a) received other objects' deletions and could corrupt filtered client caches. Apply the field filter to every event type. Both paths are covered by new tests driving a fake backing watch. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The reconciler intentionally treats a malformed attachments annotation and a failed pod-mapping List as empty / no-op to stay resilient, but it swallowed both errors silently. That hides real defects: a bad annotation writer, or a transient API error that drops a pod update until the next periodic resync with no trace. Log both while preserving the resilient return behavior. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…s not deny The Ingress/Egress field docs claimed "an empty list denies all ingress/egress", which is a false security guarantee. A SecurityGroup projects onto a CiliumNetworkPolicy that only ADDS allow rules, and Cilium policies are additive over the tenant's blanket-allow baseline, so an empty list does not isolate a pod — effective connectivity stays open. Deny / default-deny enforcement depends on the default-deny baseline, which is separate future work. Rewrite the API field docs to the accurate semantics, regenerate the OpenAPI so the served schema description matches, and fix the one DESIGN.md sentence that still asserted "denies all traffic" (it already contradicted its own safety section, which correctly states the API cannot deny anything today). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
A SecurityGroup with attachments but no ingress or egress rules exists
only to stamp its membership label on the attached applications' pods, so
other SecurityGroups can reference it as a peer. Its backing
CiliumNetworkPolicy carried only an endpointSelector, which the
cilium.io/v2 CRD rejects synchronously: the spec anyOf requires at least
one of ingress/ingressDeny/egress/egressDeny, so a selector-only policy
fails with "spec.ingress: Required value". A membership-only group
could therefore never be created.
Always serialize the backing policy's ingress section, emitting an empty
list 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, matching the additive, non-isolating semantics of an
empty rule list. The projection guarantees the slice is non-nil so it
never marshals to null, which the CRD (no nullable fields) would also
reject. The empty section collapses back to nil on read, so a
membership-only group round-trips with no ingress or egress.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
The backing CiliumNetworkPolicy carries the membership finalizer, so deleting a SecurityGroup is virtually always asynchronous: the policy gets a deletionTimestamp and survives until the securitygroup-controller strips the membership labels off member pods and removes the finalizer. REST.Delete nonetheless returned (nil, true), reporting the object as instantly deleted, so the endpoint emitted a Success status claiming completion instead of surfacing the terminating object — a rest.GracefulDeleter contract violation for any client that distinguishes the two to detect pending teardown. After the delete, read the object's state back through the direct (uncached) client and report deleted=true only when it is actually gone; while a finalizer holds it, return deleted=false with the terminating object. Using the direct client avoids a stale cache-backed read misreporting an async delete as instant, and a dry-run (which leaves the object in place) is resolved from the object's authoritative finalizers. Implement rest.MayReturnFullObjectDeleter so the DELETE endpoint is advertised and serialized as returning the SecurityGroup object, matching the generic registry. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
The ingress fromApp/fromSG projection is exercised end to end, but the symmetric egress toApp/toSG path — toApp to a lineage-label endpointSelector and toSG to the referenced group's membership-label endpointSelector — had no test. A wrong label key there would silently point an egress allow at the wrong endpoints. Pin the projection and the reverse reconstruction with a Create+Get round-trip, mirroring the existing ingress coverage. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Re-approve — the follow-up fixes are solid, several beyond cosmetic
Re-reviewed the new commits. All six land well, and a few are real correctness wins, not just polish:
- Membership-only groups now create (
cc4fb40da). A SecurityGroup withattachmentsbut noingress/egress— exactly the shape a group meant to be referenced byfromSG/toSGtakes — projected to a selector-only spec that thecilium.io/v2CRD rejects (specanyOf requires a rule section). Emitting an always-presentingress: []carrier fixes it, and the reasoning is the important part: an empty ingress list is inert in Cilium (it doesn't enable ingress default-deny), so the policy is schema-valid yet adds and denies nothing — consistent with the additive model, and still correct once the baseline becomes default-deny. Forcing a non-nil empty slice (so it can't marshal tonull) and testing both the JSON and unstructured-converter paths is the right level of care. - Async delete reported correctly (
a86817915). Because the backing policy carries the membership finalizer, deletion is virtually always asynchronous; returning the terminating object withdeleted=false(read back through the direct client) and implementingMayReturnFullObjectDeletermatches theGracefulDeletercontract. - Two watch-proxy fixes (
11ed2a960). Forwardingwatch.Error(e.g. 410 Gone) so clients relist instead of seeing a clean close, and applying the field filter to every event type since name/namespace are immutable — both correct, both well-tested with a fake backing watch. - Empty-list doc semantics corrected (
f03d2a510), OpenAPI regenerated to match. Reconciler error logging (881591d34) and build wiring (09f4d8bb8) round it out.
Every change ships with tests. The branch is in better shape than at the prior approval — the create-path and watch fixes close gaps that would have surfaced in production. Approving the current SHA.
Main added securitygroup.bats (#2922 membership model) and serviceexposure.bats (#3081 ExposureClass/ServiceExposure) after the Chainsaw migration branch was cut. - securitygroup: two Tests — the aggregated-API projection round-trip (backing CNP marker label, empty-value membership endpointSelector, lineage fromApp + membership fromSG peers, 1:1 rule translation, the view hiding storage internals, unmarked-CNP invisibility via an error op, delete propagation) and the controller membership-label stamp/clear lifecycle. Only the attachments annotation stays a jq script (it is a JSON string). Chainsaw auto-cleanup covers every fixture even on mid-test failure — the bats had no teardown hook and leaked its fixtures. - serviceexposure: the externalIPs backend resolve/Ready/assignedIPs path with the no-MetalLB-pool negative as an error op (ordered after resolvedBackend so it cannot false-pass), and the finalizer-removal test collapsed onto the delete op itself, which waits for full disappearance. - selection: kubernetes-application now also maps to the two OIDC render-side suites (they exercise the same app chart — a gap main's bats selection had too); the new dirs enter all_apps discovery automatically. Selector unit tests extended and green (13/13 under cozytest). - README: suites list updated. Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
What this PR does
Introduces a tenant-facing
SecurityGroupresource in a newsdn.cozystack.io/v1alpha1API group, served by the Cozystack aggregated API server as a 1:1 projection of aCiliumNetworkPolicy. A SecurityGroup attaches to one of the tenant's managed applications by reference (spec.targetRef: {apiGroup, kind, name}) and declares allow-list ingress/egress for that application. Tenants manage network policy for their own apps entirely throughsecuritygroups.sdn.cozystack.io, without being granted any access to thecilium.ioAPI group.The translation is synchronous and stateless — the API server converts each SecurityGroup into a CiliumNetworkPolicy in the same namespace on write and back on read; there is no controller. The backing policy's
endpointSelectoris derived fromtargetRef(matched on the application's lineage labelsapps.cozystack.io/application.{group,kind,name}), never copied from tenant input, so the selector is machine-generated and can only ever match the referenced application's own pods in the tenant's namespace. Policies owned by this API carry ansdn.cozystack.io/securitygroupmarker label that the storage always re-asserts and hides from the view, so it only ever surfaces or mutates its own policies and never touches platform-managed network policies.The value is self-service network policy on a Cozystack-native, RBAC-scoped surface, without exposing Cilium's version-coupled CRDs to tenants. Because the selector is derived from an authorized application reference rather than free-form tenant input, the RBAC boundary is structural, not selector-validated: a tenant cannot express network policy against arbitrary or platform-owned pods. The full design — selector derivation from lineage labels, the structural boundary, the additive-policy reality (a SecurityGroup widens a managed application's allowed traffic but cannot sever its platform-managed management plane, and equally cannot tighten below the platform baseline), the in-tree CiliumNetworkPolicy mirror that avoids a Cilium Go-module dependency, RBAC, and deferred extensions (raw-pod targeting, cluster scope, reusable groups) — is in
pkg/apis/sdn/DESIGN.md.Implements the attachment model from #1614.
Release note
Summary by CodeRabbit
SecurityGroupAPI resource (sdn.cozystack.iov1alpha1) with a namespaced projection backed by Cilium network policies.APIServiceand wired SDN routing, scheme, and REST support.