Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(securityGroups): Improve robustness of security group reconcilers #373

Closed
wants to merge 1 commit into from
Closed

feat(securityGroups): Improve robustness of security group reconcilers #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/v1beta1/osccluster_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func ValidateOscClusterSpec(spec OscClusterSpec) field.ErrorList {
// ValidateCidr check that the cidr string is a valide CIDR
func ValidateCidr(cidr string) (string, error) {
if !strings.Contains(cidr, "/") {
return cidr, errors.New("Invalid Not A CIDR")
return cidr, errors.New("invalid Not A CIDR")
}
_, _, err := net.ParseCIDR(cidr)
if err != nil {
Expand Down
9 changes: 6 additions & 3 deletions api/v1beta1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ type OscNetwork struct {
// The subregion name
// + optional
SubregionName string `json:"subregionName,omitempty"`
// Add SecurityGroup Rule after the cluster is created
// + optional
ExtraSecurityGroupRule bool `json:"extraSecurityGroupRule,omitempty"`
}

type OscLoadBalancer struct {
Expand Down Expand Up @@ -219,6 +216,9 @@ type OscSecurityGroup struct {
// The description of the security group
// +optional
Description string `json:"description,omitempty"`
// Should the default allow all outbound rule be deleted
// +optional
DeleteDefaultOutboundRule bool `json:"deleteDefaultOutboundRule,omitempty"`
// The Security Group Rules configuration
// +optional
SecurityGroupRules []OscSecurityGroupRule `json:"securityGroupRules,omitempty"`
Expand Down Expand Up @@ -278,6 +278,9 @@ type OscSecurityGroupRule struct {
// The ip range of the security group rule
// +optional
IpRange string `json:"ipRange,omitempty"`
// The name of the security group to use as target
// +optional
TargetSecurityGroupName string `json:"targetSecurityGroupName,omitempty"`
// The beginning of the port range
// +optional
FromPortRange int32 `json:"fromPortRange,omitempty"`
Expand Down
10 changes: 0 additions & 10 deletions cloud/scope/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,16 +180,6 @@ func (s *ClusterScope) GetNet() *infrastructurev1beta1.OscNet {
return &s.OscCluster.Spec.Network.Net
}

// GetExtraSecurityGroupRule return the extraSecurityGroupRule
func (s *ClusterScope) GetExtraSecurityGroupRule() bool {
return s.OscCluster.Spec.Network.ExtraSecurityGroupRule
}

// SetExtraSecurityGroupRule set the extraSecurityGroupRule
func (s *ClusterScope) SetExtraSecurityGroupRule(extraSecurityGroupRule bool) {
s.OscCluster.Spec.Network.ExtraSecurityGroupRule = extraSecurityGroupRule
}

// GetPublicIpNameAfterBastion return publicIpNameAfterBastion
func (s *ClusterScope) GetPublicIpNameAfterBastion() bool {
return s.OscCluster.Spec.Network.Bastion.PublicIpNameAfterBastion
Expand Down
5 changes: 2 additions & 3 deletions cloud/services/security/mock_security/securitygroup_mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 30 additions & 14 deletions cloud/services/security/securitygroup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package security

import (
"fmt"
"net/http"

"errors"

Expand All @@ -36,7 +35,7 @@ type OscSecurityGroupInterface interface {
CreateSecurityGroup(netId string, clusterName string, securityGroupName string, securityGroupDescription string, securityGroupTag string) (*osc.SecurityGroup, error)
CreateSecurityGroupRule(securityGroupId string, flow string, ipProtocol string, ipRange string, securityGroupMemberId string, fromPortRange int32, toPortRange int32) (*osc.SecurityGroup, error)
DeleteSecurityGroupRule(securityGroupId string, flow string, ipProtocol string, ipRange string, securityGroupMemberId string, fromPortRange int32, toPortRange int32) error
DeleteSecurityGroup(securityGroupId string) (error, *http.Response)
DeleteSecurityGroup(securityGroupId string) error
GetSecurityGroup(securityGroupId string) (*osc.SecurityGroup, error)
GetSecurityGroupFromSecurityGroupRule(securityGroupId string, Flow string, IpProtocols string, IpRanges string, securityGroupMemberId string, FromPortRanges int32, ToPortRanges int32) (*osc.SecurityGroup, error)
GetSecurityGroupIdsFromNetIds(netId string) ([]string, error)
Expand Down Expand Up @@ -218,7 +217,7 @@ func (s *Service) DeleteSecurityGroupRule(securityGroupId string, flow string, i
oscApiClient := s.scope.GetApi()
oscAuthClient := s.scope.GetAuth()

deleteSecurityGroupCallBack := func() (bool, error) {
deleteSecurityGroupRuleCallBack := func() (bool, error) {
var httpRes *_nethttp.Response
var err error

Expand All @@ -239,26 +238,45 @@ func (s *Service) DeleteSecurityGroupRule(securityGroupId string, flow string, i
return true, err
}
backoff := reconciler.EnvBackoff()
waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupCallBack)
waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupRuleCallBack)
if waitErr != nil {
return waitErr
}
return nil
}

// DeleteSecurityGroup delete the securitygroup associated with the net
func (s *Service) DeleteSecurityGroup(securityGroupId string) (error, *http.Response) {
func (s *Service) DeleteSecurityGroup(securityGroupId string) error {
deleteSecurityGroupRequest := osc.DeleteSecurityGroupRequest{SecurityGroupId: &securityGroupId}
oscApiClient := s.scope.GetApi()
oscAuthClient := s.scope.GetAuth()
_, httpRes, err := oscApiClient.SecurityGroupApi.DeleteSecurityGroup(oscAuthClient).DeleteSecurityGroupRequest(deleteSecurityGroupRequest).Execute()
if err != nil {
if httpRes != nil {
fmt.Printf("Error with http result %s", httpRes.Status)
return err, httpRes

deleteSecurityGroupCallBack := func() (bool, error) {
var httpRes *_nethttp.Response
var err error

_, httpRes, err = oscApiClient.SecurityGroupApi.DeleteSecurityGroup(oscAuthClient).DeleteSecurityGroupRequest(deleteSecurityGroupRequest).Execute()
if err != nil {
if httpRes != nil {
return false, fmt.Errorf("error %w httpRes %s", err, httpRes.Status)
}
requestStr := fmt.Sprintf("%v", deleteSecurityGroupRequest)
if reconciler.KeepRetryWithError(
requestStr,
httpRes.StatusCode,
reconciler.ThrottlingErrors) {
return false, nil
}
return false, err
}
return true, err
}
backoff := reconciler.EnvBackoff()
waitErr := wait.ExponentialBackoff(backoff, deleteSecurityGroupCallBack)
if waitErr != nil {
return waitErr
}
return nil, httpRes
return nil
}

// GetSecurityGroup retrieve security group object from the security group id
Expand Down Expand Up @@ -314,9 +332,7 @@ func (s *Service) GetSecurityGroupFromSecurityGroupRule(securityGroupId string,
fromPortRanges = -1
toPortRanges = -1
}
if fromPortRanges == 53 && toPortRanges == 53 {
ipProtocols = "udp"
}

switch {
case flow == "Inbound":
readSecurityGroupRuleRequest = osc.ReadSecurityGroupsRequest{
Expand Down
13 changes: 9 additions & 4 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.15.0
controller-gen.kubebuilder.io/version: v0.16.2
name: oscclusters.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down Expand Up @@ -122,9 +122,6 @@ spec:
items:
type: string
type: array
extraSecurityGroupRule:
description: Add SecurityGroup Rule after the cluster is created
type: boolean
image:
description: The image configuration
properties:
Expand Down Expand Up @@ -329,6 +326,10 @@ spec:
securityGroups:
items:
properties:
deleteDefaultOutboundRule:
description: Should the default allow all outbound rule
be deleted
type: boolean
description:
description: The description of the security group
type: string
Expand Down Expand Up @@ -364,6 +365,10 @@ spec:
resourceId:
description: The security group rule id
type: string
targetSecurityGroupName:
description: The name of the security group to use
as target
type: string
toPortRange:
description: The end of the port range
format: int32
Expand Down
18 changes: 9 additions & 9 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscclustertemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.15.0
controller-gen.kubebuilder.io/version: v0.16.2
name: oscclustertemplates.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down Expand Up @@ -49,26 +49,22 @@ spec:
ObjectMeta is metadata that all persisted resources must have, which includes all objects
users must create. This is a copy of customizable fields from metav1.ObjectMeta.


ObjectMeta is embedded in `Machine.Spec`, `MachineDeployment.Template` and `MachineSet.Template`,
which are not top-level Kubernetes objects. Given that metav1.ObjectMeta has lots of special cases
and read-only fields which end up in the generated CRD validation, having it as a subset simplifies
the API and some issues that can impact user experience.


During the [upgrade to controller-tools@v2](https://github.com/kubernetes-sigs/cluster-api/pull/1054)
for v1alpha2, we noticed a failure would occur running Cluster API test suite against the new CRDs,
specifically `spec.metadata.creationTimestamp in body must be of type string: "null"`.
The investigation showed that `controller-tools@v2` behaves differently than its previous version
when handling types from [metav1](k8s.io/apimachinery/pkg/apis/meta/v1) package.


In more details, we found that embedded (non-top level) types that embedded `metav1.ObjectMeta`
had validation properties, including for `creationTimestamp` (metav1.Time).
The `metav1.Time` type specifies a custom json marshaller that, when IsZero() is true, returns `null`
which breaks validation because the field isn't marked as nullable.


In future versions, controller-tools@v2 might allow overriding the type and validation for embedded
types. When that happens, this hack should be revisited.
properties:
Expand Down Expand Up @@ -178,10 +174,6 @@ spec:
items:
type: string
type: array
extraSecurityGroupRule:
description: Add SecurityGroup Rule after the cluster
is created
type: boolean
image:
description: The image configuration
properties:
Expand Down Expand Up @@ -399,6 +391,10 @@ spec:
securityGroups:
items:
properties:
deleteDefaultOutboundRule:
description: Should the default allow all outbound
rule be deleted
type: boolean
description:
description: The description of the security group
type: string
Expand Down Expand Up @@ -436,6 +432,10 @@ spec:
resourceId:
description: The security group rule id
type: string
targetSecurityGroupName:
description: The name of the security group
to use as target
type: string
toPortRange:
description: The end of the port range
format: int32
Expand Down
2 changes: 1 addition & 1 deletion config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachines.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.15.0
controller-gen.kubebuilder.io/version: v0.16.2
name: oscmachines.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down
6 changes: 1 addition & 5 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_oscmachinetemplates.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.15.0
controller-gen.kubebuilder.io/version: v0.16.2
name: oscmachinetemplates.infrastructure.cluster.x-k8s.io
spec:
group: infrastructure.cluster.x-k8s.io
Expand Down Expand Up @@ -50,26 +50,22 @@ spec:
ObjectMeta is metadata that all persisted resources must have, which includes all objects
users must create. This is a copy of customizable fields from metav1.ObjectMeta.
ObjectMeta is embedded in `Machine.Spec`, `MachineDeployment.Template` and `MachineSet.Template`,
which are not top-level Kubernetes objects. Given that metav1.ObjectMeta has lots of special cases
and read-only fields which end up in the generated CRD validation, having it as a subset simplifies
the API and some issues that can impact user experience.
During the [upgrade to controller-tools@v2](https://github.com/kubernetes-sigs/cluster-api/pull/1054)
for v1alpha2, we noticed a failure would occur running Cluster API test suite against the new CRDs,
specifically `spec.metadata.creationTimestamp in body must be of type string: "null"`.
The investigation showed that `controller-tools@v2` behaves differently than its previous version
when handling types from [metav1](k8s.io/apimachinery/pkg/apis/meta/v1) package.
In more details, we found that embedded (non-top level) types that embedded `metav1.ObjectMeta`
had validation properties, including for `creationTimestamp` (metav1.Time).
The `metav1.Time` type specifies a custom json marshaller that, when IsZero() is true, returns `null`
which breaks validation because the field isn't marked as nullable.
In future versions, controller-tools@v2 might allow overriding the type and validation for embedded
types. When that happens, this hack should be revisited.
properties:
Expand Down
Loading