Skip to content

🐛 port: don't add any SGs when port security is disabled #2159

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

Merged
merged 1 commit into from
Aug 16, 2024
Merged

🐛 port: don't add any SGs when port security is disabled #2159

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
19 changes: 12 additions & 7 deletions pkg/cloud/services/networking/port.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@ func (s *Service) CreatePort(eventObject runtime.Object, portSpec *infrav1.Resol
createOpts.FixedIPs = fixedIPs
}
if portSpec.SecurityGroups != nil {
if ptr.Deref(portSpec.DisablePortSecurity, false) {
return nil, errors.New("security groups cannot be set when port security is disabled")
}
Comment on lines +173 to +175
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC if you try to add a security group to a port with port security disabled you get an error, right? i.e. It's not just that it ignores the option, but it actually won't add the security groups?

If so, we should be able to safely add API validation for this instead, because we know there is no working configuration with it set.

i.e. We should write this as CEL instead. The tests would be in apivalidations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC if you try to add a security group to a port with port security disabled you get an error, right? i.e. It's not just that it ignores the option, but it actually won't add the security groups?

yeah right. And good points on API validations. I'll check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added tests in the webhook like other fields and tests in API validations. I've let this check on purpose because:

  1. it doesn't hurt, I think
  2. it's safe and it makes sense to report an error and not let it through anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack to leaving the check in.

I'd prefer we didn't add anything new to the webhooks, though, unless we absolutely have to. I take a look to see how easy the CEL is to write.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to withdraw this objection. While the CEL is simple enough to write, unfortunately because it affects both the SecurityGroups and DisablePortSecurity fields it needs to be implemented on the PortOpts struct rather than just one field. Unfortunately this struct is a monster, so I wasn't able to write the rule which doesn't exceed the complexity budget.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my thought when I looked at CEL and why I took the webhook way instead 😕

createOpts.SecurityGroups = &portSpec.SecurityGroups
}
builder = createOpts
Expand Down Expand Up @@ -434,13 +437,15 @@ func (s *Service) normalizePorts(ports []infrav1.PortOpts, clusterResourceName,
return nil, err
}

// Resolve security groups
if len(port.SecurityGroups) == 0 {
normalizedPort.SecurityGroups = defaultSecurityGroupIDs
} else {
normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
if err != nil {
return nil, fmt.Errorf("error getting security groups: %v", err)
// Resolve security groups when port security is not disabled
if !ptr.Deref(port.DisablePortSecurity, false) {
if len(port.SecurityGroups) == 0 {
normalizedPort.SecurityGroups = defaultSecurityGroupIDs
} else {
normalizedPort.SecurityGroups, err = s.GetSecurityGroups(port.SecurityGroups)
if err != nil {
return nil, fmt.Errorf("error getting security groups: %v", err)
}
}
}
}
Expand Down
49 changes: 49 additions & 0 deletions pkg/cloud/services/networking/port_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,21 @@ func Test_CreatePort(t *testing.T) {
},
want: &ports.Port{ID: portID},
},
{
name: "disable port security with security groups produces an error",
port: infrav1.ResolvedPortSpec{
Name: "test-port",
NetworkID: netID,
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
DisablePortSecurity: ptr.To(true),
},
SecurityGroups: []string{portSecurityGroupID},
},
expect: func(m *mock.MockNetworkClientMockRecorder, _ Gomega) {
m.CreatePort(gomock.Any()).Times(0)
},
wantErr: true,
},
{
name: "disable port security also ignores allowed address pairs",
port: infrav1.ResolvedPortSpec{
Expand Down Expand Up @@ -780,6 +795,40 @@ func TestService_ConstructPorts(t *testing.T) {
},
},
},
{
name: "port security disabled override machine spec security groups",
spec: infrav1.OpenStackMachineSpec{
SecurityGroups: []infrav1.SecurityGroupParam{
{Filter: &infrav1.SecurityGroupFilter{Name: "machine-security-group"}},
},
Ports: []infrav1.PortOpts{
{
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
DisablePortSecurity: ptr.To(true),
},
},
},
},
expectNetwork: func(m *mock.MockNetworkClientMockRecorder) {
m.ListSecGroup(groups.ListOpts{Name: "machine-security-group"}).Return([]groups.SecGroup{
{ID: securityGroupID1},
}, nil)
},
want: []infrav1.ResolvedPortSpec{
{
Name: "test-instance-0",
NetworkID: defaultNetworkID,
FixedIPs: []infrav1.ResolvedFixedIP{
{SubnetID: ptr.To(defaultSubnetID)},
},
Description: defaultDescription,
Tags: []string{"test-tag"},
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
DisablePortSecurity: ptr.To(true),
},
},
},
},
}
for i := range tests {
tt := &tests[i]
Expand Down
7 changes: 7 additions & 0 deletions pkg/webhooks/openstackmachine_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -62,6 +63,12 @@ func (*openStackMachineWebhook) ValidateCreate(_ context.Context, objRaw runtime
}
}

