Security group and Security group rules validation #165

itzikb-redhat · 2024-12-31T12:16:06Z

Add/Update Validations and validations tests to SecurityGroup and SecurityGroupRule

mdbooth · 2025-01-03T09:08:21Z

@itzikb-redhat I just rebased this so it no longer includes #167, no other changes.

api/v1alpha1/securitygroup_types.go

pierreprinetti · 2025-01-03T09:28:47Z

api/v1alpha1/securitygroup_types.go

 // +kubebuilder:validation:Enum:=IPv4;IPv6
 // +kubebuilder:validation:MinLength:=1
-// +kubebuilder:validation:MaxLength:=16
+// +kubebuilder:validation:MaxLength:=4


I'm curious: what does MaxLength do when the field is an enum?

I also wondered this. It's possible that even though it's technically redundant for validation purposes, it may still be used in the runtime complexity calculations? I'll bet @JoelSpeed would know.

For runtime complexity, the openapi schema shows that the field is an enum, so the estimated worst case is based on the maximum length of the enum values. You don't actually need a maxlength on an enum WRT CEL, but it also cannot hurt to include it

I'll remove both length rules

api/v1alpha1/securitygroup_types.go

pierreprinetti · 2025-01-03T09:38:12Z

api/v1alpha1/securitygroup_types.go

+	Ethertype *Ethertype `json:"ethertype"`
+	// If the protocol is [tcp, udp, dccp sctp,udplite] this value must be less than
+	// or equal to the PortRangeMax attribute value.
+	// If the protocol is ICMP, this value must be an ICMP type


If the protocol is ICMP, this value must be an ICMP type

Before reading the code, I can't immediately find a meaning to this sentence

When setting up ICMP, I would naïvely leave PortRangeMin and PortRangeMax empty. Would I be wrong?

First I have a typo, PorRangeMin -> icmp type, PortRangeMax - icmp code
Can you suggest an alternative to the comment?
At least from my testing with the openstack client you can leave them out (Then I think the rule catch all the ICMP types)

pierreprinetti · 2025-01-03T09:43:04Z

api/v1alpha1/securitygroup_types.go

 	// Protocol is the IP protocol can be represented by a string, an
 	// integer, or null
+	// +optional


