Skip to content

Commit

Permalink
[occm] Not all OpenStack providers support AllowedAddressPairs (#2492)
Browse files Browse the repository at this point in the history
* Ignore allowed_address_pairs if extension is not available

Some cloud providers disable extension allowed_address_pairs

* Update pkg/openstack/routes.go

* Update pkg/openstack/routes.go

* Ignore allowed_address_pairs if port_security_enabled=false for port

Any changes to allowed_address_pairs will returns errors if port_security_enabled is false for port.
Log message in such cases warns users of potential packet drops by Neutron port security as it is only allows port ip to pass through.

---------

Co-authored-by: kayrus <[email protected]>
  • Loading branch information
sircthulhu and kayrus authored Dec 13, 2023
1 parent f3e92a2 commit 822a1de
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 13 deletions.
2 changes: 1 addition & 1 deletion pkg/openstack/openstack.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ func (os *OpenStack) Routes() (cloudprovider.Routes, bool) {
return nil, false
}

r, err := NewRoutes(os, network, netExts["extraroute-atomic"])
r, err := NewRoutes(os, network, netExts["extraroute-atomic"], netExts["allowed-address-pairs"])
if err != nil {
klog.Warningf("Error initialising Routes support: %v", err)
return nil, false
Expand Down
45 changes: 33 additions & 12 deletions pkg/openstack/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package openstack

import (
"context"
openstackutil "k8s.io/cloud-provider-openstack/pkg/util/openstack"
"net"
"sync"

Expand All @@ -43,6 +44,8 @@ type Routes struct {
networkIDs []string
// whether Neutron supports "extraroute-atomic" extension
atomicRoutes bool
// whether Neutron supports "allowed-address-pairs" extension
allowedAddressPairs bool
// Neutron with no "extraroute-atomic" extension can modify only one route at
// once
sync.Mutex
Expand All @@ -51,15 +54,16 @@ type Routes struct {
var _ cloudprovider.Routes = &Routes{}

// NewRoutes creates a new instance of Routes
func NewRoutes(os *OpenStack, network *gophercloud.ServiceClient, atomicRoutes bool) (cloudprovider.Routes, error) {
func NewRoutes(os *OpenStack, network *gophercloud.ServiceClient, atomicRoutes bool, allowedAddressPairs bool) (cloudprovider.Routes, error) {
if os.routeOpts.RouterID == "" {
return nil, errors.ErrNoRouterID
}

return &Routes{
network: network,
os: os,
atomicRoutes: atomicRoutes,
network: network,
os: os,
atomicRoutes: atomicRoutes,
allowedAddressPairs: allowedAddressPairs,
}, nil
}

Expand Down Expand Up @@ -239,7 +243,7 @@ func removeRoute(network *gophercloud.ServiceClient, routerID string, oldRoute [
return unwinder, nil
}

func updateAllowedAddressPairs(network *gophercloud.ServiceClient, port *ports.Port, newPairs []ports.AddressPair) (func(), error) {
func updateAllowedAddressPairs(network *gophercloud.ServiceClient, port *PortWithPortSecurity, newPairs []ports.AddressPair) (func(), error) {
origPairs := port.AllowedAddressPairs // shallow copy

mc := metrics.NewMetricContext("port", "update")
Expand Down Expand Up @@ -329,11 +333,22 @@ func (r *Routes) CreateRoute(ctx context.Context, clusterName string, nameHint s
defer onFailure.call(unwind)
}

if !r.allowedAddressPairs {
klog.V(4).Infof("Route created (skipping the allowed_address_pairs update): %v", route)
onFailure.disarm()
return nil
}

// get the port of addr on target node.
port, err := getPortByIP(r.network, addr, r.networkIDs)
if err != nil {
return err
}
if !port.PortSecurityEnabled {
klog.Warningf("Skipping allowed_address_pair for port: %s", port.ID)
onFailure.disarm()
return nil
}

found := false
for _, item := range port.AllowedAddressPairs {
Expand Down Expand Up @@ -437,11 +452,22 @@ func (r *Routes) DeleteRoute(ctx context.Context, clusterName string, route *clo
defer onFailure.call(unwind)
}

if !r.allowedAddressPairs {
klog.V(4).Infof("Route deleted (skipping the allowed_address_pairs update): %v", route)
onFailure.disarm()
return nil
}

// get the port of addr on target node.
port, err := getPortByIP(r.network, addr, r.networkIDs)
if err != nil {
return err
}
if !port.PortSecurityEnabled {
klog.Warningf("Skipping allowed_address_pair for port: %s", port.ID)
onFailure.disarm()
return nil
}

addrPairs := port.AllowedAddressPairs
index := -1
Expand Down Expand Up @@ -469,7 +495,7 @@ func (r *Routes) DeleteRoute(ctx context.Context, clusterName string, route *clo
return nil
}

func getPortByIP(network *gophercloud.ServiceClient, addr string, networkIDs []string) (*ports.Port, error) {
func getPortByIP(network *gophercloud.ServiceClient, addr string, networkIDs []string) (*PortWithPortSecurity, error) {
for _, networkID := range networkIDs {
opts := ports.ListOpts{
FixedIPs: []ports.FixedIPOpts{
Expand All @@ -479,12 +505,7 @@ func getPortByIP(network *gophercloud.ServiceClient, addr string, networkIDs []s
},
NetworkID: networkID,
}
mc := metrics.NewMetricContext("port", "list")
pages, err := ports.List(network, opts).AllPages()
if mc.ObserveRequest(err) != nil {
return nil, err
}
ports, err := ports.ExtractPorts(pages)
ports, err := openstackutil.GetPorts[PortWithPortSecurity](network, opts)
if err != nil {
return nil, err
}
Expand Down

0 comments on commit 822a1de

Please sign in to comment.