Ms/validate sec groups #4269
Ms/validate sec groups #4269
Conversation
| } | ||
|
|
||
| func (v *Validator) validateRules(rules []korifiv1alpha1.SecurityGroupRule) error { | ||
| if len(rules) == 0 { |
There was a problem hiding this comment.
I don't think we need that check - ff the rules slice is empty, the code would not enter the for loop and would eventually return nil.
| return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a CFSecurityGroup but got a %T", obj)) | ||
| } | ||
|
|
||
| if err := validateName(securityGroup.Spec.DisplayName); err != nil { |
There was a problem hiding this comment.
Besides duplicate validation, it sounds like security group validation is identical on create and update.
How about introduce a validateSecurityGroup(CFSecurityGroup) method that invokes validateName and validateRules? We would then reduce code repetition
|
|
||
| func validateRulePorts(ports, protocol string) error { | ||
| if !slices.Contains([]string{"tcp", "udp", "all"}, protocol) { | ||
| if !slices.Contains([]string{"tcp", "udp", "icmp", "icmpv6", "all"}, protocol) { |
There was a problem hiding this comment.
I wonder whether we could offload this validation to the bube builder annotation // +kubebuilder:validation:Enum, here is an example
| return nil | ||
| } | ||
|
|
||
| if icmpType < -1 || icmpType > 255 { |
There was a problem hiding this comment.
kubebuilder validation annotations (https://book.kubebuilder.io/reference/markers/crd-validation) can configure minimum and maximum accepted values for crd fields. I wonder whether we could implement the range check by leveraging them instead of doing it ourselves
| } | ||
|
|
||
| func validateRuleICMP(icmpType, icmpCode int, protocol string) error { | ||
| if protocol != korifiv1alpha1.ProtocolICMP && protocol != korifiv1alpha1.ProtocolICMPv6 { |
There was a problem hiding this comment.
I would suggest simplifying this if-else slalom. How about moving the range checks above, and then
if protocol == korifiv1alpha1.ProtocolICMP {
return nil
}
if protocol == korifiv1alhpa1.ProtocolICMPv6 {
return nil
}
if icmpType != 0 {
return errors.New("type allowed for ICMP and ICMPv6 only")
}
if icmpCode != 0 {
return errors.New("code allowed for ICMP and ICMPv6 only")
}
| func validateRuleICMP(icmpType, icmpCode int, protocol string) error { | ||
| if protocol != korifiv1alpha1.ProtocolICMP && protocol != korifiv1alpha1.ProtocolICMPv6 { | ||
| if icmpType != 0 || icmpCode != 0 { | ||
| return fmt.Errorf("type and code are only allowed for ICMP and ICMPv6 protocols") |
There was a problem hiding this comment.
There is no formatting here, so errors.New is probably the better choice here. Linters would probably give you hard time on this as well.
| if ports == "" { | ||
| return fmt.Errorf("ports are required for protocols of type TCP and UDP, %s", InvalidPortsErrorMessage) | ||
| } | ||
| if protocol == korifiv1alpha1.ProtocolTCP || protocol == korifiv1alpha1.ProtocolUDP { |
There was a problem hiding this comment.
what about the other protocols? aren't there restrictions on ports?
| }, | ||
| Spec: korifiv1alpha1.CFSecurityGroupSpec{ | ||
| DisplayName: uuid.NewString(), | ||
| DisplayName: "RuleName", |
There was a problem hiding this comment.
Isn't the random guid a valid display name?
| fi | ||
|
|
||
| helm repo add twuni https://twuni.github.io/docker-registry.helm | ||
| helm repo add twuni https://twuni.github.io/docker-registry.helm --force-update |
There was a problem hiding this comment.
Is this related to the validation?
2 participants
Is there a related GitHub Issue?
#4103
What is this change about?
Updating and enhancing the existing Validator for Security Groups
Does this PR introduce a breaking change?
It should not as Security Groups are not supported, yet.
Acceptance Steps
create sec-group.json:
E.g.:
then create them:
cf curl -X POST "/v3/security_groups" [email protected]
Tag your pair, your PM, and/or team
@danail-branekov, @georgethebeatle