for _, port := range newObj.Spec.Ports {
if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
}
}

return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/webhooks/openstackserver_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/utils/ptr"
"sigs.k8s.io/cluster-api/util/topology"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/manager"
Expand Down Expand Up @@ -64,6 +65,12 @@ func (*openStackServerWebhook) ValidateCreate(_ context.Context, objRaw runtime.
}
}

for _, port := range newObj.Spec.Ports {
if ptr.Deref(port.DisablePortSecurity, false) && len(port.SecurityGroups) > 0 {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "ports"), "cannot have security groups when DisablePortSecurity is set to true"))
}
}
Comment on lines +68 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we can't do this in CEL? We haven't added any new logic to the webhooks in a while and I'd like to phase them out.


return aggregateObjErrors(newObj.GroupVersionKind().GroupKind(), newObj.Name, allErrs)
}

Expand Down
17 changes: 17 additions & 0 deletions test/e2e/suites/apivalidations/openstackmachine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,23 @@ var _ = Describe("OpenStackMachine API validations", func() {
Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with root device name in spec.AdditionalBlockDevices should not succeed")
})

It("should not allow to create machine with both SecurityGroups and DisablePortSecurity", func() {
machine := defaultMachine()
machine.Spec.Ports = []infrav1.PortOpts{
{
SecurityGroups: []infrav1.SecurityGroupParam{{
Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
}},
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
DisablePortSecurity: ptr.To(true),
},
},
}

By("Creating a machine with both SecurityGroups and DisablePortSecurity")
Expect(k8sClient.Create(ctx, machine)).NotTo(Succeed(), "OpenStackMachine creation with both SecurityGroups and DisablePortSecurity should not succeed")
})

/* FIXME: These tests are failing
It("should not allow additional volume with empty name", func() {
machine.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{
Expand Down
17 changes: 17 additions & 0 deletions test/e2e/suites/apivalidations/openstackserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,23 @@ var _ = Describe("OpenStackServer API validations", func() {
Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackserver creation with root device name in spec.AdditionalBlockDevices should not succeed")
})

It("should not allow to create server with both SecurityGroups and DisablePortSecurity", func() {
server := defaultServer()
server.Spec.Ports = []infrav1.PortOpts{
{
SecurityGroups: []infrav1.SecurityGroupParam{{
Filter: &infrav1.SecurityGroupFilter{Name: "test-security-group"},
}},
ResolvedPortSpecFields: infrav1.ResolvedPortSpecFields{
DisablePortSecurity: ptr.To(true),
},
},
}

By("Creating a server with both SecurityGroups and DisablePortSecurity")
Expect(k8sClient.Create(ctx, server)).NotTo(Succeed(), "OpenStackServer creation with both SecurityGroups and DisablePortSecurity should not succeed")
})

/* FIXME: These tests are failing
It("should not allow additional volume with empty name", func() {
server.Spec.AdditionalBlockDevices = []infrav1.AdditionalBlockDevice{
Expand Down