Add Network, NetworkLink, InterNetworkIngress, NetworkAccess CRD types and manifests - #2541
Add Network, NetworkLink, InterNetworkIngress, NetworkAccess CRD types and manifests#2541fgiorgetti wants to merge 9 commits into
Conversation
…s and manifests Introduces four new Skupper v2alpha1 API types to enable inter-network connectivity: - Network: identifies a Skupper network by its NetworkId - NetworkLink: establishes a link to another network via TLS hostname/port - InterNetworkIngress: routes traffic from a NetworkLink or NetworkAccess to a local routing key - NetworkAccess: exposes a local endpoint to another network with optional dynamic port allocation and TLS Also adds: - CRD YAML manifests for all four new types under config/crd/bases/ - Generated client, informer, and lister code for all four new types - Updates config/crd/kustomization.yaml to reference the new manifests - Updates pkg/apis/skupper/v2alpha1/register.go to register the new types - Updates zz_generated.deepcopy.go with DeepCopy methods for new types
|
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:
📝 WalkthroughWalkthroughAdds v2alpha1 API types and helpers for network resources, registers and deep-copies them, and introduces corresponding Kubernetes CRDs with validation, status subresources, printer columns, Kustomize inclusion, tests, and an ADR describing multi-VAN resource management. ChangesNetwork API and CRDs
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a1b68e5-7543-4569-8c83-cb7a82adf8e6
⛔ Files ignored due to path filters (22)
pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_internetworkingress.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_network.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_networkaccess.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_networklink.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_skupper_client.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/generated_expansion.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/networklink.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/skupper_client.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/interface.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/networklink.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/expansion_generated.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/networklink.gois excluded by!**/generated/**
📒 Files selected for processing (9)
config/crd/bases/skupper_inter_network_ingress_crd.yamlconfig/crd/bases/skupper_network_access_crd.yamlconfig/crd/bases/skupper_network_crd.yamlconfig/crd/bases/skupper_network_link_crd.yamlconfig/crd/bases/skupper_router_access_crd.yamlconfig/crd/kustomization.yamlpkg/apis/skupper/v2alpha1/register.gopkg/apis/skupper/v2alpha1/types.gopkg/apis/skupper/v2alpha1/zz_generated.deepcopy.go
b545f7d to
2b809a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/apis/skupper/v2alpha1/types.go (1)
1381-1415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated endpoint reconciliation logic.
This is an exact copy of
RouterAccessStatus.UpdateEndpointsForGroup(Lines 926-959). Extract a shared helper, e.g.func updateEndpointsForGroup(existing []Endpoint, desired []Endpoint, group string) ([]Endpoint, bool), and have both status types delegate to it, so future fixes don't have to be applied twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dacf0106-37d6-4fb2-8c86-f1bd57e9a0a1
⛔ Files ignored due to path filters (22)
pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_internetworkingress.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_network.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_networkaccess.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_networklink.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/fake/fake_skupper_client.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/generated_expansion.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/networklink.gois excluded by!**/generated/**pkg/generated/client/clientset/versioned/typed/skupper/v2alpha1/skupper_client.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/interface.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/informers/externalversions/skupper/v2alpha1/networklink.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/expansion_generated.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/internetworkingress.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/network.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/networkaccess.gois excluded by!**/generated/**pkg/generated/client/listers/skupper/v2alpha1/networklink.gois excluded by!**/generated/**
📒 Files selected for processing (9)
config/crd/bases/skupper_inter_network_ingress_crd.yamlconfig/crd/bases/skupper_network_access_crd.yamlconfig/crd/bases/skupper_network_crd.yamlconfig/crd/bases/skupper_network_link_crd.yamlconfig/crd/bases/skupper_router_access_crd.yamlconfig/crd/kustomization.yamlpkg/apis/skupper/v2alpha1/register.gopkg/apis/skupper/v2alpha1/types.gopkg/apis/skupper/v2alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (6)
- config/crd/bases/skupper_network_access_crd.yaml
- config/crd/bases/skupper_network_crd.yaml
- config/crd/bases/skupper_network_link_crd.yaml
- config/crd/bases/skupper_inter_network_ingress_crd.yaml
- pkg/apis/skupper/v2alpha1/register.go
- pkg/apis/skupper/v2alpha1/zz_generated.deepcopy.go
| - bases/skupper_multikeylistener_crd.yaml | ||
| - bases/skupper_network_crd.yaml | ||
| - bases/skupper_network_link_crd.yaml | ||
| - bases/skupper_network_access_crd.yaml |
There was a problem hiding this comment.
Waiting to see your ADR, I'm sure you'll have considered this. I had imagined we'd have the net-new resources not installed by default - enabled/installed in some alternate controller configuration so that "experimental" features require opt in until we feel ready to commit to them.
There was a problem hiding this comment.
The current implementation (which should come next, once we settle the CRD discussion) does not yet handle the new CRDs as optionals, but I agree with you, that they must be considered optionals.
Along with the new ones, there is also a change to RouterAccess (which should not affect exisintg installations). But that is a great point.
There was a problem hiding this comment.
@c-kruse I have added a new chart skupper-crds, which installs only the default crds unless you specify --set multiVan=true. The Makefile target generate-crd-manifest now generates a new yaml called: skupper-multi-van-crds.yaml.
Let me know if you agree with this updated mechanism.
|
|
||
| ## Context | ||
|
|
||
| New CRDs have been added to support multi-van management through VMS. |
There was a problem hiding this comment.
We should review the decision to add new CRDs versus extending the existing ones for multi-van support.
I recall that RBAC for the resources was a consideration but we should revisit.
Were there other factors to support adding new CRDs?
There was a problem hiding this comment.
I'm thinking that depending on the type of modification of the CRD, it could provoke an incompatibility with existent custom resources that were created with old CRDs (ie fields reconsidered as required). Adding a new CRD instead of modifying the current ones gives more capacity for change without having deal with the migration of older CRs.
There was a problem hiding this comment.
@ajssmith NetworkAccess could be accommodated as part of RouterAccess, as RouterAccess is going to be used for "normal" roles as part of the multi-van work. The main reason why it is defined as a separate resource now, is RBAC.
For NetworkLinks, thought, I believe it should be kept separate from Links, because the purpose is different. While Links are used to form a VAN (let's say it as sites that will be able to access all available routing keys on the connected VAN), a NetworkLink has the purpose of connecting distinct VANs, whereas only specific routingKeys will be made available to the other.
Network should be its own IMO. The motivation for it is written as part of the Decision section.
InterNetworkIngress is also something that I see as a new Resource, as we could eventually have multiple of them to expose the same routingKey to different VANs.
There was a problem hiding this comment.
I'm thinking that depending on the type of modification of the CRD, it could provoke an incompatibility with existent custom resources that were created with old CRDs (ie fields reconsidered as required). Adding a new CRD instead of modifying the current ones gives more capacity for change without having deal with the migration of older CRs.
@nluaces this is also something to be careful about. Right now, the change we have here for RouterAccess, is not supposed to break existing resources as a new field was added as part of the Status field. But if we modify something to accommodate new needs, it is something we have to keep in mind.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/apis/skupper/v2alpha1/types_test.go (1)
132-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for
ReleaseUnusedPorts(nil/[]).This exact case (empty
unusedPorts) previously causedStatus.Rolesto be wiped entirely — worth locking in explicitly so a future refactor can't reintroduce it.t.Run("empty input keeps all roles", func(t *testing.T) { r := &RouterAccess{} r.AllocatePort("inter-router", 55671) r.ReleaseUnusedPorts(nil) assert.Equal(t, r.GetAllocatedPortForRole("inter-router"), int32(55671)) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bb4b8f3-6b2a-4cbd-a124-4ddfb66fc5f3
📒 Files selected for processing (3)
config/crd/bases/skupper_inter_network_ingress_crd.yamlpkg/apis/skupper/v2alpha1/types.gopkg/apis/skupper/v2alpha1/types_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/crd/bases/skupper_network_link_crd.yaml (1)
29-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConstrain router ports at the CRD boundary.
These unbounded integer fields currently allow invalid values to be accepted by the API before control-plane validation. Constrain
config/crd/bases/skupper_network_link_crd.yaml#L29-L31andconfig/crd.bases/skupper_network_access_crd.yaml#L22-L24to valid router endpoints, and constrain the status port atconfig/crd/bases/skupper_network_access_crd.yaml#L109-L112to1..65535since allocated ports are non-zero.
♻️ Duplicate comments (1)
config/crd/bases/skupper_inter_network_ingress_crd.yaml (1)
44-44:⚠️ Potential issue | 🔴 CriticalStill invalid CEL string literals.
Line 44 uses Unicode quotation marks instead of ASCII
", so the validation rule cannot be parsed. Fix this generated YAML and the matching marker inpkg/apis/skupper/v2alpha1/types.go, Line 1306, before regenerating the CRD. This repeats the previous review finding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3612cfd4-98aa-4e0e-8e49-029a924442d9
📒 Files selected for processing (4)
config/crd/bases/skupper_inter_network_ingress_crd.yamlconfig/crd/bases/skupper_network_access_crd.yamlconfig/crd/bases/skupper_network_crd.yamlconfig/crd/bases/skupper_network_link_crd.yaml
4 participants
This is the initial PR (split from multi-van effort) that just introduces the new CRDs and a small addition to RouterAccessStatus. Here are the new custom resource types:
Also adds:
Summary by CodeRabbit
Network,NetworkLink,NetworkAccess, andInterNetworkIngress, with validatedspec/statusschemas and improvedkubectlcolumns.InterNetworkIngressinput validation to requirenetworkLinkand/ornetworkAccess.RouterAccessby exposing allocated ports viastatus.roles, improving role port/endpoint tracking.