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

🐛 Don't apply worker SG to control plane machines #1785

Merged
merged 1 commit into from
Dec 11, 2023
Merged

🐛 Don't apply worker SG to control plane machines #1785

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
20 changes: 13 additions & 7 deletions controllers/openstackmachine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,15 +499,21 @@ func machineToInstanceSpec(openStackCluster *infrav1.OpenStackCluster, machine *
instanceSpec.SecurityGroups = openStackMachine.Spec.SecurityGroups
if openStackCluster.Spec.ManagedSecurityGroups {
var managedSecurityGroup string
if util.IsControlPlaneMachine(machine) && openStackCluster.Status.ControlPlaneSecurityGroup != nil {
managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
} else if openStackCluster.Status.WorkerSecurityGroup != nil {
managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
if util.IsControlPlaneMachine(machine) {
if openStackCluster.Status.ControlPlaneSecurityGroup != nil {
managedSecurityGroup = openStackCluster.Status.ControlPlaneSecurityGroup.ID
}
Comment on lines +503 to +505
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in practise this would mean we're in some unusual state and we shouldn't reconcile at all until ControlPlaneSecurityGroup is set. However:

  • This is clearly better and safer than the current code
  • @EmilienM is currently working on a refactor which will be able to fix this up after the fact even if we were briefly in a weird state

So I'm happy to merge this.

} else {
if openStackCluster.Status.WorkerSecurityGroup != nil {
managedSecurityGroup = openStackCluster.Status.WorkerSecurityGroup.ID
}
}

instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupFilter{
ID: managedSecurityGroup,
})
if managedSecurityGroup != "" {
instanceSpec.SecurityGroups = append(instanceSpec.SecurityGroups, infrav1.SecurityGroupFilter{
ID: managedSecurityGroup,
})
}
}

instanceSpec.Ports = openStackMachine.Spec.Ports
Expand Down
52 changes: 46 additions & 6 deletions controllers/openstackmachine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ func getDefaultOpenStackMachine() *infrav1.OpenStackMachine {
ServerMetadata: map[string]string{
"test-metadata": "test-value",
},
ConfigDrive: pointer.Bool(true),
ServerGroupID: serverGroupUUID,
ConfigDrive: pointer.Bool(true),
SecurityGroups: []infrav1.SecurityGroupFilter{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you're setting SecurityGroups to an empty list to avoid appending to nil here? If so, you don't need to do that: nil is a valid empty slice. Appending to it will create a slice in the same way that a new slice is created if you exceed the capacity of the existing slice: https://go.dev/play/p/Nhln2sbvEGD

It's also safe and idiomatic to iterate over a nil slice in a for loop: https://go.dev/play/p/E5aqZHV51L0. This means, e.g. you don't need to write an additional guard like:

if (slice != nil) {
    for _, elem := range slice {
        ...
    }
}

Copy link
Contributor Author

@stephenfin stephenfin Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, but the reason I did this is dumber: I had set the instance spec SecurityGroups attribute to an empty list in the test cases, and I modified the default machine to avoid having to also create a custom machine in the test case, which in turn meant I had to modify the default instance spec 😅 With the benefit of hindight, I could have simply not set SecurityGroups in those two additional test cases and thus avoided modifying anything in the defaults. lmk if you want me to respin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth a respin just for this imho.

ServerGroupID: serverGroupUUID,
},
}
}
Expand All @@ -105,10 +106,11 @@ func getDefaultInstanceSpec() *compute.InstanceSpec {
Metadata: map[string]string{
"test-metadata": "test-value",
},
ConfigDrive: *pointer.Bool(true),
FailureDomain: *pointer.String(failureDomain),
ServerGroupID: serverGroupUUID,
Tags: []string{"test-tag"},
ConfigDrive: *pointer.Bool(true),
FailureDomain: *pointer.String(failureDomain),
ServerGroupID: serverGroupUUID,
SecurityGroups: []infrav1.SecurityGroupFilter{},
Tags: []string{"test-tag"},
}
}

Expand Down Expand Up @@ -165,6 +167,44 @@ func Test_machineToInstanceSpec(t *testing.T) {
return i
},
},
{
name: "Control plane security group not applied to worker",
openStackCluster: func() *infrav1.OpenStackCluster {
c := getDefaultOpenStackCluster()
c.Spec.ManagedSecurityGroups = true
c.Status.WorkerSecurityGroup = nil
return c
},
machine: getDefaultMachine,
openStackMachine: getDefaultOpenStackMachine,
wantInstanceSpec: func() *compute.InstanceSpec {
i := getDefaultInstanceSpec()
i.SecurityGroups = []infrav1.SecurityGroupFilter{}
return i
},
},
{
name: "Worker security group not applied to control plane",
openStackCluster: func() *infrav1.OpenStackCluster {
c := getDefaultOpenStackCluster()
c.Spec.ManagedSecurityGroups = true
c.Status.ControlPlaneSecurityGroup = nil
return c
},
machine: func() *clusterv1.Machine {
m := getDefaultMachine()
m.Labels = map[string]string{
clusterv1.MachineControlPlaneLabel: "true",
}
return m
},
openStackMachine: getDefaultOpenStackMachine,
wantInstanceSpec: func() *compute.InstanceSpec {
i := getDefaultInstanceSpec()
i.SecurityGroups = []infrav1.SecurityGroupFilter{}
return i
},
},
{
name: "Extra security group",
openStackCluster: func() *infrav1.OpenStackCluster {
Expand Down