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

nit: Still there, btw.

Removed. Also from RuleDirection

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?

Can you please clarify?

mdbooth · 2025-01-14T10:21:53Z

api/v1alpha1/securitygroup_types.go

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

+const (
+	ProtocolANY       Protocol = "any"


@itzikb-redhat We should delete ProtocolANY now we've decided not to use it.

mdbooth · 2025-01-14T10:23:00Z

api/v1alpha1/securitygroup_types.go

+	Min PortNumber `json:"min"`
+	// +required
+	Max PortNumber `json:"max"`
+}


nit: can we have a blank line between these. Surprised the linter didn't complain 🤔

mdbooth · 2025-01-14T10:24:19Z

api/v1alpha1/securitygroup_types.go

 // SecurityGroupRule defines a Security Group rule
 // +kubebuilder:validation:MinProperties:=1
+// +kubebuilder:validation:XValidation:rule="(!has(self.portRange)|| !(self.protocol == 'tcp'|| self.protocol == 'udp' || self.protocol == 'dccp' || self.protocol == 'sctp' || self.protocol == 'udplite') || (self.portRange.min <= self.portRange.max))",message="portRangeMax should be equal or greater than portRange.min"


Please can you move this validation to PortRangeSpec? We can just implement it unconditionally there. i.e. Everywhere that PortRangeSpec is defined:

It must have min and max

Max >= min

Anywhere that PortRangeSpec is not defined doesn't need to worry about these rules.

Edit: See below. There might actually be a case where max can be less than min? My feeling that we should put ICMP in its own field grows.

Moved. IIUC Max should be >=0 for specific protocols

mdbooth · 2025-01-14T10:32:09Z

api/v1alpha1/securitygroup_types.go

+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.min >= 0 && self.portRange.min <= 255)",message="When protocol is ICMP or ICMPv6 portRange.min should be between 0 and 255"
+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.max >= 0 && self.portRange.max <= 255)",message="When protocol is ICMP or ICMPv6 portRange.max should be between 0 and 255"


I've been tempted to say that we should separately define an ICMP Protocol field in our API which we marshal into portRange internally. I'm still tempted, tbh. However, I worry that may be straying too far from the admittedly terrible OpenStack API. Would we be creating unnecessary maintenance for ourselves and impedance for our users? I don't know how much I want to do this kind of thing in general. Would an abundance of this kind of difference it make it harder for OpenStack devs to contribute?

Thoughts @mandre @JoelSpeed

I suspect it will indeed make our (and our users') life miserable if we differ too much from the OpenStack API. Wasn't it the reason to align with OpenStack constants?

For posterity, this was the balance we agreed on. We'll stay close to the terrible OpenStack API.

mdbooth · 2025-01-14T10:37:33Z

api/v1alpha1/securitygroup_types.go

+	// If the protocol is ICMP, this PortRamge.Min must be an ICMP code and PortRange.Max
+	// should be an ICMP type


WHA? So for ICMP max might be less than min?

mdbooth · 2025-01-14T11:08:27Z

test/apivalidations/securitygroup_test.go

+		Entry(string(orcv1alpha1.ProtocolIPV6NONXT), orcv1alpha1.ProtocolIPV6NONXT, orcv1alpha1.EthertypeIPv6),
+		Entry(string(orcv1alpha1.ProtocolIPV6OPTS), orcv1alpha1.ProtocolIPV6OPTS, orcv1alpha1.EthertypeIPv6),
+		Entry(string(orcv1alpha1.ProtocolIPV6ROUTE), orcv1alpha1.ProtocolIPV6ROUTE, orcv1alpha1.EthertypeIPv6),
+	)


We need a negative test here: it should not permit invalid protocol.

Edit: it's below.
nit: Might be better to move it here so it's next to the other protocol tests.

mdbooth · 2025-01-14T11:14:22Z

test/apivalidations/securitygroup_test.go

+			sgRulePatchSpec := applyconfigv1alpha1.SecurityGroupRule().WithEthertype(ethertype).WithProtocol(protocol)
+			patch.Spec.WithResource(applyconfigv1alpha1.SecurityGroupResourceSpec().WithRules(sgRulePatchSpec.WithPortRange(
+				&applyconfigv1alpha1.PortRangeSpecApplyConfiguration{
+					Min: ptr.To(v1alpha1.PortNumber(256)), Max: ptr.To(v1alpha1.PortNumber(0))})))


Could do with a comment here. The reason this should fail is that min > 255, not (bizarrely, see ICMP portRange abuse) because min > max.

mdbooth · 2025-01-14T11:16:45Z

test/apivalidations/securitygroup_test.go

+			patch.Spec.WithResource(applyconfigv1alpha1.SecurityGroupResourceSpec().WithRules(sgRulePatchSpec.WithPortRange(
+				&applyconfigv1alpha1.PortRangeSpecApplyConfiguration{
+					Min: ptr.To(v1alpha1.PortNumber(256)), Max: ptr.To(v1alpha1.PortNumber(0))})))
+			Expect(applyObj(ctx, securityGroup, patch)).NotTo(Succeed(), "create security group")


Incidentally, "create security group" is context we'll get if this fails. As we write more of these it might be good to add better context. e.g. Here we might write something like "ICMP security group with portRange.min > 255".

mdbooth · 2025-01-14T11:19:02Z

api/v1alpha1/securitygroup_types.go

+	// Protocol is the IP protocol can be represented by a string or an
+	// integer represented as a string.


