[securitygroups] WIP - #1614
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a new SDN (Software-Defined Networking) API group with SecurityGroup resource backed by CiliumNetworkPolicy storage, updates Go dependencies to 1.24.0, removes unused fuzzer and validation implementations for apps and core APIs, and provides the complete API scaffolding including registration, types, and REST storage. Changes
Sequence DiagramsequenceDiagram
participant Client as Kubernetes Client
participant REST as SecurityGroup REST Storage
participant CNP as CiliumNetworkPolicy Client
participant Convert as Converter
Client->>REST: Create SecurityGroup
REST->>Convert: toCiliumNetworkPolicy
Convert->>REST: CiliumNetworkPolicy
REST->>CNP: Create CiliumNetworkPolicy
CNP->>REST: Created CiliumNetworkPolicy
REST->>Convert: fromCiliumNetworkPolicy
Convert->>REST: SecurityGroup
REST->>Client: Created SecurityGroup
Client->>REST: Get SecurityGroup
REST->>CNP: Get CiliumNetworkPolicy
CNP->>REST: CiliumNetworkPolicy
REST->>Convert: fromCiliumNetworkPolicy
Convert->>REST: SecurityGroup
REST->>Client: SecurityGroup
Client->>REST: List SecurityGroups (namespace)
REST->>CNP: List CiliumNetworkPolicies
CNP->>REST: []CiliumNetworkPolicy
REST->>Convert: Convert each to SecurityGroup
Convert->>REST: SecurityGroupList
REST->>Client: SecurityGroupList
Client->>REST: Watch SecurityGroups
REST->>CNP: Watch CiliumNetworkPolicies
CNP-->>REST: Event: CiliumNetworkPolicy
REST->>Convert: fromCiliumNetworkPolicy
REST-->>Client: Event: SecurityGroup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
565b741 to
480163f
Compare
|
CodeRabbit (@coderabbitai) review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
10-161: Now let me check the specific code files to understand the issues better:Multiple critical compilation errors from dependency updates must be resolved before proceeding.
Test execution revealed three categories of build failures that block compilation:
klog/v2 v2.130.1 format string errors - klog.Errorf requires compile-time format strings handled in the manner of fmt.Printf, but the code passes format strings as variables:
pkg/registry/core/tenantmodule/rest.go:546pkg/registry/apps/application/rest.go:447, 448, 858Fix: Change dynamic format strings to compile-time constants.
SecurityGroup missing DeepCopyObject - The runtime.Object interface requires DeepCopyObject() since Kubernetes 1.8, typically implemented via code generation with the
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Objectmarker:
pkg/registry/sdn/securitygroup/rest.go- 10+ compilation errorsFix: Either add code-gen markers to SecurityGroup or regenerate deepcopy methods.
CiliumNetworkPolicy type conversion errors - Type assertions and parameter mismatches prevent compilation in the same file.
Additionally, test failures in
internal/controllerandkubeovnplungerpackages suggest runtime incompatibilities.
🧹 Nitpick comments (1)
pkg/apis/sdn/fuzzer/fuzzer.go (1)
23-25: Clarify the API group in this commentThe comment still mentions the “core” API group even though we are in the SDN package. Please update it so future readers aren’t misled.
-// Funcs returns the fuzzer functions for the core api group. +// Funcs returns the fuzzer functions for the SDN API group.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumpkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (15)
go.mod(2 hunks)pkg/apis/apps/fuzzer/fuzzer.go(1 hunks)pkg/apis/core/fuzzer/fuzzer.go(1 hunks)pkg/apis/core/validation/validation.go(0 hunks)pkg/apis/sdn/fuzzer/fuzzer.go(1 hunks)pkg/apis/sdn/install/install.go(1 hunks)pkg/apis/sdn/install/roundtrip_test.go(1 hunks)pkg/apis/sdn/register.go(1 hunks)pkg/apis/sdn/v1alpha1/doc.go(1 hunks)pkg/apis/sdn/v1alpha1/register.go(1 hunks)pkg/apis/sdn/v1alpha1/securitygroup_types.go(1 hunks)pkg/apis/sdn/v1alpha1/zz_generated.conversion.go(1 hunks)pkg/apis/sdn/v1alpha1/zz_generated.deepcopy.go(1 hunks)pkg/apis/sdn/v1alpha1/zz_generated.defaults.go(1 hunks)pkg/registry/sdn/securitygroup/rest.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/apis/core/validation/validation.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (8)
pkg/apis/sdn/register.go (1)
19-22: LGTM!The
GroupNameconstant declaration follows standard Kubernetes API group conventions.pkg/apis/sdn/install/install.go (1)
25-29: LGTM!The
Installfunction follows standard Kubernetes patterns for API group registration, correctly usingAddToSchemeandSetVersionPriority.pkg/apis/sdn/v1alpha1/zz_generated.defaults.go (1)
28-33: LGTM - Expected scaffolding for new API.The no-op
RegisterDefaultsimplementation is standard for newly generated API scaffolding. This will be populated automatically when default values are added to the SDN types.pkg/apis/sdn/v1alpha1/doc.go (1)
17-25: LGTM!The code-generation annotations are correctly configured for OpenAPI, deepcopy, conversion, and defaulter generation. The
groupNamematches the constant defined inregister.go.pkg/apis/sdn/v1alpha1/zz_generated.conversion.go (1)
28-36: LGTM - Expected scaffolding for new API.The no-op
RegisterConversionsimplementation is standard for newly generated API scaffolding. This will be populated automatically when conversion functions are added between API versions.go.mod (1)
5-7: Verify Go version selection and ensure latest stable compatibility.Go 1.24.0 is superseded by Go 1.25.4, the latest stable version as of November 2025. Confirm why you're using Go 1.24.0 instead of upgrading to 1.25.4, especially given the major version gap. Verify this choice is compatible with your CI/CD environment, all dependencies, and any version constraints. If staying on 1.24.0 is intentional, document the reason; otherwise, consider upgrading to the latest stable release.
pkg/apis/apps/fuzzer/fuzzer.go (1)
24-25: Verify that removing all fuzzer implementations doesn't degrade test coverage.This change removes the custom
ApplicationSpecfuzzer, and all API group fuzzers (apps, core, sdn) have been similarly cleared. Roundtrip tests inpkg/apis/apps/install/roundtrip_test.go:27andpkg/apiserver/scheme_test.go:28still callappsfuzzer.Funcsbut will now receive an empty fuzzer list. Confirm that tests still pass and that this systematic removal of fuzzing coverage is intentional.pkg/apis/core/fuzzer/fuzzer.go (1)
24-25: No test failures will result from this change.The custom fuzzer for
TenantNamespaceSpechas been intentionally removed. While the roundtrip tests inpkg/apis/core/install/roundtrip_test.goandpkg/apiserver/scheme_test.gostill passcorefuzzer.Funcs, k8s.io/apimachinery's roundtrip helpers apply default library fuzzers even when custom fuzzer functions are empty. Tests will continue to execute successfully and will not break.
| metav1.AddToGroupVersion(scheme, SchemeGroupVersion) | ||
| return nil | ||
| } | ||
|
|
||
| // Resource turns an unqualified resource name into a fully-qualified one. | ||
| func Resource(resource string) schema.GroupResource { | ||
| return SchemeGroupVersion.WithResource(resource).GroupResource() | ||
| } | ||
|
|
||
| // ----------------------------------------------------------------------------- | ||
| // Public helpers consumed by the apiserver wiring | ||
| // ----------------------------------------------------------------------------- | ||
|
|
||
| // RegisterStaticTypes adds *compile-time* resources such as TenantNamespace. | ||
| func RegisterStaticTypes(scheme *runtime.Scheme) { | ||
| scheme.AddKnownTypes(SchemeGroupVersion) | ||
| metav1.AddToGroupVersion(scheme, SchemeGroupVersion) | ||
| klog.V(1).Info("Registered static kinds: SecurityGroups") |
There was a problem hiding this comment.
Register SecurityGroup types with the scheme
addKnownTypes and RegisterStaticTypes never pass SecurityGroup / SecurityGroupList into scheme.AddKnownTypes. As written, the scheme never learns about these kinds, so any create/get/list against the SecurityGroup resource will fail with NoKindMatch/MissingKind. Please register the concrete types in both helpers.
func addKnownTypes(scheme *runtime.Scheme) error {
+ scheme.AddKnownTypes(
+ SchemeGroupVersion,
+ &SecurityGroup{},
+ &SecurityGroupList{},
+ )
metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
return nil
}
// RegisterStaticTypes adds *compile-time* resources such as TenantNamespace.
func RegisterStaticTypes(scheme *runtime.Scheme) {
- scheme.AddKnownTypes(SchemeGroupVersion)
+ scheme.AddKnownTypes(
+ SchemeGroupVersion,
+ &SecurityGroup{},
+ &SecurityGroupList{},
+ )
metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
klog.V(1).Info("Registered static kinds: SecurityGroups")
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/apis/sdn/v1alpha1/register.go around lines 42 to 59, the scheme
registration functions call AddKnownTypes without including the concrete
SecurityGroup and SecurityGroupList types, so the API server will report
NoKindMatch/MissingKind for that resource; update both addKnownTypes (the lower
helper used during init) and RegisterStaticTypes to pass the concrete types into
scheme.AddKnownTypes(SchemeGroupVersion, &SecurityGroup{}, &SecurityGroupList{})
(or the correct type names/addresses if different) so the scheme learns those
kinds, and ensure the types are imported/visible to this package.
| func toCiliumNetworkPolicy(sg *sdnv1alpha1.SecurityGroup, np *ciliumv2.CiliumNetworkPolicy) *ciliumv2.CiliumNetworkPolicy { | ||
| var out ciliumv2.CiliumNetworkPolicy | ||
| if np != nil { | ||
| out = *np.DeepCopy() | ||
| } | ||
| out.TypeMeta = metav1.TypeMeta{ | ||
| APIVersion: ciliumv2.SchemeGroupVersion.String(), | ||
| Kind: ciliumv2.CNPKindDefinition, | ||
| } | ||
| out.Name, out.Namespace = np.Name, np.Namespace | ||
|
|
||
| if out.Labels == nil { | ||
| out.Labels = map[string]string{} | ||
| } | ||
| for k, v := range sg.Labels { | ||
| out.Labels[k] = v | ||
| } |
There was a problem hiding this comment.
Fix nil dereference in toCiliumNetworkPolicy.
Creating a SecurityGroup currently panics because np is nil yet we still read np.Name/np.Namespace. Initialise out solely from sg when np is nil (and then overlay any live object data when present) so the create path is safe.
Apply this diff:
func toCiliumNetworkPolicy(sg *sdnv1alpha1.SecurityGroup, np *ciliumv2.CiliumNetworkPolicy) *ciliumv2.CiliumNetworkPolicy {
- var out ciliumv2.CiliumNetworkPolicy
- if np != nil {
- out = *np.DeepCopy()
- }
+ var out ciliumv2.CiliumNetworkPolicy
+ if np != nil {
+ out = *np.DeepCopy()
+ }
out.TypeMeta = metav1.TypeMeta{
APIVersion: ciliumv2.SchemeGroupVersion.String(),
Kind: ciliumv2.CNPKindDefinition,
}
- out.Name, out.Namespace = np.Name, np.Namespace
+ out.Name, out.Namespace = sg.Name, sg.Namespace📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func toCiliumNetworkPolicy(sg *sdnv1alpha1.SecurityGroup, np *ciliumv2.CiliumNetworkPolicy) *ciliumv2.CiliumNetworkPolicy { | |
| var out ciliumv2.CiliumNetworkPolicy | |
| if np != nil { | |
| out = *np.DeepCopy() | |
| } | |
| out.TypeMeta = metav1.TypeMeta{ | |
| APIVersion: ciliumv2.SchemeGroupVersion.String(), | |
| Kind: ciliumv2.CNPKindDefinition, | |
| } | |
| out.Name, out.Namespace = np.Name, np.Namespace | |
| if out.Labels == nil { | |
| out.Labels = map[string]string{} | |
| } | |
| for k, v := range sg.Labels { | |
| out.Labels[k] = v | |
| } | |
| func toCiliumNetworkPolicy(sg *sdnv1alpha1.SecurityGroup, np *ciliumv2.CiliumNetworkPolicy) *ciliumv2.CiliumNetworkPolicy { | |
| var out ciliumv2.CiliumNetworkPolicy | |
| if np != nil { | |
| out = *np.DeepCopy() | |
| } | |
| out.TypeMeta = metav1.TypeMeta{ | |
| APIVersion: ciliumv2.SchemeGroupVersion.String(), | |
| Kind: ciliumv2.CNPKindDefinition, | |
| } | |
| out.Name, out.Namespace = sg.Name, sg.Namespace | |
| if out.Labels == nil { | |
| out.Labels = map[string]string{} | |
| } | |
| for k, v := range sg.Labels { | |
| out.Labels[k] = v | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/sdn/securitygroup/rest.go around lines 51 to 67, the function
toCiliumNetworkPolicy dereferences np (np.Name/np.Namespace) without checking
for nil which causes a panic on create; change the logic to initialize out from
the SecurityGroup when np is nil (set out.Name and out.Namespace from sg or
leave empty as appropriate), only overlay fields from np when np != nil (i.e.,
deep-copy np into out and then copy labels/other live-object data), and ensure
labels are initialized and populated from sg first then merged/overwritten by
np.Labels when present so the create path does not read np.Name/np.Namespace
when np is nil.
| func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtime.Object, error) { | ||
| ns, ok := request.NamespaceFrom(ctx) | ||
| if !ok { | ||
| return nil, apierrors.NewBadRequest("namespace required") | ||
| } | ||
|
|
||
| list := &ciliumv2.CiliumNetworkPolicyList{} | ||
| err := r.c.List(ctx, list, | ||
| &client.ListOptions{ | ||
| Namespace: ns, | ||
| Raw: &metav1.ListOptions{}, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| out := &sdnv1alpha1.SecurityGroupList{ |
There was a problem hiding this comment.
Honor list selectors and resource version.
We currently discard opts entirely, so label selectors, field selectors, limits, continues, and resource versions sent by clients are ignored. That breaks standard API semantics (kubectl get --selector=…, chunking, consistent reads). Please plumb opts through when constructing client.ListOptions.
Apply this diff:
- list := &ciliumv2.CiliumNetworkPolicyList{}
- err := r.c.List(ctx, list,
- &client.ListOptions{
- Namespace: ns,
- Raw: &metav1.ListOptions{},
- },
- )
+ rawOpts := &metav1.ListOptions{
+ ResourceVersion: opts.ResourceVersion,
+ Limit: opts.Limit,
+ Continue: opts.Continue,
+ }
+ if opts.LabelSelector != nil {
+ rawOpts.LabelSelector = opts.LabelSelector.String()
+ }
+ if opts.FieldSelector != nil {
+ rawOpts.FieldSelector = opts.FieldSelector.String()
+ }
+
+ list := &ciliumv2.CiliumNetworkPolicyList{}
+ err := r.c.List(ctx, list, &client.ListOptions{
+ Namespace: ns,
+ Raw: rawOpts,
+ })🤖 Prompt for AI Agents
In pkg/registry/sdn/securitygroup/rest.go around lines 161-178, the List method
ignores the incoming metainternal.ListOptions (opts) so client list selectors,
limits, continue token and resourceVersion are dropped; map the relevant fields
from opts into the client.ListOptions.Raw metav1.ListOptions (LabelSelector,
FieldSelector, Limit, Continue, ResourceVersion, ResourceVersionMatch if
present) and pass that pointer into r.c.List, and preserve Namespace as before
so kubectl selectors, chunking and consistent reads behave correctly.
| cur := &ciliumv2.CiliumNetworkPolicy{} | ||
| err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| return nil, false, err | ||
| } | ||
|
|
||
| newObj, err := objInfo.UpdatedObject(ctx, nil) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| in := newObj.(*sdnv1alpha1.SecurityGroup) | ||
|
|
||
| newNp := toCiliumNetworkPolicy(in, cur) | ||
| newNp.Namespace = ns | ||
| if cur == nil { | ||
| if !forceCreate && err == nil { | ||
| return nil, false, apierrors.NewNotFound(sdnv1alpha1.Resource(sdnv1alpha1.SecurityGroupPluralName), name) | ||
| } | ||
| err := r.c.Create(ctx, newNp, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) | ||
| return fromCiliumNetworkPolicy(newNp), true, err | ||
| } | ||
|
|
||
| newNp.ResourceVersion = cur.ResourceVersion | ||
| err = r.c.Update(ctx, newNp, &client.UpdateOptions{Raw: opts}) | ||
| return fromCiliumNetworkPolicy(newNp), false, err |
There was a problem hiding this comment.
Handle NotFound correctly in Update.
cur is always non-nil, so the create-on-update branch is unreachable; we then issue an Update against a non-existent object (with empty resourceVersion). Track the Get error separately, treat the object as absent when IsNotFound, and only call Update when the live object exists.
Apply this diff:
- cur := &ciliumv2.CiliumNetworkPolicy{}
- err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}})
- if err != nil && !apierrors.IsNotFound(err) {
+ var cur *ciliumv2.CiliumNetworkPolicy
+ existing := &ciliumv2.CiliumNetworkPolicy{}
+ err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, existing, &client.GetOptions{Raw: &metav1.GetOptions{}})
+ if err != nil {
+ if !apierrors.IsNotFound(err) {
+ return nil, false, err
+ }
+ } else {
+ cur = existing
+ }
+
+ newObj, err := objInfo.UpdatedObject(ctx, cur)
+ if err != nil {
return nil, false, err
}
-
- newObj, err := objInfo.UpdatedObject(ctx, nil)
- if err != nil {
- return nil, false, err
- }
- in := newObj.(*sdnv1alpha1.SecurityGroup)
+ in := newObj.(*sdnv1alpha1.SecurityGroup)
newNp := toCiliumNetworkPolicy(in, cur)
newNp.Namespace = ns
if cur == nil {
- if !forceCreate && err == nil {
+ if !forceCreate {
return nil, false, apierrors.NewNotFound(sdnv1alpha1.Resource(sdnv1alpha1.SecurityGroupPluralName), name)
}
- err := r.c.Create(ctx, newNp, &client.CreateOptions{Raw: &metav1.CreateOptions{}})
- return fromCiliumNetworkPolicy(newNp), true, err
+ if err := r.c.Create(ctx, newNp, &client.CreateOptions{Raw: &metav1.CreateOptions{}}); err != nil {
+ return nil, false, err
+ }
+ return fromCiliumNetworkPolicy(newNp), true, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cur := &ciliumv2.CiliumNetworkPolicy{} | |
| err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) | |
| if err != nil && !apierrors.IsNotFound(err) { | |
| return nil, false, err | |
| } | |
| newObj, err := objInfo.UpdatedObject(ctx, nil) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| in := newObj.(*sdnv1alpha1.SecurityGroup) | |
| newNp := toCiliumNetworkPolicy(in, cur) | |
| newNp.Namespace = ns | |
| if cur == nil { | |
| if !forceCreate && err == nil { | |
| return nil, false, apierrors.NewNotFound(sdnv1alpha1.Resource(sdnv1alpha1.SecurityGroupPluralName), name) | |
| } | |
| err := r.c.Create(ctx, newNp, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) | |
| return fromCiliumNetworkPolicy(newNp), true, err | |
| } | |
| newNp.ResourceVersion = cur.ResourceVersion | |
| err = r.c.Update(ctx, newNp, &client.UpdateOptions{Raw: opts}) | |
| return fromCiliumNetworkPolicy(newNp), false, err | |
| var cur *ciliumv2.CiliumNetworkPolicy | |
| existing := &ciliumv2.CiliumNetworkPolicy{} | |
| err := r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, existing, &client.GetOptions{Raw: &metav1.GetOptions{}}) | |
| if err != nil { | |
| if !apierrors.IsNotFound(err) { | |
| return nil, false, err | |
| } | |
| } else { | |
| cur = existing | |
| } | |
| newObj, err := objInfo.UpdatedObject(ctx, cur) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| in := newObj.(*sdnv1alpha1.SecurityGroup) | |
| newNp := toCiliumNetworkPolicy(in, cur) | |
| newNp.Namespace = ns | |
| if cur == nil { | |
| if !forceCreate { | |
| return nil, false, apierrors.NewNotFound(sdnv1alpha1.Resource(sdnv1alpha1.SecurityGroupPluralName), name) | |
| } | |
| if err := r.c.Create(ctx, newNp, &client.CreateOptions{Raw: &metav1.CreateOptions{}}); err != nil { | |
| return nil, false, err | |
| } | |
| return fromCiliumNetworkPolicy(newNp), true, nil | |
| } | |
| newNp.ResourceVersion = cur.ResourceVersion | |
| err = r.c.Update(ctx, newNp, &client.UpdateOptions{Raw: opts}) | |
| return fromCiliumNetworkPolicy(newNp), false, err |
🤖 Prompt for AI Agents
In pkg/registry/sdn/securitygroup/rest.go around lines 206 to 230, the Get error
is overwritten and cur is assumed non-nil which makes the create-on-update
branch unreachable and leads to Update calls against non-existent objects;
preserve the Get error in a separate variable (e.g., getErr), treat the object
as absent when apierrors.IsNotFound(getErr), only call Create when the live
object is absent (respecting forceCreate) and only call Update when cur is
non-nil, and avoid reusing the err variable so ResourceVersion is only set when
cur exists and the correct error is returned otherwise.
| npList := &ciliumv2.CiliumNetworkPolicyList{} | ||
| base, err := r.w.Watch(ctx, npList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ | ||
| Watch: true, | ||
| ResourceVersion: opts.ResourceVersion, | ||
| }}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ch := make(chan watch.Event) | ||
| proxy := watch.NewProxyWatcher(ch) | ||
|
|
||
| go func() { | ||
| defer proxy.Stop() | ||
| for ev := range base.ResultChan() { | ||
| np, ok := ev.Object.(*ciliumv2.CiliumNetworkPolicy) | ||
| if !ok || sec == nil { | ||
| continue | ||
| } | ||
| sg := fromCiliumNetworkPolicy(np) | ||
| ch <- watch.Event{ | ||
| Type: ev.Type, | ||
| Object: sg, | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Fix build-breaking typo in watch proxy.
The guard references sec, which is undefined, so this file does not compile. Check the decoded object (np) instead before forwarding the event.
Apply this diff:
- np, ok := ev.Object.(*ciliumv2.CiliumNetworkPolicy)
- if !ok || sec == nil {
+ np, ok := ev.Object.(*ciliumv2.CiliumNetworkPolicy)
+ if !ok || np == nil {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| npList := &ciliumv2.CiliumNetworkPolicyList{} | |
| base, err := r.w.Watch(ctx, npList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ | |
| Watch: true, | |
| ResourceVersion: opts.ResourceVersion, | |
| }}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ch := make(chan watch.Event) | |
| proxy := watch.NewProxyWatcher(ch) | |
| go func() { | |
| defer proxy.Stop() | |
| for ev := range base.ResultChan() { | |
| np, ok := ev.Object.(*ciliumv2.CiliumNetworkPolicy) | |
| if !ok || sec == nil { | |
| continue | |
| } | |
| sg := fromCiliumNetworkPolicy(np) | |
| ch <- watch.Event{ | |
| Type: ev.Type, | |
| Object: sg, | |
| } | |
| } | |
| }() | |
| npList := &ciliumv2.CiliumNetworkPolicyList{} | |
| base, err := r.w.Watch(ctx, npList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ | |
| Watch: true, | |
| ResourceVersion: opts.ResourceVersion, | |
| }}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| ch := make(chan watch.Event) | |
| proxy := watch.NewProxyWatcher(ch) | |
| go func() { | |
| defer proxy.Stop() | |
| for ev := range base.ResultChan() { | |
| np, ok := ev.Object.(*ciliumv2.CiliumNetworkPolicy) | |
| if !ok || np == nil { | |
| continue | |
| } | |
| sg := fromCiliumNetworkPolicy(np) | |
| ch <- watch.Event{ | |
| Type: ev.Type, | |
| Object: sg, | |
| } | |
| } | |
| }() |
🤖 Prompt for AI Agents
In pkg/registry/sdn/securitygroup/rest.go around lines 287 to 312, the
goroutine's guard incorrectly references an undefined variable `sec`; change the
guard to validate the decoded object instead (e.g., check the type assertion
result and that `np` is non-nil: if !ok || np == nil { continue }) before
calling fromCiliumNetworkPolicy and sending the event to the proxy channel so
the file compiles and forwards only valid events.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
480163f to
10ada3b
Compare
|
This PR has had no activity for 60 days and was marked |
…o) (#2922) ## What this PR does Introduces a tenant-facing `SecurityGroup` resource in a new `sdn.cozystack.io/v1alpha1` API group, served by the Cozystack aggregated API server as a 1:1 projection of a `CiliumNetworkPolicy`. 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 through `securitygroups.sdn.cozystack.io`, without being granted any access to the `cilium.io` API 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 `endpointSelector` is **derived** from `targetRef` (matched on the application's lineage labels `apps.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 an `sdn.cozystack.io/securitygroup` marker 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 ```release-note feat(api): add the SecurityGroup resource (sdn.cozystack.io/v1alpha1) — a tenant-facing, namespace-scoped firewall that attaches to a managed application by reference and projects 1:1 onto a CiliumNetworkPolicy with a machine-derived endpoint selector, letting tenants manage their application's network policy without cilium.io access. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added the `SecurityGroup` API resource (`sdn.cozystack.io` v1alpha1) with a namespaced projection backed by Cilium network policies. * Enabled the SDN `APIService` and wired SDN routing, scheme, and REST support. * Expanded RBAC to include SecurityGroup access and required Cilium policy permissions. * **Documentation** * Added a design document describing projection behavior and rule semantics. * **Bug Fixes** * Improved API rule validation coverage for missing list-type fields. * **Tests** * Added SDN round-trip, fuzzing, RBAC, and comprehensive REST create/get/list/update/delete/watch/resource-version tests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
Superseded by #2922 |
Summary by CodeRabbit
Release Notes
New Features
Chores