Skip to content

Commit

Permalink
Make sure securitygroup resourceMap is populated before reconcile
Browse files Browse the repository at this point in the history 
Signed-off-by: hanenMizouni <[email protected]>
  • Loading branch information
@outscale-hmi
outscale-hmi committed Dec 18, 2024
1 parent 616aab4 commit d289669
Show file tree
Hide file tree
Showing 5 changed files with 185 additions and 101 deletions.
5 changes: 5 additions & 0 deletions cloud/scope/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@ func (s *ClusterScope) GetSecurityGroupsRef() *infrastructurev1beta1.OscResource
return &s.OscCluster.Status.Network.SecurityGroupsRef
}

// SetSecurityGroupsRef updates the SecurityGroupsRef in the cluster scope
func (s *ClusterScope) SetSecurityGroupsRef(securityGroupsRef infrastructurev1beta1.OscResourceReference) {
s.OscCluster.Status.Network.SecurityGroupsRef = securityGroupsRef
}

// GetRouteRef get the status of route (a Map with tag name with cluster uid associate with resource response id)
func (s *ClusterScope) GetRouteRef() *infrastructurev1beta1.OscResourceReference {
return &s.OscCluster.Status.Network.RouteRef
Expand Down
117 changes: 57 additions & 60 deletions controllers/osccluster_securitygroup_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,9 @@ func deleteSecurityGroup(ctx context.Context, clusterScope *scope.ClusterScope,
// reconcileSecurityGroup reconcile the securityGroup of the cluster.
func reconcileSecurityGroup(ctx context.Context, clusterScope *scope.ClusterScope, securityGroupSvc security.OscSecurityGroupInterface, tagSvc tag.OscTagInterface) (reconcile.Result, error) {
clusterScope.V(4).Info("Reconciling security groups for OscCluster")
securityGroupsSpec := clusterScope.GetSecurityGroups()

// Retrieve security group specifications
securityGroupsSpec := clusterScope.GetSecurityGroups()
netSpec := clusterScope.GetNet()
netSpec.SetDefaultValue()
netName := netSpec.Name + "-" + clusterScope.GetUID()
Expand All @@ -193,71 +194,77 @@ func reconcileSecurityGroup(ctx context.Context, clusterScope *scope.ClusterScop
networkSpec := clusterScope.GetNetwork()
clusterName := networkSpec.ClusterName + "-" + clusterScope.GetUID()

clusterScope.V(4).Info("Get list of all desired securitygroup in net", "netId", netId)
// Fetch existing security group IDs associated with the NetId
clusterScope.V(4).Info("Get list of all desired security groups in net", "netId", netId)
securityGroupIds, err := securityGroupSvc.GetSecurityGroupIdsFromNetIds(netId)
clusterScope.V(4).Info("Get securityGroup Ids", "securityGroups", securityGroupIds)
if err != nil {
return reconcile.Result{}, err
}
clusterScope.V(4).Info("Fetched security group IDs", "securityGroupIds", securityGroupIds)

// Initialize ResourceMap if necessary
securityGroupsRef := clusterScope.GetSecurityGroupsRef()
clusterScope.V(4).Info("Number of securityGroup", "securityGroupLength", len(securityGroupsSpec))
if len(securityGroupsRef.ResourceMap) == 0 {
securityGroupsRef.ResourceMap = make(map[string]string)
}

// Iterate through desired security groups
for _, securityGroupSpec := range securityGroupsSpec {
securityGroupName := securityGroupSpec.Name + "-" + clusterScope.GetUID()
clusterScope.V(2).Info("Check if the desired securityGroup exists in net", "securityGroupName", securityGroupName)
clusterScope.V(2).Info("Checking if desired security group exists", "securityGroupName", securityGroupName)
securityGroupDescription := securityGroupSpec.Description
deleteDefaultOutboundRule := securityGroupSpec.DeleteDefaultOutboundRule

// Fetch existing tag for the security group
tagKey := "Name"
tagValue := securityGroupName
tag, err := tagSvc.ReadTag(tagKey, tagValue)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w cannot get tag for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
securityGroupTag := securityGroupSpec.Tag
if len(securityGroupsRef.ResourceMap) == 0 {
securityGroupsRef.ResourceMap = make(map[string]string)
return reconcile.Result{}, fmt.Errorf("cannot get tag for OscCluster %s/%s: %w", clusterScope.GetNamespace(), clusterScope.GetName(), err)
}

// Populate ResourceMap if the security group is already known
if securityGroupSpec.ResourceId != "" {
securityGroupsRef.ResourceMap[securityGroupName] = securityGroupSpec.ResourceId
}
_, resourceMapExist := securityGroupsRef.ResourceMap[securityGroupName]
if resourceMapExist {
securityGroupSpec.ResourceId = securityGroupsRef.ResourceMap[securityGroupName]
if securityGroupId, exists := securityGroupsRef.ResourceMap[securityGroupName]; exists {
securityGroupSpec.ResourceId = securityGroupId
}
var securityGroup *osc.SecurityGroup
securityGroupId := securityGroupsRef.ResourceMap[securityGroupName]

// Check if the security group needs to be created
securityGroupId := securityGroupsRef.ResourceMap[securityGroupName]
if !Contains(securityGroupIds, securityGroupId) && tag == nil {
clusterScope.V(2).Info("Create the desired securitygroup", "securityGroupName", securityGroupName)
if securityGroupTag == "OscK8sMainSG" {
clusterScope.V(2).Info("Creating the desired security group", "securityGroupName", securityGroupName)
var securityGroup *osc.SecurityGroup
if securityGroupSpec.Tag == "OscK8sMainSG" {
securityGroup, err = securityGroupSvc.CreateSecurityGroup(netId, clusterName, securityGroupName, securityGroupDescription, "OscK8sMainSG")
} else {
securityGroup, err = securityGroupSvc.CreateSecurityGroup(netId, clusterName, securityGroupName, securityGroupDescription, "")
}
clusterScope.V(4).Info("Get securityGroup", "securityGroup", securityGroup)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w cannot create securityGroup for Osccluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("cannot create security group for OscCluster %s/%s: %w", clusterScope.GetNamespace(), clusterScope.GetName(), err)
}

// Update ResourceMap and security group spec
securityGroupsRef.ResourceMap[securityGroupName] = *securityGroup.SecurityGroupId
securityGroupSpec.ResourceId = *securityGroup.SecurityGroupId

// Delete default outbound rule if specified
if deleteDefaultOutboundRule {
clusterScope.V(2).Info("Delete default outbound rule for sg", "securityGroupName", securityGroupSpec.Name)
clusterScope.V(2).Info("Deleting default outbound rule", "securityGroupName", securityGroupName)
err = securityGroupSvc.DeleteSecurityGroupRule(*securityGroup.SecurityGroupId, "Outbound", "-1", "0.0.0.0/0", "", 0, 0)
if err != nil {
return reconcile.Result{}, fmt.Errorf("%w Cannot delete default Outbound rule for sg %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("cannot delete default outbound rule for security group %s/%s: %w", clusterScope.GetNamespace(), clusterScope.GetName(), err)
}
}
}
}

// Cleanup any unexpected security groups
for securityGroupName, securityGroupId := range securityGroupsRef.ResourceMap {
if !Contains(securityGroupIds, securityGroupId) {
clusterScope.V(4).Info("Deleting securityGroup and associated securityGroupRules", "securityGroupName", securityGroupName)

clusterScope.V(4).Info("Deleting security group and associated rules", "securityGroupName", securityGroupName)
securityGroupRulesSpec := clusterScope.GetSecurityGroupRule(securityGroupName)
clusterScope.V(4).Info("Deleting securityGroupRules", "securityGroupName", securityGroupName)
for _, securityGroupRuleSpec := range *securityGroupRulesSpec {
reconcileDeleteSecurityGroupsRule, err := reconcileDeleteSecurityGroupsRule(ctx, clusterScope, securityGroupRuleSpec, securityGroupName, securityGroupSvc)
if err != nil {
Expand All @@ -270,19 +277,25 @@ func reconcileSecurityGroup(ctx context.Context, clusterScope *scope.ClusterScop
}
}
}

// Update the cluster scope with the updated ResourceMap
clusterScope.SetSecurityGroupsRef(*securityGroupsRef)

return reconcile.Result{}, nil
}

// reconcileSecurityGroupRule reconcile the securityGroupRules of the cluster.
func reconcileSecurityGroupRule(ctx context.Context, clusterScope *scope.ClusterScope, securityGroupSvc security.OscSecurityGroupInterface, tagSvc tag.OscTagInterface) (reconcile.Result, error) {
clusterScope.V(4).Info("Reconciling security group rules")
securityGroupsSpec := clusterScope.GetSecurityGroups()

// Fetch security groups and their references
securityGroupsSpec := clusterScope.GetSecurityGroups()
securityGroupsRef := clusterScope.GetSecurityGroupsRef()
if len(securityGroupsRef.ResourceMap) == 0 {
return reconcile.Result{}, fmt.Errorf("securityGroupsRef.ResourceMap is empty, security groups should be reconciled first")
}

// Fetch or initialize security group rule references
securityGroupRuleRef := clusterScope.GetSecurityGroupRuleRef()
if len(securityGroupRuleRef.ResourceMap) == 0 {
securityGroupRuleRef.ResourceMap = make(map[string]string)
Expand All @@ -291,69 +304,53 @@ func reconcileSecurityGroupRule(ctx context.Context, clusterScope *scope.Cluster
for _, securityGroupSpec := range securityGroupsSpec {
securityGroupName := securityGroupSpec.Name + "-" + clusterScope.GetUID()
clusterScope.V(2).Info("Reconciling security group rules for securityGroup", "securityGroupName", securityGroupName)

securityGroupRulesSpec := clusterScope.GetSecurityGroupRule(securityGroupSpec.Name)
clusterScope.V(4).Info("Number of securityGroupRules", "securityGroupRuleLength", len(*securityGroupRulesSpec))
var securityGroupRuleNames []string

for _, securityGroupRuleSpec := range *securityGroupRulesSpec {
securityGroupRuleName := securityGroupRuleSpec.Name + "-" + clusterScope.GetUID()
securityGroupRuleNames = append(securityGroupRuleNames, securityGroupRuleName)
clusterScope.V(4).Info("Reconciling securityGroupRule for the desired securityGroup", "securityGroupName", securityGroupName, "securityGroupRuleName", securityGroupRuleName)
clusterScope.V(4).Info("Reconciling securityGroupRule", "securityGroupRuleName", securityGroupRuleName)

// Define rule parameters
Flow := securityGroupRuleSpec.Flow
IpProtocol := securityGroupRuleSpec.IpProtocol
IpRange := securityGroupRuleSpec.IpRange
FromPortRange := securityGroupRuleSpec.FromPortRange
ToPortRange := securityGroupRuleSpec.ToPortRange
associateSecurityGroupId := securityGroupsRef.ResourceMap[securityGroupName]

// Handle target security group if specified
targetSecurityGroupId := ""
if securityGroupRuleSpec.TargetSecurityGroupName != "" {
targetSecurityGroupName := securityGroupRuleSpec.TargetSecurityGroupName + "-" + clusterScope.GetUID()
targetSecurityGroupId = securityGroupsRef.ResourceMap[targetSecurityGroupName]
clusterScope.V(4).Info("Get targetSecurityGroupId", "securityGroup", targetSecurityGroupId)
clusterScope.V(4).Info("Retrieved targetSecurityGroupId", "targetSecurityGroupId", targetSecurityGroupId)

if targetSecurityGroupId == "" {
return reconcile.Result{RequeueAfter: 30 * time.Second}, fmt.Errorf("the target securityGroup %s does not exist (yet) for OscCluster %s/%s", targetSecurityGroupName, clusterScope.GetNamespace(), clusterScope.GetName())
clusterScope.V(2).Info("Target security group not found, requeuing", "targetSecurityGroupName", targetSecurityGroupName)
return reconcile.Result{RequeueAfter: 30 * time.Second}, fmt.Errorf("the target securityGroup %s does not exist (yet)", targetSecurityGroupName)
}
}

// The GetSecurityGroupFromSecurityGroupRule function does not work for Rules containing a targetSecurityGroupId, for now we just try to create and ignore if it already exists (error 409)
// clusterScope.V(4).Info("Check if the desired securityGroupRule exist", "securityGroupRuleName", securityGroupRuleName)
// securityGroupFromSecurityGroupRule, err := securityGroupSvc.GetSecurityGroupFromSecurityGroupRule(associateSecurityGroupId, Flow, IpProtocol, IpRange, targetSecurityGroupId, FromPortRange, ToPortRange)
// if err != nil {
// return reconcile.Result{}, err
// }
// if securityGroupFromSecurityGroupRule == nil {
// clusterScope.V(4).Info("Create the desired securityGroupRule", "securityGroupRuleName", securityGroupRuleName)
// securityGroupFromSecurityGroupRule, err = securityGroupSvc.CreateSecurityGroupRule(associateSecurityGroupId, Flow, IpProtocol, IpRange, targetSecurityGroupId, FromPortRange, ToPortRange)
// if err != nil {
// return reconcile.Result{}, fmt.Errorf("%w cannot create securityGroupRule for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
// }
// }
clusterScope.V(4).Info("Create the desired securityGroupRule", "securityGroupRuleName", securityGroupRuleName)
securityGroupFromSecurityGroupRule, err := securityGroupSvc.CreateSecurityGroupRule(associateSecurityGroupId, Flow, IpProtocol, IpRange, targetSecurityGroupId, FromPortRange, ToPortRange)
// Attempt to create the rule
clusterScope.V(4).Info("Creating securityGroupRule", "securityGroupRuleName", securityGroupRuleName)
_, err := securityGroupSvc.CreateSecurityGroupRule(associateSecurityGroupId, Flow, IpProtocol, IpRange, targetSecurityGroupId, FromPortRange, ToPortRange)
if err != nil {
if strings.Contains(err.Error(), "409 Conflict") {
clusterScope.V(4).Info("Rule already exists, ignoring 409 Conflict", "securityGroupRuleName", securityGroupRuleName)
clusterScope.V(4).Info("Rule already exists, skipping creation", "securityGroupRuleName", securityGroupRuleName)
continue
}
return reconcile.Result{}, fmt.Errorf("%w cannot create securityGroupRule for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
return reconcile.Result{}, fmt.Errorf("%w: cannot create securityGroupRule for OscCluster %s/%s", err, clusterScope.GetNamespace(), clusterScope.GetName())
}
securityGroupRuleRef.ResourceMap[securityGroupRuleName] = securityGroupFromSecurityGroupRule.GetSecurityGroupId()
}

// for securityGroupRuleName, _ := range securityGroupRuleRef.ResourceMap {
// if !Contains(securityGroupRuleNames, securityGroupRuleName) {
// clusterScope.V(4).Info("Deleting securityGroupRule", "securityGroupRuleName", securityGroupRuleName)
// clusterScope.V(2).Info("Deleting individual sg rules after they have been deleted from the spec is not supported yet")
// We cannot delete securityGroupRules here while we should as we require the securityGroupRuleSpec, but that is already deleted from the osc cluster spec
// reconcileDeleteSecurityGroupsRule, err := reconcileDeleteSecurityGroupsRule(ctx, clusterScope, securityGroupRuleSpec, securityGroupName, securityGroupSvc)
// if err != nil {
// return reconcileDeleteSecurityGroupsRule, err
// }
// }
// }
// Update rule reference
securityGroupRuleRef.ResourceMap[securityGroupRuleName] = associateSecurityGroupId
}
}

// Note: Deletion of orphaned rules from reference map is currently not supported.
clusterScope.V(4).Info("Security group rule reconciliation complete")
return reconcile.Result{}, nil
}

Expand Down
4 changes: 2 additions & 2 deletions controllers/osccluster_securitygroup_controller_unit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1134,7 +1134,7 @@ func TestReconcileCreateSecurityGroupFailedCreate(t *testing.T) {
expTagFound: false,
expCreateSecurityGroupErr: fmt.Errorf("CreateSecurityGroup generic error"),
expReadTagErr: nil,
expReconcileSecurityGroupErr: fmt.Errorf("CreateSecurityGroup generic error cannot create securityGroup for Osccluster test-system/test-osc"),
expReconcileSecurityGroupErr: fmt.Errorf("cannot create security group for OscCluster test-system/test-osc: CreateSecurityGroup generic error"),
},
}
for _, sgtc := range securityGroupTestCases {
Expand Down Expand Up @@ -1228,7 +1228,7 @@ func TestReconcileCreateSecurityGroupResourceId(t *testing.T) {
expNetFound: true,
expGetSecurityGroupIdsFromNetIds: nil,
expReadTagErr: fmt.Errorf("ReadTag generic error"),
expReconcileSecurityGroupErr: fmt.Errorf("ReadTag generic error cannot get tag for OscCluster test-system/test-osc"),
expReconcileSecurityGroupErr: fmt.Errorf("cannot get tag for OscCluster test-system/test-osc: ReadTag generic error"),
},
}
for _, sgtc := range securityGroupTestCases {
Expand Down
4 changes: 2 additions & 2 deletions hack/verify-shellcheck.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ run_shellcheck(){
SHELLCHECK_DISABLED="$(join_by , "${disabled[@]}")"
readonly SHELLCHECK_DISABLED
ROOT_PATH=$(git rev-parse --show-toplevel)
IGNORE_FILES=$(find "${ROOT_PATH}" -name "*.sh" | grep "tilt_modules")
IGNORE_FILES=$(find "${ROOT_PATH}" -name "*.sh" | grep "tilt_modules|vendor")
echo "Ignoring shellcheck on ${IGNORE_FILES}"
FILES=$(find "${ROOT_PATH}" -name "*.sh" -not -path "./tilt_modules/*")
FILES=$(find "${ROOT_PATH}" -name "*.sh" -not -path "./tilt_modules/*" -not -path "./vendor/*")
while read -r file; do
"${ROOT_PATH}/bin/shellcheck" "--exclude=${SHELLCHECK_DISABLED}" "--color=auto" -x "$file" >> "${OUT}" 2>&1
done <<< "$FILES"
Expand Down
Loading

0 comments on commit d289669

Please sign in to comment.