Skip to content

Commit 3940ff4

Browse files
lexfreilexfrei
committed
fix(charts,test): filter malformed CIDRs in addresses_by_link; pin stp/vlan independence
Two more correctness gaps from review: 1. addresses_by_link propagated corrupt entries verbatim into LinkConfig / VLANConfig / BridgeConfig `addresses` blocks. link_name_for_address was deliberately lenient on cidrContains parse failures so a single bad COSI entry could not crash the render, but the same lenience did not extend to the address-emission path: a "definitely-not-a-cidr" entry would surface verbatim into the rendered YAML and fail on apply with a less-informative error. Filter on cidrPrefixLen >= 0 — symmetric to the same parse helper link_name_for_address uses — so malformed entries are dropped at the same boundary. Also hoist the scope skip-list out of the iteration body to match link_name_for_address. Pinned by TestContract_NetworkMultidoc_LinkAddressesFilterMalformedCidr: the malformedAddressEntryLookup fixture's "definitely-not-a-cidr" entry must NOT reach any `- address:` line of the rendered output, and the valid sibling 192.168.100.4/24 MUST still be present. 2. BridgeConfig.stp / BridgeConfig.vlan independence was only pinned for the both-set case; a refactor that accidentally nested one inside the other (e.g. de-duplicating the wrapping `if` checks) would silently regress one path. Add two single-block fixtures (bridgeWithVLANOnlyLookup, bridgeWithSTPOnlyLookup) sharing a common bridgeOnlyLookup helper, plus contract tests asserting each sub-block emits in isolation AND the absent sub-block does NOT emit. Signed-off-by: Aleksei Sviridkin <f@lex.la>
1 parent 409d391 commit 3940ff4

3 files changed

Lines changed: 188 additions & 3 deletions

File tree

‎charts/talm/templates/_helpers.tpl‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -506,11 +506,26 @@ vlans:
506506
{{- define "talm.discovered.addresses_by_link" -}}
507507
{{- $linkName := . -}}
508508
{{- $addresses := list -}}
509+
{{- /* Hoist the scope skip-list out of the range body so it is
510+
built once per call rather than once per address-table
511+
entry. Symmetric to the hoisted skip-list inside
512+
link_name_for_address. */ -}}
513+
{{- $skipScopes := list "host" "link" "nowhere" -}}
509514
{{- range (lookup "addresses" "" "").items -}}
510515
{{- $hasScope := and .spec.scope (ne (.spec.scope | toString) "") -}}
511-
{{- $skip := has (.spec.scope | toString) (list "host" "link" "nowhere") -}}
512-
{{- if and (eq .spec.linkName $linkName) $hasScope (not $skip) -}}
513-
{{- $addresses = append $addresses .spec.address -}}
516+
{{- $skip := has (.spec.scope | toString) $skipScopes -}}
517+
{{- /* Filter out corrupt or future-format entries whose address
518+
does not parse as a CIDR. cidrPrefixLen returns -1 on parse
519+
failure, which we treat as "skip" the same way
520+
link_name_for_address does — a single bad entry in COSI
521+
must not propagate into LinkConfig / VLANConfig / BridgeConfig
522+
addresses where it would produce a config Talos rejects on
523+
apply with a less-informative error than the chart could
524+
give. */ -}}
525+
{{- $address := .spec.address | toString -}}
526+
{{- $validCidr := ge (int (cidrPrefixLen $address)) 0 -}}
527+
{{- if and (eq .spec.linkName $linkName) $hasScope (not $skip) $validCidr -}}
528+
{{- $addresses = append $addresses $address -}}
514529
{{- end -}}
515530
{{- end -}}
516531
{{- toJson $addresses -}}

‎pkg/engine/contract_network_multidoc_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,63 @@ func TestContract_NetworkMultidoc_VIPLinkLongestPrefixMatch(t *testing.T) {
572572
}
573573
}
574574

575+
// Contract: BridgeConfig emits the vlan sub-block even when
576+
// spec.bridgeMaster carries no stp setting. Pins the independence
577+
// of the two BridgeConfig sub-blocks (stp / vlan) against a future
578+
// refactor that accidentally nests one inside the other or
579+
// conditions one on the other.
580+
func TestContract_NetworkMultidoc_BridgeConfig_VLANOnlyNoStp(t *testing.T) {
581+
out := renderCozystackWith(t, bridgeWithVLANOnlyLookup(), map[string]any{
582+
"advertisedSubnets": []any{"10.5.0.0/24"},
583+
})
584+
assertContains(t, out, "kind: BridgeConfig")
585+
assertContains(t, out, "vlan:")
586+
assertContains(t, out, "filtering: true")
587+
if strings.Contains(out, "stp:") {
588+
t.Errorf("BridgeConfig emits stp: block when spec.bridgeMaster.stp is unset; sub-blocks must be independent:\n%s", out)
589+
}
590+
}
591+
592+
// Contract: BridgeConfig emits the stp sub-block even when
593+
// spec.bridgeMaster carries no vlan setting. Mirror of the
594+
// VLAN-only contract above.
595+
func TestContract_NetworkMultidoc_BridgeConfig_StpOnlyNoVlan(t *testing.T) {
596+
out := renderCozystackWith(t, bridgeWithSTPOnlyLookup(), map[string]any{
597+
"advertisedSubnets": []any{"10.5.0.0/24"},
598+
})
599+
assertContains(t, out, "kind: BridgeConfig")
600+
assertContains(t, out, "stp:")
601+
assertContains(t, out, "enabled: true")
602+
if strings.Contains(out, "vlan:") {
603+
t.Errorf("BridgeConfig emits vlan: block when spec.bridgeMaster.vlan is unset; sub-blocks must be independent:\n%s", out)
604+
}
605+
}
606+
607+
// Contract: malformed entries in COSI's addresses table do not
608+
// propagate into the rendered LinkConfig / VLANConfig / BridgeConfig
609+
// `addresses` blocks. The chart's addresses_by_link helper filters
610+
// out entries whose `.spec.address` fails to parse as a CIDR
611+
// (cidrPrefixLen returns -1), so a corrupt or future-format entry
612+
// stays inside discovery and never reaches a typed document Talos
613+
// would reject on apply.
614+
//
615+
// Fixture: malformedAddressEntryLookup carries
616+
// "definitely-not-a-cidr" on enp0s31f6.4000 sandwiched between two
617+
// well-formed entries. The test asserts the bad value is absent
618+
// from any `- address:` line and the valid sibling
619+
// 192.168.100.4/24 IS present.
620+
func TestContract_NetworkMultidoc_LinkAddressesFilterMalformedCidr(t *testing.T) {
621+
out := renderCozystackWith(t, malformedAddressEntryLookup(), map[string]any{
622+
"advertisedSubnets": []any{"192.168.100.0/24"},
623+
})
624+
if strings.Contains(out, "definitely-not-a-cidr") {
625+
t.Errorf("malformed CIDR leaked into LinkConfig/VLANConfig.addresses; corrupt COSI entries must be filtered at addresses_by_link:\n%s", out)
626+
}
627+
if !strings.Contains(out, "- address: 192.168.100.4/24") {
628+
t.Errorf("valid sibling CIDR 192.168.100.4/24 missing from VLANConfig.addresses; filter must not drop well-formed entries:\n%s", out)
629+
}
630+
}
631+
575632
// Contract: a malformed address entry in COSI's addresses table does
576633
// not crash the chart render. cidrContains is lenient on parse
577634
// failures (returns false), so the helper skips the bad entry and

‎pkg/engine/render_test.go‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4833,6 +4833,119 @@ func bridgeWithClusterSubnetLookup() func(string, string, string) (map[string]an
48334833
}
48344834
}
48354835

4836+
// bridgeWithVLANOnlyLookup is a slim BridgeConfig fixture where
4837+
// spec.bridgeMaster carries ONLY a vlan.filteringEnabled setting
4838+
// (no stp). The contract under test: the chart emits the
4839+
// BridgeConfig.vlan block even when stp is unset, and does NOT
4840+
// emit a stp block at all. Together with bridgeWithSTPOnlyLookup
4841+
// this pins the independence of the two sub-blocks against a
4842+
// future refactor that accidentally nests one inside the other.
4843+
func bridgeWithVLANOnlyLookup() func(string, string, string) (map[string]any, error) {
4844+
br0 := map[string]any{
4845+
"metadata": map[string]any{"id": "br0"},
4846+
"spec": map[string]any{
4847+
"kind": "bridge",
4848+
"index": 1,
4849+
"bridgeMaster": map[string]any{
4850+
"vlan": map[string]any{"filteringEnabled": true},
4851+
},
4852+
},
4853+
}
4854+
4855+
return bridgeOnlyLookup(br0)
4856+
}
4857+
4858+
// bridgeWithSTPOnlyLookup is the stp-only counterpart of
4859+
// bridgeWithVLANOnlyLookup. Pins that the BridgeConfig.stp block
4860+
// emits even when vlan is unset.
4861+
func bridgeWithSTPOnlyLookup() func(string, string, string) (map[string]any, error) {
4862+
br0 := map[string]any{
4863+
"metadata": map[string]any{"id": "br0"},
4864+
"spec": map[string]any{
4865+
"kind": "bridge",
4866+
"index": 1,
4867+
"bridgeMaster": map[string]any{
4868+
"stp": map[string]any{"enabled": true},
4869+
},
4870+
},
4871+
}
4872+
4873+
return bridgeOnlyLookup(br0)
4874+
}
4875+
4876+
// bridgeOnlyLookup builds a single-bridge lookup fixture given a
4877+
// pre-shaped br0 link map. The bridge owns the IPv4 default route
4878+
// at 10.5.0.1 via address 10.5.0.10/24. No physical NIC, no
4879+
// slaves — keeps the fixture tight around the BridgeConfig
4880+
// sub-block contracts the callers exercise.
4881+
func bridgeOnlyLookup(br0 map[string]any) func(string, string, string) (map[string]any, error) {
4882+
routesList := map[string]any{
4883+
"apiVersion": "v1",
4884+
"kind": "List",
4885+
"items": []any{
4886+
map[string]any{
4887+
"spec": map[string]any{
4888+
"dst": "",
4889+
"gateway": "10.5.0.1",
4890+
"outLinkName": "br0",
4891+
"family": "inet4",
4892+
"table": "main",
4893+
},
4894+
},
4895+
},
4896+
}
4897+
linksList := map[string]any{
4898+
"apiVersion": "v1",
4899+
"kind": "List",
4900+
"items": []any{br0},
4901+
}
4902+
addressesList := map[string]any{
4903+
"apiVersion": "v1",
4904+
"kind": "List",
4905+
"items": []any{
4906+
map[string]any{"spec": map[string]any{"linkName": "br0", "address": "10.5.0.10/24", "family": "inet4", "scope": "global"}},
4907+
},
4908+
}
4909+
nodeDefault := map[string]any{
4910+
"spec": map[string]any{
4911+
"addresses": []any{"10.5.0.10/24"},
4912+
},
4913+
}
4914+
resolvers := map[string]any{
4915+
"spec": map[string]any{
4916+
"dnsServers": []any{"8.8.8.8"},
4917+
},
4918+
}
4919+
4920+
return func(resource, _, id string) (map[string]any, error) {
4921+
switch resource {
4922+
case "routes":
4923+
return routesList, nil
4924+
case "links":
4925+
switch id {
4926+
case "br0":
4927+
return br0, nil
4928+
case "":
4929+
return linksList, nil
4930+
}
4931+
4932+
return map[string]any{}, nil
4933+
case "addresses":
4934+
return addressesList, nil
4935+
case "nodeaddress":
4936+
if id == "default" {
4937+
return nodeDefault, nil
4938+
}
4939+
case "resolvers":
4940+
if id == "resolvers" {
4941+
return resolvers, nil
4942+
}
4943+
}
4944+
4945+
return map[string]any{}, nil
4946+
}
4947+
}
4948+
48364949
// defaultRouteOnNonConfigurableLinkLookup pins the contract that
48374950
// the default-route-link fallback in the discovery-derived
48384951
// Layer2VIPConfig path must also pass the configurable-link gate.

0 commit comments

Comments
 (0)