Fix kubebuilder validation for SecurityGroup resources #1710

cjschaef · 2024-04-03T21:51:20Z

Some of the kubebuilder validation logic was written inversely and needs to be fixed so validation is done correctly.

What this PR does / why we need it: Some kubebuilder validation is written inversely, or is incorrect, causing issues with valid definitions.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Fix kubebuiler validation for new SecurityGroup related resources

k8s-ci-robot · 2024-04-03T21:51:30Z

Hi @cjschaef. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

netlify · 2024-04-03T21:51:37Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`da5d10b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/66101a6512e95e0008954342
😎 Deploy Preview	https://deploy-preview-1710--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Karthik-K-N · 2024-04-04T03:03:17Z

/ok-to-test

Karthik-K-N · 2024-04-04T07:19:38Z

api/v1beta2/types.go

@@ -269,7 +269,7 @@ type PortRange struct {
 }

 // SecurityGroup defines a VPC Security Group that should exist or be created within the specified VPC, with the specified Security Group Rules.
-// +kubebuilder:validation:XValidation:rule="!has(self.id) && !has(self.name)",message="either an id or name must be specified"
+// +kubebuilder:validation:XValidation:rule="has(self.id) || has(self.name)",message="either an id or name must be specified"


Could you please help me understand this, How it would affect, User should set either id or name right?

https://playcel.undistro.io/?content=H4sIAAAAAAAAA3VRS0sDMRD%2BK%2BN6aAu1xdeltyJFkapFkSLsZbo73Q0kkyXJti7W%2F%2B5ku6hVSg4Z8mW%2Bx8xHkmPAZJKcwh05AuUhlHJxVQeIkJTwNn2Yg3Vw%2F%2FL0CGvrDIZRyil7ymqnQnPrbF1NUgZQ%2BQTOLy6vrlNOhgm9V468V5ZFYDyGJenMGoJgW5Gb2RwWGptC2jk%2FSVm%2BHL5FO8hiIZDDLKgNCcdq6j2ZlW6gslvxnAPxRjnLhjhEbpHVVrKgEEhNTrXIVoVyr2uNsQyzb3cwRy5qLAj6oj8YRSetmzNYSj6CxtautfaTCEqR7j69emqJ0RGCQLH2Kqc4q1%2BjHB6ZZceyiMzQe66516bYoK7xiDgWqNiHP7vqiGZd%2FtiXWa1JJic9di0MaCpNvjPmK%2BUwYml3SvT9g6WOVD6A3Q7%2BA4yGBsnnFwKmXNFAAgAA

Yes, a name or an id must be provided.

@Karthik-K-N isn't this appropriate !has(self.id) && !has(self.name) ?

No, Sometimes CEL expressions are confusing but the main logic is, The message will be displayed when the expression evaluates to false, In the above case if we set !has(self.id) && !has(self.name), it does not allow us to set either one of id or name.
Example when we set id !true && !false --> false && true --> false = either an id or name must be specified

More info: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules

Karthik-K-N

The expressions can be optimised to provide better error messages to user, But its not a blocking issue but an improvisation and good to have
Either we can add a TODO to fix it later or spend sometime to fix it now itself its upto you.

Karthik-K-N · 2024-04-05T03:56:42Z

api/v1beta2/types.go

@@ -298,8 +298,7 @@ type SecurityGroup struct {

 // SecurityGroupRule defines a VPC Security Group Rule for a specified Security Group.
 // +kubebuilder:validation:XValidation:rule="(has(self.destination) && !has(self.source)) || (!has(self.destination) && has(self.source))",message="both destination and source cannot be provided"
-// +kubebuilder:validation:XValidation:rule="has(self.destination) && self.direction == 'inbound'",message="destinationis not valid for SecurityGroupRuleDirectionInbound direction"
-// +kubebuilder:validation:XValidation:rule="has(self.source) && self.direction == 'outbound'",message="source is not valid for SecurityGroupRuleDirectionOutbound direction"
+// +kubebuilder:validation:XValidation:rule="(self.direction == 'inbound' && !has(self.destination) && has(self.source)) || (self.direction == 'outbound' && has(self.destination) && !has(self.source))",message="destination is not valid for SecurityGroupRuleDirectionInbound direction and source is not valid for SecruityGroupRuleDirectionOutbound"


I have spent some time in understanding validations, Here are my thoughts

The above expression may confuse with very generic error , example

securityGroupRule: - action: "action" direction: "inbound" spec.securityGroupRule[0]: Invalid value: "object": destination is not valid for SecurityGroupRuleDirectionInbound direction and source is not valid for SecruityGroupRuleDirectionOutbound

Instead of combining them into one line expression with combined error messages, It should be split to display appropriate validation failure messages, for example

// +kubebuilder:validation:XValidation:rule="self.direction == 'inbound' ? has(self.source) : true",message="source should be set for SecurityGroupRuleDirectionInbound direction" // +kubebuilder:validation:XValidation:rule="self.direction == 'inbound' ? (!has(self.destination)) : true",message="destination is not valid for SecurityGroupRuleDirectionInbound direction" // +kubebuilder:validation:XValidation:rule="self.direction == 'outbound' ? has(self.destination) : true",message="destination should be set for SecruityGroupRuleDirectionOutbound direction" // +kubebuilder:validation:XValidation:rule="self.direction == 'outbound' ? (!has(self.source)) : true",message="source is not valid for SecruityGroupRuleDirectionOutbound direction"

Sure I can split the validation logic up.

Updated, and performed some basic (non-exhaustive) validation.

Thanks for updating

Some of the kubebuilder validation logic was written inversely and needs to be fixed so validation is done correctly.

Karthik-K-N

/lgtm

Prajyot-Parab · 2024-04-08T07:41:52Z

@cjschaef can you please update the release note section as well in this PR.

cjschaef · 2024-04-08T14:10:55Z

Updated

Prajyot-Parab · 2024-04-10T05:09:27Z

/easycla

k8s-ci-robot · 2024-04-10T05:10:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjschaef, Prajyot-Parab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Prajyot-Parab]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Apr 3, 2024

k8s-ci-robot requested review from Amulyam24 and Prajyot-Parab April 3, 2024 21:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 3, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 3, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2024

Karthik-K-N reviewed Apr 4, 2024

View reviewed changes

Karthik-K-N reviewed Apr 5, 2024

View reviewed changes

cjschaef force-pushed the patch_sg_validation branch from 8ae1d80 to 0189f04 Compare April 5, 2024 14:48

Fix kubebuilder validation for SecurityGroup resources

da5d10b

Some of the kubebuilder validation logic was written inversely and needs to be fixed so validation is done correctly.

cjschaef force-pushed the patch_sg_validation branch from 0189f04 to da5d10b Compare April 5, 2024 15:36

Karthik-K-N reviewed Apr 8, 2024

View reviewed changes

k8s-ci-robot assigned Karthik-K-N Apr 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 8, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 8, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 10, 2024

Prajyot-Parab approved these changes Apr 10, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2024

k8s-ci-robot merged commit 6a19629 into kubernetes-sigs:main Apr 10, 2024
12 of 13 checks passed

cjschaef deleted the patch_sg_validation branch April 15, 2024 13:43

cjschaef mentioned this pull request Aug 7, 2024

Extend VPC (x86_64) Infrastructure reconciliation support #1896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kubebuilder validation for SecurityGroup resources #1710

Fix kubebuilder validation for SecurityGroup resources #1710

cjschaef commented Apr 3, 2024 •

edited

Loading

k8s-ci-robot commented Apr 3, 2024

netlify bot commented Apr 3, 2024 •

edited

Loading

Karthik-K-N commented Apr 4, 2024

Karthik-K-N Apr 4, 2024

cjschaef Apr 4, 2024

Prajyot-Parab Apr 4, 2024

Karthik-K-N Apr 5, 2024

Karthik-K-N left a comment

Karthik-K-N Apr 5, 2024

cjschaef Apr 5, 2024

cjschaef Apr 5, 2024

Karthik-K-N Apr 8, 2024

Karthik-K-N left a comment

Prajyot-Parab commented Apr 8, 2024

cjschaef commented Apr 8, 2024

Prajyot-Parab commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

Fix kubebuilder validation for SecurityGroup resources #1710

Fix kubebuilder validation for SecurityGroup resources #1710

Conversation

cjschaef commented Apr 3, 2024 • edited Loading

k8s-ci-robot commented Apr 3, 2024

netlify bot commented Apr 3, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Karthik-K-N commented Apr 4, 2024

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

Karthik-K-N left a 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

Karthik-K-N left a comment

Choose a reason for hiding this comment

Prajyot-Parab commented Apr 8, 2024

cjschaef commented Apr 8, 2024

Prajyot-Parab commented Apr 10, 2024

k8s-ci-robot commented Apr 10, 2024

cjschaef commented Apr 3, 2024 •

edited

Loading

netlify bot commented Apr 3, 2024 •

edited

Loading