Update vpc-go-sdk module and introduce the new SecurityGroupRule types#2603
Update vpc-go-sdk module and introduce the new SecurityGroupRule types#2603Pacho20 wants to merge 6 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @Pacho20! |
|
Hi @Pacho20. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Pacho20 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @Karthik-K-N |
|
/ok-to-test |
b072c8b to
4ae0b24
Compare
The latest vpc-go-sdk release introduced a breaking change to SecurityGroupRuleProtocol: the value 'all' was removed and replaced by 'icmp_tcp_udp'. This commit updates the security group rule creation and validation to use 'icmp_tcp_udp' instead of 'all'.
The latest vpc-go-sdk introduces two new security group rule protocol types: any and individual. This commit adds full compatibility for these new protocols in the VPC implementation.
The latest vpc-go-sdk introduces two new security group rule protocol types: 'any' and 'individual'. This commit adds full compatibility for these new protocols in the PowerVS implementation.
Karthik-K-N
left a comment
There was a problem hiding this comment.
Overall LGTM, Couple of changes required in error statements or logs.
Thanks for working on it
| ogRule := ogRuleIntf.(*vpcv1.SecurityGroupRuleProtocolAny) | ||
| ruleID = ogRule.ID | ||
|
|
||
| if *ogRule.Direction == string(direction) && *ogRule.Protocol == protocol { |
There was a problem hiding this comment.
Is it safe to directly deref the pointer? is it non nil always?
There was a problem hiding this comment.
Yes, it should be safe. If you check the API docs, these values are marked as always included. Without the direction and protocol fields, the rule would not make sense at all. Besides that the same pattern is used for all the other SGR types.
https://cloud.ibm.com/apidocs/vpc/latest#get-security-group
cloud/scope/vpc/cluster_v2.go
Outdated
| IBMVPCCluster *infrav1.IBMVPCCluster | ||
| ServiceEndpoint []endpoints.ServiceEndpoint | ||
|
|
||
| individualSgrRegexp *regexp.Regexp |
There was a problem hiding this comment.
Is it required to be part of ClusterScope, as its a constant and can't we keep it as variable?
There was a problem hiding this comment.
You are right it can be a variable. I will move it from ClusterScope.
cloud/scope/vpc/cluster_v2.go
Outdated
| if exists, err := s.checkSecurityGroupRulePrototypeRemote(ctx, securityGroupRuleRemote, existingRule.Remote); err != nil { | ||
| return false, fmt.Errorf("error failed checking security group rule all remote: %w", err) | ||
| } else if exists { | ||
| log.V(3).Info("security group rule all protocols match") |
There was a problem hiding this comment.
security group rule all or security group rule any?
cloud/scope/vpc/cluster_v2.go
Outdated
| } | ||
|
|
||
| if exists, err := s.checkSecurityGroupRulePrototypeRemote(ctx, securityGroupRuleRemote, existingRule.Remote); err != nil { | ||
| return false, fmt.Errorf("error failed checking security group rule all remote: %w", err) |
|
PR needs rebase. DetailsInstructions 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. |
|
Most of the things LGTM, Have you tested this change by creating a cluster? |
What this PR does / why we need it:
This PR updates the vpc-go-sdk version
The latest release of vpc-go-sdk contains some breaking changes for SecurityGroupRuleProtocol, Its changed from all to icmp_tcp_udp and we are no more able to use 'all' protocol. Besides that it introduced two new protocol types Any and Individual.
This PR changes the protocol usage from 'all' to 'icmp_tcp_udp' and add support for the new protocol types 'any' and 'individual'.
The release notes: https://cloud.ibm.com/docs/vpc?topic=vpc-api-change-log#9-december-2025
Which issue(s) this PR fixes:
Fixes #2579
Special notes for your reviewer:
/area provider/ibmcloud
Release note: