VPC: Add Security Group resources and constants/enums #1669
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
cncf-cla: yes
|
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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cjschaef
force-pushed
the
vpc_security_group_apis
branch
2 times, most recently
from
2ec6b42
to
c820d22
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
api/v1beta2/types.go
Outdated
| // SecurityGroupRuleProtocolAll defines the Rule is for all network protocols. | ||
| SecurityGroupRuleProtocolAll SecurityGroupRuleProtocol = vpcv1.NetworkACLRuleProtocolAllConst | ||
| // SecurityGroupRuleProtocolICMP defiens the Rule is for ICMP network protocol. | ||
| SecurityGroupRuleProtocolICMP SecurityGroupRuleProtocol = vpcv1.NetworkACLRuleProtocolIcmpConst |
There was a problem hiding this comment.
| SecurityGroupRuleProtocolICMP SecurityGroupRuleProtocol = vpcv1.NetworkACLRuleProtocolIcmpConst | |
| SecurityGroupRuleProtocolICMP SecurityGroupRuleProtocol = vpcv1.NetworkACLRuleProtocolICMPConst |
lets maintain uniformity
There was a problem hiding this comment.
That is the name of the constant in the VPC code, I cannot change that. But I would like to make an enum of these constants, for validation's sake.
// Constants associated with the NetworkACLRule.Protocol property.
// The protocol to enforce.
const (
NetworkACLRuleProtocolAllConst = "all"
NetworkACLRuleProtocolIcmpConst = "icmp"
NetworkACLRuleProtocolTCPConst = "tcp"
NetworkACLRuleProtocolUDPConst = "udp"
)
There was a problem hiding this comment.
If it is referred as Icmp in that package, lets do change in our code to follow same standard wherever possible.
There was a problem hiding this comment.
I think the standard is ICMP actually, which is also what the docs follow too. I think the SDK itself is inaccurate.
There was a problem hiding this comment.
I changed this one reference to Icmp to match the SDK constant name, have left remaining references as ICMP
|
@Karthik-K-N @Amulyam24 ptal |
cjschaef
force-pushed
the
vpc_security_group_apis
branch
from
c820d22
to
b31f53a
Compare
| ID *string `json:"id,omitempty"` | ||
|
|
||
| // name of the Security Group. | ||
| Name *string `json:"name,omitempty"` |
There was a problem hiding this comment.
Dont we need +optional tags here?
There was a problem hiding this comment.
Yes, let me add those tags quick.
Add resource definitions for managing VPC Security Groups and Rules, along with a set of enumerations to simplify validation and options.
cjschaef
force-pushed
the
vpc_security_group_apis
branch
from
b31f53a
to
4a659a3
Compare
k8s-ci-robot
added
cncf-cla: no
|
/easycla |
k8s-ci-robot
removed
the
cncf-cla: no
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Add resource definitions for managing VPC Security Groups and Rules, along with a set of enumerations to simplify validation and options.
What this PR does / why we need it: Adds support to define SecurityGroups in VPC for VPCCluster 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 # N/A
Special notes for your reviewer:
/area provider/ibmcloud
Release note: