Skip to content

Commit

Permalink
Do not try to use invalid network config
Browse files Browse the repository at this point in the history 
Even if zedagent detects invalid network config and reports DPC with
error, NIM may still try to use it, run connection tests and overwrite
the error with another or even with nil.
Instead, NIM should skip mgmt ports with invalid config and preserve the
error (incl. across reboots).
Similarly, DPC reconciler should not try to apply invalid config into
the network stack.
Additionally, diag was extended to report when port config is not valid.
Lastly, readability of some parsing error messages was improved.

Signed-off-by: Milan Lenco <milan@zededa.com>
(cherry picked from commit 2beb07e)
  • Loading branch information
@milan-zededa
milan-zededa committed Jun 11, 2024
1 parent e068289 commit d49a178
Show file tree
Hide file tree
Showing 12 changed files with 127 additions and 105 deletions.
19 changes: 15 additions & 4 deletions pkg/pillar/cmd/diag/diag.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,10 @@ func printOutput(ctx *diagContext, caller string) {
mgmtPorts++
}

var isValidStr string
if port.InvalidConfig {
isValidStr = " (invalid config)"
}
typeStr := "use: app-shared "
if isMgmt {
if priority == types.PortCostMin {
Expand Down Expand Up @@ -921,8 +925,8 @@ func printOutput(ctx *diagContext, caller string) {
ipCount++
noGeo := ipinfo.IPInfo{}
if dpcSuccess {
ctx.ph.Print("INFO: Port %s: %s%s%s%s\n",
ifname, macStr, linkStr, typeStr, ai.Addr)
ctx.ph.Print("INFO: Port %s%s: %s%s%s%s\n",
ifname, isValidStr, macStr, linkStr, typeStr, ai.Addr)
} else if ai.Geo == noGeo {
ctx.ph.Print("INFO: %s: IP address %s not geolocated\n",
ifname, ai.Addr)
Expand All @@ -932,8 +936,8 @@ func printOutput(ctx *diagContext, caller string) {
}
}
if ipCount == 0 {
ctx.ph.Print("INFO: Port %s: %s%s%sNo IP address\n",
ifname, macStr, linkStr, typeStr)
ctx.ph.Print("INFO: Port %s%s: %s%s%sNo IP address\n",
ifname, isValidStr, macStr, linkStr, typeStr)
}

// Skip details if we are connected to controller. Count
Expand Down Expand Up @@ -963,6 +967,13 @@ func printOutput(ctx *diagContext, caller string) {
ifname, port.NtpServer.String())
}
printProxy(ctx, port, ifname)
if port.HasError() {
if port.InvalidConfig {
ctx.ph.Print("ERROR: %s: invalid config: %s\n", ifname, port.LastError)
} else {
ctx.ph.Print("ERROR: %s: has error: %s\n", ifname, port.LastError)
}
}

if !isMgmt {
ctx.ph.Print("INFO: %s: not intended for EV controller; skipping those tests\n",
Expand Down
121 changes: 50 additions & 71 deletions pkg/pillar/cmd/zedagent/parseconfig.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,8 +466,9 @@ func publishNetworkInstanceConfig(ctx *getconfigContext,
Activate: apiConfigEntry.Activate,
PropagateConnRoutes: apiConfigEntry.PropagateConnectedRoutes,
}
uuidStr := networkInstanceConfig.UUID.String()
log.Functionf("publishNetworkInstanceConfig: processing %s %s type %d activate %v",
networkInstanceConfig.UUID.String(), networkInstanceConfig.DisplayName,
uuidStr, networkInstanceConfig.DisplayName,
networkInstanceConfig.Type, networkInstanceConfig.Activate)

if apiConfigEntry.Port != nil {
Expand All @@ -480,7 +481,7 @@ func publishNetworkInstanceConfig(ctx *getconfigContext,
// network instances
if networkInstanceConfig.IpType != types.AddressTypeNone {
log.Errorf("Switch network instance %s %s with invalid IpType %d should be %d",
networkInstanceConfig.UUID.String(),
uuidStr,
networkInstanceConfig.DisplayName,
networkInstanceConfig.IpType,
types.AddressTypeNone)
Expand All @@ -494,9 +495,8 @@ func publishNetworkInstanceConfig(ctx *getconfigContext,
if networkInstanceConfig.IpType != types.AddressTypeNone {
err := parseIpspec(apiConfigEntry.Ip, &networkInstanceConfig)
if err != nil {
errStr := fmt.Sprintf("Network Instance %s parameter parse failed: %s",
networkInstanceConfig.Key(), err)
log.Error(errStr)
errStr := fmt.Sprintf("Invalid IP configuration: %s", err)
log.Errorf("publishNetworkInstanceConfig (%s): %s", uuidStr, errStr)
networkInstanceConfig.SetErrorNow(errStr)
// Proceed to send error back to controller
}
Expand All @@ -505,10 +505,9 @@ func publishNetworkInstanceConfig(ctx *getconfigContext,
for _, route := range apiConfigEntry.StaticRoutes {
err := parseStaticRoute(route, &networkInstanceConfig)
if err != nil {
err = fmt.Errorf("network Instance %s IP route %v parsing failed: %w",
networkInstanceConfig.Key(), route, err)
log.Error(err)
networkInstanceConfig.SetErrorNow(err.Error())
errStr := fmt.Sprintf("Invalid IP route (%v): %v", route, err)
log.Errorf("publishNetworkInstanceConfig (%s): %s", uuidStr, errStr)
networkInstanceConfig.SetErrorNow(errStr)
// Proceed to send error back to controller
}
}
Expand Down Expand Up @@ -757,7 +756,7 @@ func parseSystemAdapterConfig(getconfigCtx *getconfigContext, config *zconfig.Ed
// Check if all management ports have errors
// Propagate any parse errors for all ports to the DPC
// since controller expects LastError and LastFailed for the DPC
ok := false
hasValidMgmtPort := false
mgmtCount := 0
errStr := ""
for _, p := range portConfig.Ports {
Expand All @@ -766,12 +765,12 @@ func parseSystemAdapterConfig(getconfigCtx *getconfigContext, config *zconfig.Ed
}
mgmtCount++
if !p.HasError() {
ok = true
hasValidMgmtPort = true
break
}
errStr += p.LastError + "\n"
}
if !ok {
if !hasValidMgmtPort {
if errStr == "" && portConfig.HasError() {
errStr = portConfig.LastError
}
Expand Down Expand Up @@ -948,6 +947,9 @@ func validateAndAssignNetPorts(dpc *types.DevicePortConfig, newPorts []*types.Ne

// 4. Assign all non-duplicate, validated ports.
for _, port := range validatedPorts {
if port.HasError() {
port.InvalidConfig = true
}
dpc.Ports = append(dpc.Ports, *port)
}
}
Expand Down Expand Up @@ -1165,9 +1167,8 @@ func parseOneSystemAdapterConfig(getconfigCtx *getconfigContext,
if sysAdapter.Addr != "" {
ip = net.ParseIP(sysAdapter.Addr)
if ip == nil {
errStr := fmt.Sprintf("Device Config Error. Port %s has Bad "+
"SysAdapter.Addr %s. The IP address is ignored. Please fix the "+
"device configuration.", sysAdapter.Name, sysAdapter.Addr)
errStr := fmt.Sprintf("Port %s configured with invalid IP address (%s)",
sysAdapter.Name, sysAdapter.Addr)
log.Errorf("parseSystemAdapterConfig: %s", errStr)
port.RecordFailure(errStr)
// IP will not be set below
Expand All @@ -1182,9 +1183,7 @@ func parseOneSystemAdapterConfig(getconfigCtx *getconfigContext,
networkXObject, err := getconfigCtx.pubNetworkXObjectConfig.Get(sysAdapter.NetworkUUID)
if err != nil {
// XXX when do we retry looking for the networkXObject?
errStr := fmt.Sprintf("Device Config Error. Port %s configured with "+
"UNKNOWN Network UUID (%s). Err: %s. Please fix the "+
"device configuration.",
errStr := fmt.Sprintf("Port %s configured with unknown Network UUID (%s): %v",
port.Logicallabel, sysAdapter.NetworkUUID, err)
log.Errorf("parseSystemAdapterConfig: %s", errStr)
port.RecordFailure(errStr)
Expand All @@ -1194,9 +1193,8 @@ func parseOneSystemAdapterConfig(getconfigCtx *getconfigContext,
port.Type = net.Type
network = &net
if network.HasError() {
errStr := fmt.Sprintf("Port %s configured with a network "+
"(UUID: %s) which has an error (%s).",
port.Logicallabel, port.NetworkUUID, network.Error)
errStr := fmt.Sprintf("Network %s assigned to port %s has invalid config: %v",
port.NetworkUUID, port.Logicallabel, network.Error)
log.Errorf("parseSystemAdapterConfig: %s", errStr)
port.RecordFailure(errStr)
}
Expand All @@ -1217,10 +1215,9 @@ func parseOneSystemAdapterConfig(getconfigCtx *getconfigContext,
port.Dhcp = network.Dhcp
switch port.Dhcp {
case types.DhcpTypeStatic:
if port.AddrSubnet == "" {
errStr := fmt.Sprintf("Port %s Configured as DhcpTypeStatic but "+
"missing subnet address. SysAdapter - Name: %s, Addr:%s",
port.Logicallabel, sysAdapter.Name, sysAdapter.Addr)
if sysAdapter.Addr == "" {
errStr := fmt.Sprintf("Port %s configured with static IP config "+
"but IP address is not defined", port.Logicallabel)
log.Errorf("parseSystemAdapterConfig: %s", errStr)
port.RecordFailure(errStr)
}
Expand Down Expand Up @@ -1248,8 +1245,8 @@ func parseOneSystemAdapterConfig(getconfigCtx *getconfigContext,
}
}
} else if isMgmt {
errStr := fmt.Sprintf("Port %s Configured as Management port without "+
"configuring a Network. Network is required for Management ports",
errStr := fmt.Sprintf("Port %s configured as Management port but without "+
"Network assigned. Network is required for Management ports",
port.Logicallabel)
log.Errorf("parseSystemAdapterConfig: %s", errStr)
port.RecordFailure(errStr)
Expand Down Expand Up @@ -1789,9 +1786,8 @@ func parseOneNetworkXObjectConfig(ctx *getconfigContext, netEnt *zconfig.Network
config.Type = types.NetworkType(netEnt.Type)
id, err := uuid.FromString(netEnt.Id)
if err != nil {
errStr := fmt.Sprintf("parseOneNetworkXObjectConfig: Malformed UUID ignored: %s",
err)
log.Error(errStr)
errStr := fmt.Sprintf("Malformed UUID ignored: %s", err)
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
Expand Down Expand Up @@ -1849,17 +1845,15 @@ func parseOneNetworkXObjectConfig(ctx *getconfigContext, netEnt *zconfig.Network
switch config.Type {
case types.NetworkTypeIPv4, types.NetworkTypeIPV6:
if ipspec == nil {
errStr := fmt.Sprintf("parseOneNetworkXObjectConfig: Missing ipspec for %s in %v",
config.Key(), netEnt)
log.Error(errStr)
errStr := "Missing IP configuration"
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
err := parseIpspecNetworkXObject(ipspec, config)
if err != nil {
errStr := fmt.Sprintf("Network parameter parse for %s failed: %s",
config.Key(), err)
log.Error(errStr)
errStr := fmt.Sprintf("Invalid IP configuration: %s", err)
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
Expand All @@ -1870,18 +1864,16 @@ func parseOneNetworkXObjectConfig(ctx *getconfigContext, netEnt *zconfig.Network
config.Key(), ipspec)
err := parseIpspecNetworkXObject(ipspec, config)
if err != nil {
errStr := fmt.Sprintf("Network parameter parse for %s failed: %s",
config.Key(), err)
log.Error(errStr)
errStr := fmt.Sprintf("Invalid IP configuration: %s", err)
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
}

default:
errStr := fmt.Sprintf("parseOneNetworkXObjectConfig: Unknown NetworkConfig type %d for %s in %v; ignored",
config.Type, id.String(), netEnt)
log.Error(errStr)
errStr := fmt.Sprintf("Unknown Network type (%d)", config.Type)
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
Expand All @@ -1901,9 +1893,8 @@ func parseOneNetworkXObjectConfig(ctx *getconfigContext, netEnt *zconfig.Network
if ip != nil {
ips = append(ips, ip)
} else {
errStr := fmt.Sprintf("parseOneNetworkXObjectConfig: bad dnsEntry %s for %s",
strAddr, config.Key())
log.Error(errStr)
errStr := fmt.Sprintf("Invalid DNS entry address (%s)", strAddr)
log.Errorf("parseOneNetworkXObjectConfig (%s): %s", config.Key(), errStr)
config.SetErrorNow(errStr)
return config
}
Expand Down Expand Up @@ -2038,43 +2029,37 @@ func parseIpspecNetworkXObject(ipspec *zconfig.Ipspec, config *types.NetworkXObj
if s := ipspec.GetSubnet(); s != "" {
_, subnet, err := net.ParseCIDR(s)
if err != nil {
return errors.New(fmt.Sprintf("bad subnet %s: %s",
s, err))
return fmt.Errorf("invalid subnet (%s): %w", s, err)
}
config.Subnet = *subnet
}
if g := ipspec.GetGateway(); g != "" {
config.Gateway = net.ParseIP(g)
if config.Gateway == nil {
return errors.New(fmt.Sprintf("bad gateway IP %s",
g))
return fmt.Errorf("invalid gateway IP (%s)", g)
}
}
if n := ipspec.GetNtp(); n != "" {
config.NTPServer = net.ParseIP(n)
if config.NTPServer == nil {
return errors.New(fmt.Sprintf("bad ntp IP %s",
n))
return fmt.Errorf("invalid NTP IP (%s)", n)
}
}
for _, dsStr := range ipspec.GetDns() {
ds := net.ParseIP(dsStr)
if ds == nil {
return errors.New(fmt.Sprintf("bad dns IP %s",
dsStr))
return fmt.Errorf("invalid DNS IP (%s)", dsStr)
}
config.DNSServers = append(config.DNSServers, ds)
}
if dr := ipspec.GetDhcpRange(); dr != nil && dr.GetStart() != "" {
start := net.ParseIP(dr.GetStart())
if start == nil {
return errors.New(fmt.Sprintf("bad start IP %s",
dr.GetStart()))
return fmt.Errorf("invalid DHCP range start IP (%s)", dr.GetStart())
}
end := net.ParseIP(dr.GetEnd())
if end == nil && dr.GetEnd() != "" {
return errors.New(fmt.Sprintf("bad end IP %s",
dr.GetEnd()))
return fmt.Errorf("invalid DHCP range end IP (%s)", dr.GetEnd())
}
config.DhcpRange.Start = start
config.DhcpRange.End = end
Expand All @@ -2090,56 +2075,50 @@ func parseIpspec(ipspec *zconfig.Ipspec,
if n := ipspec.GetNtp(); n != "" {
config.NtpServer = net.ParseIP(n)
if config.NtpServer == nil {
return errors.New(fmt.Sprintf("bad ntp IP %s",
n))
return fmt.Errorf("invalid NTP IP (%s)", n)
}
}
// Parse Dns Servers
for _, dsStr := range ipspec.GetDns() {
ds := net.ParseIP(dsStr)
if ds == nil {
return errors.New(fmt.Sprintf("bad dns IP %s",
dsStr))
return fmt.Errorf("invalid DNS IP (%s)", dsStr)
}
config.DnsServers = append(config.DnsServers, ds)
}
// Parse Subnet
if s := ipspec.GetSubnet(); s != "" {
_, subnet, err := net.ParseCIDR(s)
if err != nil {
return errors.New(fmt.Sprintf("bad subnet %s: %s",
s, err))
return fmt.Errorf("invalid subnet (%s): %w", s, err)
}
config.Subnet = *subnet
}
// Parse Gateway
if g := ipspec.GetGateway(); g != "" {
config.Gateway = net.ParseIP(g)
if config.Gateway == nil {
return errors.New(fmt.Sprintf("bad gateway IP %s",
g))
return fmt.Errorf("invalid gateway IP (%s)", g)
}
}
// Parse DhcpRange
if dr := ipspec.GetDhcpRange(); dr != nil && dr.GetStart() != "" {
start := net.ParseIP(dr.GetStart())
if start == nil {
return errors.New(fmt.Sprintf("bad start IP %s",
dr.GetStart()))
return fmt.Errorf("invalid DHCP range start IP (%s)", dr.GetStart())
}
end := net.ParseIP(dr.GetEnd())
if end == nil && dr.GetEnd() != "" {
return errors.New(fmt.Sprintf("bad end IP %s",
dr.GetEnd()))
return fmt.Errorf("invalid DHCP range end IP (%s)", dr.GetEnd())
}
config.DhcpRange.Start = start
config.DhcpRange.End = end
}

addrCount := netutils.GetIPAddrCountOnSubnet(config.Subnet)
if addrCount < types.MinSubnetSize {
return fmt.Errorf("network(%s), Subnet too small(%d)",
config.Key(), addrCount)
return fmt.Errorf("subnet is too small (only %d available IP addresses, need %d)",
addrCount, types.MinSubnetSize)
}

// if not set, take some default
Expand Down
2 changes: 1 addition & 1 deletion pkg/pillar/cmd/zedagent/parseconfig_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestDPCWithError(t *testing.T) {
g.Expect(dpc.LastFailed).ToNot(BeZero())
g.Expect(dpc.LastSucceeded).To(BeZero())
g.Expect(getPortError(&dpc, "adapter-shopfloor")).
To(ContainSubstring("UNKNOWN Network UUID"))
To(ContainSubstring("Port adapter-shopfloor configured with unknown Network UUID"))
g.Expect(dpc.Ports).To(HaveLen(1))
port := dpc.Ports[0]
g.Expect(port.Logicallabel).To(Equal("adapter-shopfloor"))
Expand Down
7 changes: 6 additions & 1 deletion pkg/pillar/cmd/zedrouter/networkinstance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,12 @@ func (z *zedrouter) setSelectedUplink(uplinkLogicalLabel string,
// Wait for DPC update
return true, err
case 1:
// OK
if ports[0].InvalidConfig {
return false, fmt.Errorf("port %s has invalid config: %s", ports[0].Logicallabel,
ports[0].LastError)
}
// Selected port is OK
break
default:
err = fmt.Errorf("label of selected uplink matches multiple ports (%v)", ports)
return false, err
Expand Down
Loading

0 comments on commit d49a178

Please sign in to comment.