(I'm commenting the comment) I don't believe you can represent the protocol as an integer here. You probably could if type Protocol's underlying type was an integer and you overrode UnmarshalJSON to also accept strings. But I am not sure that Kubebuilder would like it.

Do you really plan on accepting integers here? And if not, how do I set e.g. vrrp?

I'm not sure what the concern is. If you want vrrp to be set as a number you set the number "112" (as a string)

If you want users to pass numbers as strings, you should tell them explicitly in the comment. Also would null be passed as "null"?

Regarding null I think I'll just remove it. About numbers as strings - Do we want to use also allow integers?

that's a question for @mdbooth @mandre

I think:

We don't allow numbers and just use the strings

We should validate Protocol as an enum

As for null, what does OpenStack do with that? Is it the same as any? If so, I think we should support either any or null (meaning unspecified here), but not both.

If we go with null we should write it as 'if not specified...' or similar.

ok we'll go with not specified and use strings. As for enum - we use a pattern instead

@mdbooth
May I humour you with this

(also cf gophercloud/gophercloud#2442 (comment) )

JoelSpeed · 2025-01-08T16:23:59Z

api/v1alpha1/securitygroup_types.go

@@ -26,37 +26,84 @@ type RuleDirection string
 // +kubebuilder:validation:MaxLength:=16
 type Protocol string

+const (
+	ProtocolANY       Protocol = "any"


Enums in Kube APIs are generally PascalCase, these probably want to follow that convention? Any, ICMP, ICMPv6, IPv6Encap etc

@JoelSpeed I previously gave this some thought in the Image API. These constants are defined by the OpenStack API. In my first version of the Image API I translated the constants, but:

It's a huge mess in the code

It's another source of errors

I suspect it's more confusing for users who are likely using the OpenStack docs as a primary source for more complex things like this

So my current thinking is that we don't translate constants which are defined by the OpenStack API.

This is itself a source of confusion because now users need to understand which constants are OpenStack constants and which are not. I think either choice has trade offs, but my current feeling is that this has the better balance for this API.

@mandre Incidentally, if we agree with this we should document it explicitly in our API conventions.

Perhaps we could have a convention that we point out explicitly in the Godocs that these are 'passthrough' values which correlate exactly to OpenStack values.

Sounds good. I've added the following to our API convention doc:

* Constants coming from OpenStack should be copied verbatim.

If that's the direction of the project, then that is fair. A lot of other projects around clouds have made the same decision. Though of course, you do then juxtapose with core Kube APIs and other APIs that follow kube conventions

If a user knows openstack uses snake case, and knows Kube uses pascal case, I guess they have a 50/50 of getting it right

JoelSpeed · 2025-01-08T16:25:44Z

api/v1alpha1/securitygroup_types.go

 // SecurityGroupRule defines a Security Group rule
 // +kubebuilder:validation:MinProperties:=1
+// +kubebuilder:validation:XValidation:rule="has(self.portRangeMin) == has(self.portRangeMax)",message="PortRangeMin and PortRangeMax should exist together"


Use the serialised version of the field name in the user facing error, also, maybe prefer must to should?

Suggested change

// +kubebuilder:validation:XValidation:rule="has(self.portRangeMin) == has(self.portRangeMax)",message="PortRangeMin and PortRangeMax should exist together"

// +kubebuilder:validation:XValidation:rule="has(self.portRangeMin) == has(self.portRangeMax)",message="portRangeMin and portRangeMax must only exist together"

Have you considered making an optional portRange struct, with min and max fields within that are required?

JoelSpeed · 2025-01-08T16:26:46Z

api/v1alpha1/securitygroup_types.go

-	PortRangeMin *int32 `json:"portRangeMin,omitempty"`
-	PortRangeMax *int32 `json:"portRangeMax,omitempty"`
+	// +kubebuilder:validation:Required
+	Ethertype *Ethertype `json:"ethertype"`


Why a pointer for a required field?

JoelSpeed · 2025-01-08T16:27:44Z

api/v1alpha1/securitygroup_types.go

+	PortRangeMin *uint16 `json:"portRangeMin,omitempty"`
+	// If the protocol is [tcp, udp, dccp sctp,udplite] this value must be greater than
+	// or equal to the PortRangeMin attribute value.
+	// If the protocol is ICMP, this value must be an ICMP type
+	// +optional
+	PortRangeMax *uint16 `json:"portRangeMax,omitempty"`


Don't use uints, they don't translate into an openapi schema in any way that correctly validates the unsigned nature, use int32 and set a minimum value of zero using the appropriate kubebuilder marker

JoelSpeed · 2025-01-08T16:28:32Z

api/v1alpha1/securitygroup_types.go

@@ -26,37 +26,85 @@ type RuleDirection string
 // +kubebuilder:validation:MaxLength:=16
 type Protocol string

+const (


Why aren't you using an enum for protocol?

Add/Update Validations and validations tests to SecurityGroup and SecurityGroupRule

github-actions bot added the semver:major Breaking change label Dec 31, 2024

itzikb-redhat force-pushed the sg-validations branch 2 times, most recently from b25cf0a to dcf0f04 Compare January 2, 2025 19:13

mdbooth force-pushed the sg-validations branch from dcf0f04 to f440e1a Compare January 3, 2025 09:07

pierreprinetti reviewed Jan 3, 2025

View reviewed changes

api/v1alpha1/securitygroup_types.go Outdated Show resolved Hide resolved

pierreprinetti reviewed Jan 3, 2025

View reviewed changes

api/v1alpha1/securitygroup_types.go Show resolved Hide resolved

pierreprinetti reviewed Jan 3, 2025

View reviewed changes

itzikb-redhat force-pushed the sg-validations branch 2 times, most recently from 96f711f to d6864c2 Compare January 8, 2025 16:24

JoelSpeed reviewed Jan 8, 2025

View reviewed changes

itzikb-redhat force-pushed the sg-validations branch from d6864c2 to 5d4cef4 Compare January 8, 2025 16:45

Security group and Security group rules validation

a09c695

Add/Update Validations and validations tests to SecurityGroup and SecurityGroupRule

itzikb-redhat force-pushed the sg-validations branch from 5d4cef4 to a09c695 Compare January 9, 2025 15:02

itzikb-redhat marked this pull request as draft January 9, 2025 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security group and Security group rules validation #165

Security group and Security group rules validation #165

itzikb-redhat commented Dec 31, 2024

mdbooth commented Jan 3, 2025

pierreprinetti Jan 3, 2025

mdbooth Jan 3, 2025

JoelSpeed Jan 3, 2025

itzikb-redhat Jan 5, 2025

pierreprinetti Jan 3, 2025

pierreprinetti Jan 3, 2025

itzikb-redhat Jan 5, 2025

pierreprinetti Jan 3, 2025 •

edited

Loading

itzikb-redhat Jan 5, 2025

pierreprinetti Jan 6, 2025 •

edited

Loading

itzikb-redhat Jan 8, 2025

pierreprinetti Jan 8, 2025

mdbooth Jan 8, 2025

itzikb-redhat Jan 8, 2025

pierreprinetti Jan 8, 2025

JoelSpeed Jan 8, 2025

mdbooth Jan 9, 2025 •

edited

Loading

mandre Jan 9, 2025

JoelSpeed Jan 9, 2025

JoelSpeed Jan 8, 2025

JoelSpeed Jan 8, 2025

JoelSpeed Jan 8, 2025

JoelSpeed Jan 8, 2025

	// +kubebuilder:validation:XValidation:rule="has(self.portRangeMin) == has(self.portRangeMax)",message="PortRangeMin and PortRangeMax should exist together"
	// +kubebuilder:validation:XValidation:rule="has(self.portRangeMin) == has(self.portRangeMax)",message="portRangeMin and portRangeMax must only exist together"

Security group and Security group rules validation #165

Are you sure you want to change the base?

Security group and Security group rules validation #165

Conversation

itzikb-redhat commented Dec 31, 2024

mdbooth commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti Jan 3, 2025 •

edited

Loading

pierreprinetti Jan 6, 2025 •

edited

Loading

mdbooth Jan 9, 2025 •

edited

Loading