Skip to content

Commit 8973a4d

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #2159 from shiftstack/issue_1341
🐛 port: don't add any SGs when port security is disabled
2 parents 90da86d + 4a7744a commit 8973a4d

File tree

6 files changed

+109
-7
lines changed

6 files changed

+109
-7
lines changed

‎pkg/cloud/services/networking/port.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,9 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol
170170
createOpts.FixedIPs = fixedIPs
171171
}
172172
if portSpec.SecurityGroups != nil {
173+
if ptr.Deref(portSpec.DisablePortSecurity, false) {
174+
return nil, errors.New("security groups cannot be set when port security is disabled")
175+
}
173176
createOpts.SecurityGroups = &portSpec.SecurityGroups
174177
}
175178
builder = createOpts
@@ -434,13 +437,15 @@ func (s *Service) normalizePorts(ports []infrav1.PortOpts, clusterResourceName,
434437
return nil, err
435438
}
436439

437-
// Resolve security groups
438-
if len(port.SecurityGroups) == 0 {
439-
normalizedPort.SecurityGroups = defaultSecurityGroupIDs
440-
} else {
441-
normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
442-
if err != nil {
443-
return nil, fmt.Errorf("error getting security groups: %v", err)
440+
// Resolve security groups when port security is not disabled
441+
if !ptr.Deref(port.DisablePortSecurity, false) {
442+
if len(port.SecurityGroups) == 0 {
443+
normalizedPort.SecurityGroups = defaultSecurityGroupIDs
444+
} else {
445+
normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
446+
if err != nil {
447+
return nil, fmt.Errorf("error getting security groups: %v", err)
448+
}
444449
}
445450
}
446451
}

‎pkg/cloud/services/networking/port_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,21 @@ func Test_CreatePort(t *testing.T) {
192192
},
193193
want: &ports.Port{ID: portID},
194194
},
195+
{
196+
name: "disable port security with security groups produces an error",
197+
port: infrav1.ResolvedPortSpec{
198+
Name: "test-port",
199+
NetworkID: netID,
200+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
201+
DisablePortSecurity: ptr.To(true),
202+
},
203+
SecurityGroups: []string{portSecurityGroupID},
204+
},
205+
expect: func(m *mock.MockNetworkClientMockRecorder, _ Gomega) {
206+
m.CreatePort(gomock.Any()).Times(0)
207+
},
208+
wantErr: true,
209+
},
195210
{
196211
name: "disable port security also ignores allowed address pairs",
197212
port: infrav1.ResolvedPortSpec{
@@ -780,6 +795,40 @@ func TestService_ConstructPorts(t *testing.T) {
780795
},
781796
},
782797
},
798+
{
799+
name: "port security disabled override machine spec security groups",
800+
spec: infrav1.OpenStackMachineSpec{
801+
SecurityGroups: []infrav1.SecurityGroupParam{
802+
{Filter: &infrav1.SecurityGroupFilter{Name: "machine-security-group"}},
803+
},
804+
Ports: []infrav1.PortOpts{
805+
{
806+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
807+
DisablePortSecurity: ptr.To(true),
808+
},
809+
},
810+
},
811+
},
812+
expectNetwork: func(m *mock.MockNetworkClientMockRecorder) {
813+
m.ListSecGroup(groups.ListOpts{Name: "machine-security-group"}).Return([]groups.SecGroup{
814+
{ID: securityGroupID1},
815+
}, nil)
816+
},
817+
want: []infrav1.ResolvedPortSpec{
818+
{
819+
Name: "test-instance-0",
820+
NetworkID: defaultNetworkID,
821+
FixedIPs: []infrav1.ResolvedFixedIP{
822+
{SubnetID: ptr.To(defaultSubnetID)},
823+
},
824+
Description: defaultDescription,
825+
Tags: []string{"test-tag"},
826+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
827+
DisablePortSecurity: ptr.To(true),
828+
},
829+
},
830+
},
831+
},
783832
}
784833
for i := range tests {
785834
tt := &tests[i]

‎pkg/webhooks/openstackmachine_webhook.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
apierrors "k8s.io/apimachinery/pkg/api/errors"
2525
"k8s.io/apimachinery/pkg/runtime"
2626
"k8s.io/apimachinery/pkg/util/validation/field"
27+
"k8s.io/utils/ptr"
2728
"sigs.k8s.io/controller-runtime/pkg/builder"
2829
"sigs.k8s.io/controller-runtime/pkg/manager"
2930
"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -62,6 +63,12 @@ func (*openStackMachineWebhook) ValidateCreate(_ context.Context, objRaw runtime
6263
}
6364
}
6465

66+
for _, port := range newObj.Spec.Ports {
67+
if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
68+
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
69+
}
70+
}
71+
6572
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
6673
}
6774

‎pkg/webhooks/openstackserver_webhook.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
apierrors "k8s.io/apimachinery/pkg/api/errors"
2525
"k8s.io/apimachinery/pkg/runtime"
2626
"k8s.io/apimachinery/pkg/util/validation/field"
27+
"k8s.io/utils/ptr"
2728
"sigs.k8s.io/cluster-api/util/topology"
2829
"sigs.k8s.io/controller-runtime/pkg/builder"
2930
"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -64,6 +65,12 @@ func (*openStackServerWebhook) ValidateCreate(_ context.Context, objRaw runtime.
6465
}
6566
}
6667

68+
for _, port := range newObj.Spec.Ports {
69+
if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
70+
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
71+
}
72+
}
73+
6774
return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
6875
}
6976

‎test/e2e/suites/apivalidations/openstackmachine_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,23 @@ var _ = Describe("OpenStackMachine API validations", func() {
172172
Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with root device name in spec.AdditionalBlockDevices should not succeed")
173173
})
174174

175+
It("should not allow to create machine with both SecurityGroups and DisablePortSecurity", func() {
176+
machine := defaultMachine()
177+
machine.Spec.Ports = []infrav1.PortOpts{
178+
{
179+
SecurityGroups: []infrav1.SecurityGroupParam{{
180+
Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
181+
}},
182+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
183+
DisablePortSecurity: ptr.To(true),
184+
},
185+
},
186+
}
187+
188+
By("Creating a machine with both SecurityGroups and DisablePortSecurity")
189+
Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with both SecurityGroups and DisablePortSecurity should not succeed")
190+
})
191+
175192
/* FIXME: These tests are failing
176193
It("should not allow additional volume with empty name", func() {
177194
machine.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{

‎test/e2e/suites/apivalidations/openstackserver_test.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,23 @@ var _ = Describe("OpenStackServer API validations", func() {
150150
Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackserver creation with root device name in spec.AdditionalBlockDevices should not succeed")
151151
})
152152

153+
It("should not allow to create server with both SecurityGroups and DisablePortSecurity", func() {
154+
server := defaultServer()
155+
server.Spec.Ports = []infrav1.PortOpts{
156+
{
157+
SecurityGroups: []infrav1.SecurityGroupParam{{
158+
Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
159+
}},
160+
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
161+
DisablePortSecurity: ptr.To(true),
162+
},
163+
},
164+
}
165+
166+
By("Creating a server with both SecurityGroups and DisablePortSecurity")
167+
Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackServer creation with both SecurityGroups and DisablePortSecurity should not succeed")
168+
})
169+
153170
/* FIXME: These tests are failing
154171
It("should not allow additional volume with empty name", func() {
155172
server.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{

0 commit comments

Comments
 (0)