We should remove the reference to an integer here.

Can we still create a security group rule for a protocol that is not defined in ORC and part of the enum? OpenStack allows to pass the protocol number.

I think we agreed on working only with defined protocols and without numbers

mdbooth · 2025-01-14T11:23:51Z

test/apivalidations/securitygroup_test.go

+	It("should not permit invalid protocol", func(ctx context.Context) {
+		securityGroup := securityGroupStub(namespace)
+		patch := baseSecurityGroupPatch(securityGroup)
+		patch.Spec.WithResource(applyconfigv1alpha1.SecurityGroupResourceSpec().WithRules(baseSGRulePatchSpec().WithProtocol("foo")))


I can't work out what would make this test fail. If you change the NotTo to To below this test should fail and we should see which validation is tripping it. I'm curious, because we didn't make protocol an enum yet unless I missed that.

mandre · 2025-01-14T12:53:02Z

api/v1alpha1/securitygroup_types.go

+	EthertypeIPv6 Ethertype = "IPv6"
+)
+
+// +kubebuilder:validation:Minimum:=1


The port number can also represent the ICMP type when the protocol is ICMP, and the type can be 0.

Suggested change

// +kubebuilder:validation:Minimum:=1

// +kubebuilder:validation:Minimum:=0

mandre · 2025-01-14T14:30:13Z

api/v1alpha1/securitygroup_types.go

+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.min >= 0 && self.portRange.min <= 255)",message="When protocol is ICMP or ICMPv6 portRange.min should be between 0 and 255"
+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.max >= 0 && self.portRange.max <= 255)",message="When protocol is ICMP or ICMPv6 portRange.max should be between 0 and 255"


I suspect it will indeed make our (and our users') life miserable if we differ too much from the OpenStack API. Wasn't it the reason to align with OpenStack constants?

mandre · 2025-01-14T14:39:56Z

examples/bases/devstack/securitygroup.yaml

@@ -13,13 +13,14 @@ spec:
    rules:
    - direction: ingress
      protocol: tcp
-      portRangeMin: 22
-      portRangeMax: 22
-      ethertype: IPv4


We should not remove the ethertype in this example. This PR makes it required.

Done. How didn't it fail?

Because this example is not exercised in CI. Only the cirros one is.

mandre · 2025-01-14T14:43:00Z

pkg/clients/applyconfiguration/api/v1alpha1/portrangespec.go

@@ -0,0 +1,52 @@
+/*
+Copyright 2024 The ORC Authors.


mandre · 2025-01-14T14:55:04Z

api/v1alpha1/securitygroup_types.go

+	// Protocol is the IP protocol can be represented by a string or an
+	// integer represented as a string.


Can we still create a security group rule for a protocol that is not defined in ORC and part of the enum? OpenStack allows to pass the protocol number.

Add/Update Validations and validations tests to SecurityGroup and SecurityGroupRule

mdbooth · 2025-01-15T10:13:57Z

api/v1alpha1/securitygroup_types.go

+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.min >= 0 && self.portRange.min <= 255)",message="When protocol is ICMP or ICMPv6 portRange.min should be between 0 and 255"
+// +kubebuilder:validation:XValidation:rule="!(self.protocol == 'icmp' || self.protocol == 'icmpv6') || !has(self.portRange)|| (self.portRange.max >= 0 && self.portRange.max <= 255)",message="When protocol is ICMP or ICMPv6 portRange.max should be between 0 and 255"


For posterity, this was the balance we agreed on. We'll stay close to the terrible OpenStack API.

mdbooth · 2025-01-15T10:16:23Z

@mandre FWIW I manually removed the semver_major label because this doesn't break a released API.

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 2 times, most recently from 5d4cef4 to a09c695 Compare January 9, 2025 15:02

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

itzikb-redhat force-pushed the sg-validations branch 4 times, most recently from 1713d4c to 82835d0 Compare January 12, 2025 08:27

itzikb-redhat marked this pull request as ready for review January 12, 2025 08:44

itzikb-redhat force-pushed the sg-validations branch 2 times, most recently from 3c7172b to 8335adf Compare January 14, 2025 07:52

mdbooth reviewed Jan 14, 2025

View reviewed changes

mandre reviewed Jan 14, 2025

View reviewed changes

Security group and Security group rules validation

f23d352

Add/Update Validations and validations tests to SecurityGroup and SecurityGroupRule

itzikb-redhat force-pushed the sg-validations branch from 8335adf to f23d352 Compare January 15, 2025 08:19

mdbooth approved these changes Jan 15, 2025

View reviewed changes

mdbooth removed the semver:major Breaking change label Jan 15, 2025

mandre approved these changes Jan 15, 2025

View reviewed changes

mandre merged commit b3d0eb1 into k-orc:main Jan 15, 2025
9 checks passed

mandre mentioned this pull request Jan 23, 2025

SecurityGroup API Review #119

Closed

4 tasks

	// +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"

		// If the protocol is ICMP, this PortRamge.Min must be an ICMP code and PortRange.Max
		// should be an ICMP type

		// Protocol is the IP protocol can be represented by a string or an
		// integer represented as a string.

	// +kubebuilder:validation:Minimum:=1
	// +kubebuilder:validation:Minimum:=0

Security group and Security group rules validation #165

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Jan 15, 2025

pierreprinetti Jan 3, 2025 •

edited

Loading

pierreprinetti Jan 6, 2025 •

edited

Loading

mdbooth Jan 9, 2025 •

edited

Loading