OSAC-2340: Add defaults field to NetworkClass proto - #882
openshift-merge-bot[bot] merged 14 commits into
Conversation
|
@danmanor: This pull request references OSAC-2340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
Walkthrough
ChangesNetwork class defaults
Module dependency cleanup
Catalog validation test
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicAPI
participant PrivateAPI
participant Validator
Client->>PublicAPI: Submit NetworkClass request
PublicAPI->>PrivateAPI: Forward request without spec
PrivateAPI->>Validator: Validate spec.defaults
Validator-->>PrivateAPI: Validation result
PrivateAPI-->>PublicAPI: Persisted NetworkClass
PublicAPI-->>Client: NetworkClass with output-only defaults
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ 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 |
danmanor
changed the title
danmanor
changed the title
|
/retest |
|
Re-triggered failed runs:
|
| "defaults.virtual_network_cidr: %v", err) | ||
| } | ||
| vnPrefix, _ = netip.ParsePrefix(parsed) | ||
| _ = vnPrefix |
There was a problem hiding this comment.
I think this is not needed.
27aba83 to
5587c34
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/servers/network_classes_server_test.go`:
- Around line 1094-1206: Add an Update-path validation test alongside the
existing negative cases, using an existing valid NetworkClass, an invalid
defaults value such as an invalid CIDR, and an UpdateMask containing “defaults”.
Invoke the server’s Update operation and assert it returns an error, verifying
validateNetworkClass validates merged defaults during updates.
In `@internal/servers/private_network_classes_server.go`:
- Around line 395-408: Handle the error returned by netip.ParsePrefix in
validateNetworkDefaults instead of discarding it. If reparsing parsed fails,
return an appropriate InvalidArgument grpcstatus error for
defaults.virtual_network_cidr; only assign vnPrefix after successful parsing so
subsequent Contains checks never receive an invalid zero-value prefix.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fdef774b-dcb8-4ad2-9379-fc1c1b1ee2d8
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/network_class_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/network_class_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
go.modinternal/servers/network_classes_server.gointernal/servers/network_classes_server_test.gointernal/servers/private_network_classes_server.goproto/private/osac/private/v1/network_class_type.protoproto/public/osac/public/v1/network_class_type.proto
💤 Files with no reviewable changes (1)
- go.mod
|
|
||
| if vnCIDR != "" { | ||
| subnetPrefix, _ := netip.ParsePrefix(subnetCIDR) | ||
| if !vnPrefix.Contains(subnetPrefix.Addr()) || subnetPrefix.Bits() < vnPrefix.Bits() { |
There was a problem hiding this comment.
Nit: the discarded error (_) on netip.ParsePrefix is safe here since parseAndValidateCIDR already validated the string above, but it's fragile if someone refactors this later without noticing the precondition. Consider either assigning and checking, or adding a short comment noting that validation already happened.
Also, the condition !Contains(addr) || Bits() < vnBits is correct but reads a bit inside-out — the < vnPrefix.Bits() part means "subnet is wider than the VN". Flipping to Bits() >= vnPrefix.Bits() with adjusted logic would make the intent slightly more self-documenting ("subnet must be equal or narrower than VN").
There was a problem hiding this comment.
Fair point on both. I'll handle the error instead of discarding it — will return grpccodes.Internal since a re-parse failure after validation would indicate an internal inconsistency rather than bad input. And I'll flip the containment condition to make it read more naturally.
| "defaults.subnet_cidr: %v", err) | ||
| } | ||
|
|
||
| if vnCIDR != "" { |
There was a problem hiding this comment.
If subnet_cidr is set without virtual_network_cidr, the containment check is skipped and the defaults are persisted with a subnet CIDR but no parent VN CIDR. When the tenant-onboarding code later tries to use these defaults, it would have a subnet to create but no VirtualNetwork to put it in.
Should this be rejected here (i.e., if subnet_cidr is set, require virtual_network_cidr too)? Or is the onboarding code expected to handle partial defaults gracefully?
There was a problem hiding this comment.
Good catch — you're right, this is a real gap. The default-networking EP treats defaults as a bundle: tenant onboarding (OSAC-2341) creates all three resources (VN + Subnet + SG) together, and Subnet is a child of VirtualNetwork in the resource hierarchy — it can't exist without a parent.
I'll add validation to reject subnet_cidr when virtual_network_cidr is empty.
| // defaults to auto-create a VirtualNetwork, Subnet, and SecurityGroup for the tenant. | ||
| NetworkDefaults defaults = 12 [ | ||
| (google.api.field_behavior) = OUTPUT_ONLY | ||
| ]; |
There was a problem hiding this comment.
This looks like a candidate for a spec.defaults field.
There was a problem hiding this comment.
Done — moved defaults under a new NetworkClassSpec wrapper so it's now spec.defaults, following the standard {id, metadata, spec, status} pattern.
| message NetworkDefaults { | ||
| // Default CIDR for auto-created tenant VirtualNetwork. Must be valid IPv4 CIDR notation. | ||
| // Example: "10.0.0.0/16" | ||
| string virtual_network_cidr = 1; |
There was a problem hiding this comment.
Consider using protovalidate to validate the format, as explained here: https://github.com/osac-project/fulfillment-service/blob/main/docs/API.md#validation-constraints.
There was a problem hiding this comment.
Done — added buf.validate CIDR format regex on both virtual_network_cidr and subnet_cidr, plus a message-level CEL constraint enforcing that subnet_cidr requires virtual_network_cidr to be set. Deeper semantic validation (valid octets, prefix lengths, subnet containment) stays in the Go server since CEL can't do CIDR arithmetic.
| // Default CIDR for auto-created tenant Subnet. Must be valid IPv4 CIDR notation and fall within the | ||
| // virtual_network_cidr range. | ||
| // Example: "10.0.1.0/24" | ||
| string subnet_cidr = 2; |
There was a problem hiding this comment.
Consider using protovalidate.
There was a problem hiding this comment.
Addressed in the same commit — see reply above.
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…erver Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Reject subnet_cidr when virtual_network_cidr is empty (subnet requires a parent VN per the resource hierarchy), handle netip.ParsePrefix errors instead of discarding them, improve containment condition readability, and add tests for subnet-without-VN and update-path validation. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Move defaults from a top-level field to spec.defaults following the
standard {id, metadata, spec, status} object pattern. Add protovalidate
CIDR format annotations and a CEL constraint requiring virtual_network_cidr
when subnet_cidr is set.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Dan Manor <dmanor@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Replace the regex pattern that accepted invalid octets (e.g. 999.999.999.999/99) with protovalidate's isIpPrefix(4, true) CEL function for proper IPv4 CIDR validation. IPv6 is not supported for defaults per the default-networking EP. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Rename virtual_network_cidr/subnet_cidr to virtual_network_ipv4_cidr/ subnet_ipv4_cidr and add ipv6 counterparts to match the VirtualNetwork and Subnet proto patterns. Use isIpPrefix CEL for proper CIDR validation instead of loose regex. Add per-family subnet-requires-VN constraints. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
73097e6 to
c15e48f
Compare
PR osac-project#866 removed the Cores field from ComputeInstanceSpec but didn't update the test added by PR osac-project#783 in catalog_item_validation_test.go. Replace with RunStrategy to test the same unlisted-field rejection. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
|
/override ci/prow/unit |
|
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/unit DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Tenant onboarding now creates five default resources: VN, IPv4 Subnet, IPv6 Subnet, SecurityGroup, and optionally a NATGateway. The new boolean field signals whether to auto-allocate an ExternalIP and create a NATGateway for SNAT on the default VirtualNetwork. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/servers/network_classes_server_test.go`:
- Around line 956-1241: Add IPv6 validation coverage alongside the existing IPv4
cases in the Defaults tests, using NetworkDefaults fields VirtualNetworkIpv6Cidr
and SubnetIpv6Cidr. Cover invalid IPv6 CIDR format, a subnet outside the virtual
network, and a subnet specified without its virtual network; assert creation
fails with the corresponding validation messages.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c1ffa6d2-ca05-4bae-8e61-e8c690752a5b
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/network_class_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/network_class_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/network_class_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
go.modinternal/servers/catalog_item_validation_test.gointernal/servers/network_classes_server.gointernal/servers/network_classes_server_test.gointernal/servers/private_network_classes_server.goproto/private/osac/private/v1/network_class_type.protoproto/public/osac/public/v1/network_class_type.proto
💤 Files with no reviewable changes (1)
- go.mod
| Describe("Defaults", func() { | ||
| validDefaults := func() *privatev1.NetworkDefaults { | ||
| return privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "10.0.0.0/16", | ||
| SubnetIpv4Cidr: "10.0.1.0/24", | ||
| IngressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_TCP, | ||
| PortFrom: new(int32(22)), | ||
| PortTo: new(int32(22)), | ||
| Ipv4Cidr: new("0.0.0.0/0"), | ||
| }.Build(), | ||
| }, | ||
| EgressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_ALL, | ||
| Ipv4Cidr: new("0.0.0.0/0"), | ||
| }.Build(), | ||
| }, | ||
| }.Build() | ||
| } | ||
|
|
||
| createNetworkClassWithDefaults := func(defaults *privatev1.NetworkDefaults) *privatev1.NetworkClass { | ||
| response, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC with defaults", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| return response.GetObject() | ||
| } | ||
|
|
||
| It("Create with valid defaults persists and returns them", func() { | ||
| nc := createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| Expect(nc.GetSpec().GetDefaults()).ToNot(BeNil()) | ||
| Expect(nc.GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(Equal("10.0.0.0/16")) | ||
| Expect(nc.GetSpec().GetDefaults().GetSubnetIpv4Cidr()).To(Equal("10.0.1.0/24")) | ||
| Expect(nc.GetSpec().GetDefaults().GetIngressRules()).To(HaveLen(1)) | ||
| Expect(nc.GetSpec().GetDefaults().GetEgressRules()).To(HaveLen(1)) | ||
| }) | ||
|
|
||
| It("Get after create returns defaults", func() { | ||
| nc := createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| getResponse, err := privateServer.Get(ctx, privatev1.NetworkClassesGetRequest_builder{ | ||
| Id: nc.GetId(), | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| retrieved := getResponse.GetObject() | ||
| Expect(retrieved.GetSpec().GetDefaults()).ToNot(BeNil()) | ||
| Expect(retrieved.GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(Equal("10.0.0.0/16")) | ||
| Expect(retrieved.GetSpec().GetDefaults().GetSubnetIpv4Cidr()).To(Equal("10.0.1.0/24")) | ||
| Expect(retrieved.GetSpec().GetDefaults().GetIngressRules()).To(HaveLen(1)) | ||
| Expect(retrieved.GetSpec().GetDefaults().GetIngressRules()[0].GetProtocol()).To(Equal(privatev1.Protocol_PROTOCOL_TCP)) | ||
| Expect(retrieved.GetSpec().GetDefaults().GetIngressRules()[0].GetPortFrom()).To(BeNumerically("==", 22)) | ||
| }) | ||
|
|
||
| It("List after create returns defaults in items", func() { | ||
| createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| listResponse, err := privateServer.List(ctx, privatev1.NetworkClassesListRequest_builder{}.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(listResponse.GetItems()).To(HaveLen(1)) | ||
| Expect(listResponse.GetItems()[0].GetSpec().GetDefaults()).ToNot(BeNil()) | ||
| Expect(listResponse.GetItems()[0].GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(Equal("10.0.0.0/16")) | ||
| }) | ||
|
|
||
| It("Update defaults via field mask replaces entire defaults", func() { | ||
| nc := createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| newDefaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "172.16.0.0/12", | ||
| SubnetIpv4Cidr: "172.16.1.0/24", | ||
| }.Build() | ||
|
|
||
| updateResponse, err := privateServer.Update(ctx, privatev1.NetworkClassesUpdateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Id: nc.GetId(), | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: newDefaults}.Build(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.defaults"}}, | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| updated := updateResponse.GetObject() | ||
| Expect(updated.GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(Equal("172.16.0.0/12")) | ||
| Expect(updated.GetSpec().GetDefaults().GetSubnetIpv4Cidr()).To(Equal("172.16.1.0/24")) | ||
| Expect(updated.GetSpec().GetDefaults().GetIngressRules()).To(BeEmpty()) | ||
| Expect(updated.GetSpec().GetDefaults().GetEgressRules()).To(BeEmpty()) | ||
| }) | ||
|
|
||
| It("Update defaults to nil clears them", func() { | ||
| nc := createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| updateResponse, err := privateServer.Update(ctx, privatev1.NetworkClassesUpdateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Id: nc.GetId(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.defaults"}}, | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(updateResponse.GetObject().GetSpec().GetDefaults()).To(BeNil()) | ||
| }) | ||
|
|
||
| It("Create without defaults succeeds", func() { | ||
| nc := createNetworkClass() | ||
| Expect(nc.GetSpec().GetDefaults()).To(BeNil()) | ||
| }) | ||
|
|
||
| It("Defaults with CIDRs only succeeds", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "10.0.0.0/16", | ||
| SubnetIpv4Cidr: "10.0.1.0/24", | ||
| }.Build() | ||
| nc := createNetworkClassWithDefaults(defaults) | ||
| Expect(nc.GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(Equal("10.0.0.0/16")) | ||
| Expect(nc.GetSpec().GetDefaults().GetIngressRules()).To(BeEmpty()) | ||
| }) | ||
|
|
||
| It("Defaults with rules only succeeds", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| IngressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_TCP, | ||
| PortFrom: new(int32(443)), | ||
| PortTo: new(int32(443)), | ||
| Ipv4Cidr: new("0.0.0.0/0"), | ||
| }.Build(), | ||
| }, | ||
| }.Build() | ||
| nc := createNetworkClassWithDefaults(defaults) | ||
| Expect(nc.GetSpec().GetDefaults().GetVirtualNetworkIpv4Cidr()).To(BeEmpty()) | ||
| Expect(nc.GetSpec().GetDefaults().GetIngressRules()).To(HaveLen(1)) | ||
| }) | ||
|
|
||
| It("Invalid virtual_network_ipv4_cidr fails validation", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "not-a-cidr", | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC invalid VN CIDR", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("virtual_network_ipv4_cidr")) | ||
| }) | ||
|
|
||
| It("Invalid subnet_ipv4_cidr fails validation", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "10.0.0.0/16", | ||
| SubnetIpv4Cidr: "invalid", | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC invalid subnet CIDR", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("subnet_ipv4_cidr")) | ||
| }) | ||
|
|
||
| It("Subnet CIDR not within virtual_network_ipv4_cidr fails", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "10.0.0.0/16", | ||
| SubnetIpv4Cidr: "192.168.1.0/24", | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC subnet outside VN", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("not within")) | ||
| }) | ||
|
|
||
| It("Subnet CIDR without virtual_network_ipv4_cidr fails", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| SubnetIpv4Cidr: "10.0.1.0/24", | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC subnet without VN", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("subnet_ipv4_cidr requires")) | ||
| Expect(err.Error()).To(ContainSubstring("virtual_network_ipv4_cidr")) | ||
| }) | ||
|
|
||
| It("Ingress rule with invalid protocol fails", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| IngressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_UNSPECIFIED, | ||
| Ipv4Cidr: new("0.0.0.0/0"), | ||
| }.Build(), | ||
| }, | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC invalid rule protocol", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("protocol is required")) | ||
| }) | ||
|
|
||
| It("TCP rule without port range fails", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| IngressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_TCP, | ||
| PortFrom: new(int32(22)), | ||
| Ipv4Cidr: new("0.0.0.0/0"), | ||
| }.Build(), | ||
| }, | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC TCP missing port_to", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("port")) | ||
| }) | ||
|
|
||
| It("Rule with invalid CIDR fails", func() { | ||
| defaults := privatev1.NetworkDefaults_builder{ | ||
| EgressRules: []*privatev1.SecurityRule{ | ||
| privatev1.SecurityRule_builder{ | ||
| Protocol: privatev1.Protocol_PROTOCOL_ALL, | ||
| Ipv4Cidr: new("not-a-cidr"), | ||
| }.Build(), | ||
| }, | ||
| }.Build() | ||
| _, err := privateServer.Create(ctx, privatev1.NetworkClassesCreateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Title: "NC invalid rule CIDR", | ||
| ImplementationStrategy: "ovn-kubernetes", | ||
| FabricManager: "netris", | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: defaults}.Build(), | ||
| }.Build(), | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("CIDR")) | ||
| }) | ||
|
|
||
| It("Update with invalid defaults via field mask fails validation", func() { | ||
| nc := createNetworkClassWithDefaults(validDefaults()) | ||
|
|
||
| invalidDefaults := privatev1.NetworkDefaults_builder{ | ||
| VirtualNetworkIpv4Cidr: "not-a-cidr", | ||
| }.Build() | ||
| _, err := privateServer.Update(ctx, privatev1.NetworkClassesUpdateRequest_builder{ | ||
| Object: privatev1.NetworkClass_builder{ | ||
| Id: nc.GetId(), | ||
| Spec: privatev1.NetworkClassSpec_builder{Defaults: invalidDefaults}.Build(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.defaults"}}, | ||
| }.Build()) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("virtual_network_ipv4_cidr")) | ||
| }) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No test coverage for the IPv6 CIDR-pair validation branch.
All defaults tests exercise only virtual_network_ipv4_cidr/subnet_ipv4_cidr. validateNetworkDefaults also runs the IPv6 pair through validateDefaultCIDRPair (invalid format, containment, missing-VN cases) but none of that is asserted here — a regression in the IPv6 branch wouldn't be caught by this suite.
Add IPv6 counterparts to at least the "invalid CIDR", "not within", and "missing VN" cases (e.g. VirtualNetworkIpv6Cidr: "fd00::/48", SubnetIpv6Cidr: "fd00:0:0:1::/64").
🤖 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 `@internal/servers/network_classes_server_test.go` around lines 956 - 1241, Add
IPv6 validation coverage alongside the existing IPv4 cases in the Defaults
tests, using NetworkDefaults fields VirtualNetworkIpv6Cidr and SubnetIpv6Cidr.
Cover invalid IPv6 CIDR format, a subnet outside the virtual network, and a
subnet specified without its virtual network; assert creation fails with the
corresponding validation messages.
|
/override ci/prow/unit |
|
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/unit DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This branch was previously deployed
5 participants
OSAC-2340: Add defaults field to NetworkClass proto
Jira: https://redhat.atlassian.net/browse/OSAC-2340
Epic: OSAC-2339 — Default Networking
Summary
Adds a
NetworkDefaultsmessage andspec.defaultsfield to NetworkClass in both public and private protos. This is the first task in the Default Networking epic — it defines the contract that downstream tasks (tenant onboarding default resource creation, installer configuration) depend on. Admins configure default CIDRs and security group rules on the NetworkClass; the system uses these to auto-create default networking resources at tenant onboarding.Changes
Proto definitions:
NetworkClassSpecwrapper withNetworkDefaults defaultsfield, following the standard{id, metadata, spec, status}object patternNetworkDefaultshas per-address-family CIDR fields matching VirtualNetwork/Subnet conventions:virtual_network_ipv4_cidr,virtual_network_ipv6_cidr,subnet_ipv4_cidr,subnet_ipv6_cidr, plusrepeated SecurityRule ingress_rules/egress_rulesprotovalidateCEL annotations:isIpPrefix(4, true)/isIpPrefix(6, true)for CIDR validation, and per-family subnet-requires-VN message-level constraintsspecfield on public API isOUTPUT_ONLY— tenants can see defaults but only admins can set them via the private APIServer (private):
validateDefaultCIDRPair()— shared helper validating CIDR format, subnet containment within VN CIDR, and the subnet-requires-VN constraint for each address familyapplyNetworkClassUpdate— handles"spec"and"spec.defaults"field mask pathsServer (public):
specfield added toAddIgnoredFieldsoninMapperBug fix (unrelated):
catalog_item_validation_test.gocaused byCoresfield removal in PR OSAC-1222: remove cores/memory_gib from ComputeInstance in favor of instance_type #866 without updating the test added by PR OSAC-1416: enforce field_definitions as complete contract on catalog item usage #783No database migration needed — the JSONB
datacolumn stores new proto fields automatically.Testing
Acceptance Criteria
spec.defaultswith per-family CIDRs and security group rulesspec.defaultsfield is persisted and retrievable via Get/List operationsspec.defaultsSummary by CodeRabbit