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

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

Closed
wants to merge 1 commit into from
Closed

ioBundle->NetworkPortStatus matching tightening for IsPort() #4460

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
4 changes: 2 additions & 2 deletions pkg/pillar/cmd/diag/diag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,9 +887,9 @@ func printOutput(ctx *diagContext, caller string) {
// Print usefully formatted info based on which
// fields are set and Dhcp type; proxy info order
ifname := port.IfName
isMgmt := types.IsMgmtPort(*ctx.DeviceNetworkStatus, ifname)
isMgmt := types.IsMgmtPort(*ctx.DeviceNetworkStatus, ifname, port.USBAddr, port.USBProd, port.PCIAddr)
cost := types.GetPortCost(*ctx.DeviceNetworkStatus,
ifname)
ifname, port.USBAddr, port.USBProd, port.PCIAddr)
if isMgmt {
mgmtPorts++
}
Expand Down
14 changes: 2 additions & 12 deletions pkg/pillar/cmd/domainmgr/domainmgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"path/filepath"
"strconv"
"strings"
"sync"
"time"

"github.com/containerd/cgroups"
Expand Down Expand Up @@ -68,21 +67,12 @@ const (
// Really a constant
var nilUUID = uuid.UUID{}

func isPort(ctx *domainContext, ifname string) bool {
ctx.dnsLock.Lock()
defer ctx.dnsLock.Unlock()
return types.IsPort(ctx.deviceNetworkStatus, ifname)
}

// Information for handleCreate/Modify/Delete
type domainContext struct {
agentbase.AgentBase
ps *pubsub.PubSub
// The isPort function is called by different goroutines
// hence we serialize the calls on a mutex.
ps *pubsub.PubSub
decryptCipherContext cipher.DecryptCipherContext
deviceNetworkStatus types.DeviceNetworkStatus
dnsLock sync.Mutex
assignableAdapters *types.AssignableAdapters
DNSinitialized bool // Received DeviceNetworkStatus
subDeviceNetworkStatus pubsub.Subscription
Expand Down Expand Up @@ -3154,7 +3144,7 @@ func updatePortAndPciBackIoBundle(ctx *domainContext, ib *types.IoBundle) (chang
// EVE controller doesn't know it
list = aa.ExpandControllers(log, list, hyper.PCISameController)
for _, ib := range list {
if types.IsPort(ctx.deviceNetworkStatus, ib.Ifname) {
if types.IsPort(ctx.deviceNetworkStatus, ib.Ifname, ib.UsbAddr, ib.UsbProduct, ib.PciLong) {
isPort = true
keepInHost = true
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/pillar/cmd/wstunnelclient/wstunnelclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func scanAIConfigs(ctx *wstunnelclientContext) {
deviceNetworkStatus := ctx.dnsContext.deviceNetworkStatus
for _, port := range deviceNetworkStatus.Ports {
ifname := port.IfName
if !types.IsMgmtPort(*deviceNetworkStatus, ifname) {
if !types.IsMgmtPort(*deviceNetworkStatus, ifname, port.USBAddr, port.USBProd, port.PCIAddr) {
log.Tracef("Skipping connection using non-mangement intf %s\n",
ifname)
continue
Expand Down
1 change: 1 addition & 0 deletions pkg/pillar/cmd/zedagent/parseconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,7 @@ func propagatePhyioAttrsToPort(port *types.NetworkPortConfig, phyio *types.Physi
port.Phylabel = phyio.Phylabel
port.IfName = phyio.Phyaddr.Ifname
port.USBAddr = phyio.Phyaddr.UsbAddr
port.USBProd = phyio.Phyaddr.UsbProduct
port.PCIAddr = phyio.Phyaddr.PciLong
if port.IfName == "" {
// Inside device model, network adapter may be referenced by PCI or USB address
Expand Down
3 changes: 3 additions & 0 deletions pkg/pillar/dpcmanager/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ func (m *DpcManager) updateDNS() {
m.deviceNetStatus.Ports = make([]types.NetworkPortStatus, len(dpc.Ports))
for ix, port := range dpc.Ports {
m.deviceNetStatus.Ports[ix].IfName = port.IfName
m.deviceNetStatus.Ports[ix].USBAddr = port.USBAddr
m.deviceNetStatus.Ports[ix].USBProd = port.USBProd
m.deviceNetStatus.Ports[ix].PCIAddr = port.PCIAddr
m.deviceNetStatus.Ports[ix].Phylabel = port.Phylabel
m.deviceNetStatus.Ports[ix].Logicallabel = port.Logicallabel
m.deviceNetStatus.Ports[ix].SharedLabels = port.SharedLabels
Expand Down
53 changes: 36 additions & 17 deletions pkg/pillar/types/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ type DeviceNetworkStatus struct {

type NetworkPortStatus struct {
IfName string
USBAddr string
andrewd-zededa marked this conversation as resolved.
Show resolved Hide resolved
USBProd string
PCIAddr string
Phylabel string // Physical name set by controller/model
Logicallabel string
// Unlike the logicallabel, which is defined in the device model and unique
Expand Down Expand Up @@ -682,31 +685,38 @@ func getLocalAddrListImpl(dns DeviceNetworkStatus,
}

// Check if an interface name is a port owned by nim
func IsPort(dns DeviceNetworkStatus, ifname string) bool {
func IsPort(dns DeviceNetworkStatus, ifname string, usbaddr string, usbprod string, pciaddr string) bool {
for _, us := range dns.Ports {
if us.IfName != ifname {
if ifname != "" && us.IfName != ifname {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? It causes the complete breakage since a lookup of e.g. COM1 or audio0 (which do not have an ifname by definition) will match the first item in dns.Ports. If that first item is a network port like eth0, then IsPort will incorrectly return true.

continue
}
return true
}
return false
}

// IsL3Port checks if an interface name belongs to a port with SystemAdapter attached.
func IsL3Port(dns DeviceNetworkStatus, ifname string) bool {
for _, us := range dns.Ports {
if us.IfName != ifname {
if us.USBAddr != usbaddr {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether we need to treat empty strings as wildcards in some of these comparisons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out that is at least required for PCIAddr in IsPort since the comparison is between the model aka SystemAdapter, which might have an empty PCIAddr and the discovered PCIAddr on the running system, which is not empty.

continue
}
if us.USBProd != usbprod {
continue
}
return us.IsL3Port
if us.PCIAddr != pciaddr {
continue
}
return true
}
return false
}

// Check if a physical label or ifname is a management port
func IsMgmtPort(dns DeviceNetworkStatus, ifname string) bool {
func IsMgmtPort(dns DeviceNetworkStatus, ifname string, usbaddr string, usbprod string, pciaddr string) bool {
for _, us := range dns.Ports {
if us.IfName != ifname {
if ifname != "" && us.IfName != ifname {
continue
}
if us.USBAddr != usbaddr {
continue
}
if us.USBProd != usbprod {
continue
}
if us.PCIAddr != pciaddr {
continue
}
if dns.Version >= DPCIsMgmt &&
Expand All @@ -720,9 +730,18 @@ func IsMgmtPort(dns DeviceNetworkStatus, ifname string) bool {

// GetPortCost returns the port cost
// Returns 0 if the ifname does not exist.
func GetPortCost(dns DeviceNetworkStatus, ifname string) uint8 {
func GetPortCost(dns DeviceNetworkStatus, ifname string, usbaddr string, usbprod string, pciaddr string) uint8 {
for _, us := range dns.Ports {
if us.IfName != ifname {
if ifname != "" && us.IfName != ifname {
continue
}
if us.USBAddr != usbaddr {
continue
}
if us.USBProd != usbprod {
continue
}
if us.PCIAddr != pciaddr {
continue
}
return us.Cost
Expand All @@ -732,7 +751,7 @@ func GetPortCost(dns DeviceNetworkStatus, ifname string) uint8 {

func GetPort(dns DeviceNetworkStatus, ifname string) *NetworkPortStatus {
for _, us := range dns.Ports {
if us.IfName != ifname {
if ifname != "" && us.IfName != ifname {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have callers which pass an empty string as the ifname argument? If they do this will match on the first management port which might not be what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getLocalAddrListImpl() only calls it with a non empty ifname.

Could VerifyAllIntf() pass an empty string?

continue
}
if dns.Version < DPCIsMgmt {
Expand Down
2 changes: 2 additions & 0 deletions pkg/pillar/types/dpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ func (config *DevicePortConfig) MostlyEqual(config2 *DevicePortConfig) bool {
if p1.IfName != p2.IfName ||
p1.PCIAddr != p2.PCIAddr ||
p1.USBAddr != p2.USBAddr ||
p1.USBProd != p2.USBProd ||
p1.Phylabel != p2.Phylabel ||
p1.Logicallabel != p2.Logicallabel ||
!generics.EqualSets(p1.SharedLabels, p2.SharedLabels) ||
Expand Down Expand Up @@ -567,6 +568,7 @@ func (config *DevicePortConfig) IsAnyPortInPciBack(
type NetworkPortConfig struct {
IfName string
USBAddr string
USBProd string
PCIAddr string
Phylabel string // Physical name set by controller/model
Logicallabel string // SystemAdapter's name which is logical label in phyio
Expand Down
Loading