Skip to content

Commit 29d6e39

Browse files
committed
fix(applycheck): correct YAML field names against real Talos schema
Walker was using YAML keys I guessed from Talos doc strings, not verified against the actual v1alpha1 struct tags. Two of them were wrong, silently turning the walker into a no-op for the affected document class: - BridgeConfig: tag is 'links' (BridgeLinks), I had 'ports'. - VLANConfig parent: tag is 'parent' (ParentLinkConfig), I had 'link'. Real-Talos consequence: BridgeConfig slaves were never validated; VLANConfig parent was never validated. The unit tests passed only because the fixture YAML mirrored my (wrong) guess. Also adds HCloudVIPConfig (hcloud_vip.go LinkName 'yaml:link') to the dispatch table; it has the same Layer2VIPConfig shape and shares the handler. New test TestWalkRefs_v1_12_RealTalosYAMLKeys pins each tag against its struct definition so a future tag rename in machinery (or another walker oversight) surfaces immediately. Verified on the dev17 OCI cluster: BridgeConfig with a typoed 'missing-port' in .links[] now blocks; VLANConfig with a typoed 'ghost-parent' in .parent now blocks. Pre-fix both went silent. Refs: #172 Signed-off-by: Aleksei Sviridkin <f@lex.la>
1 parent cb0ade2 commit 29d6e39

2 files changed

Lines changed: 112 additions & 12 deletions

File tree

pkg/applycheck/refs.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -172,11 +172,19 @@ var multidocHandlers = map[string]multidocHandler{
172172
// BridgeConfig/VLANConfig below, which intentionally do NOT validate
173173
// their own .name field — those names describe a virtual link being
174174
// created by the apply, not an existing one to reference.
175+
//
176+
// YAML field names mirror Talos's v1alpha1 schema verbatim:
177+
//
178+
// - bond.go BondLinks -> `links`
179+
// - bridge.go BridgeLinks -> `links` (NOT `ports`)
180+
// - vlan.go ParentLinkConfig -> `parent` (NOT `link`)
181+
// - layer2_vip.go LinkName -> `link`
175182
"LinkConfig": handleNameOnly,
176183
"BondConfig": handleListOnly("links"),
177184
"VLANConfig": handleParentOnly,
178-
"BridgeConfig": handleListOnly("ports"),
185+
"BridgeConfig": handleListOnly("links"),
179186
"Layer2VIPConfig": handleLayer2VIP,
187+
"HCloudVIPConfig": handleLayer2VIP,
180188
"UserVolumeConfig": handleUserVolume,
181189
}
182190

@@ -208,14 +216,16 @@ func handleListOnly(listKey string) multidocHandler {
208216

209217
// handleParentOnly emits only the parent reference of a VLAN doc, not
210218
// its own .name. The .name is the VLAN tag's child link name (a new
211-
// virtual link being created); the .link is the parent that must exist.
219+
// virtual link being created); the .parent is the parent that must
220+
// exist. The YAML key in v1alpha1 is `parent`, not `link`
221+
// (vlan.go ParentLinkConfig `yaml:"parent"`).
212222
func handleParentOnly(refs []Ref, doc map[string]any, basePath string) []Ref {
213-
parent, ok := doc["link"].(string)
223+
parent, ok := doc["parent"].(string)
214224
if !ok || parent == "" {
215225
return refs
216226
}
217227

218-
return append(refs, Ref{Kind: RefKindLink, Name: parent, Source: basePath + ".link"})
228+
return append(refs, Ref{Kind: RefKindLink, Name: parent, Source: basePath + ".parent"})
219229
}
220230

221231
func handleLayer2VIP(refs []Ref, doc map[string]any, basePath string) []Ref {

pkg/applycheck/refs_test.go

Lines changed: 98 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ addresses:
5757
apiVersion: v1alpha1
5858
kind: VLANConfig
5959
name: eth0.4000
60-
link: eth0
60+
parent: eth0
6161
vlanID: 4000
6262
---
6363
apiVersion: v1alpha1
@@ -70,7 +70,7 @@ links:
7070
apiVersion: v1alpha1
7171
kind: BridgeConfig
7272
name: br0
73-
ports:
73+
links:
7474
- eth3
7575
- bond0
7676
---
@@ -128,26 +128,26 @@ func TestWalkRefs_v1_12_ExtractsLinksFromMultidocAndDiskSelector(t *testing.T) {
128128
// Walker emits only references to *existing* links, not names of
129129
// virtual links being created by the apply. So:
130130
// - LinkConfig.name -> emitted (override of an existing physical NIC)
131-
// - VLANConfig.link -> emitted (the parent link must exist)
131+
// - VLANConfig.parent -> emitted (the parent link must exist)
132132
// - VLANConfig.name -> NOT emitted (the VLAN child is being created)
133133
// - BondConfig.links[] -> emitted (slave NICs must exist)
134134
// - BondConfig.name -> NOT emitted (the bond is being created)
135-
// - BridgeConfig.ports[] -> emitted (port NICs must exist)
135+
// - BridgeConfig.links[] -> emitted (port NICs must exist)
136136
// - BridgeConfig.name -> NOT emitted (the bridge is being created)
137-
wantLinks := []string{"eth0" /* LinkConfig.name + VLANConfig.link parent */, "eth1", "eth2" /* BondConfig.links */, "eth3" /* BridgeConfig.ports */}
137+
wantLinks := []string{"eth0" /* LinkConfig.name + VLANConfig.parent */, "eth1", "eth2" /* BondConfig.links */, "eth3" /* BridgeConfig.links */}
138138
for _, name := range wantLinks {
139139
if _, ok := findRef(refs, applycheck.RefKindLink, name); !ok {
140140
t.Errorf("expected link ref for %q, got refs=%+v", name, refs)
141141
}
142142
}
143143

144144
// Names of virtual links being created must NOT surface (bond0 is
145-
// a BondConfig.name + a BridgeConfig.ports[] entry; the .name path
146-
// should not appear, only the .ports[] one. eth0.4000 is a
145+
// a BondConfig.name + a BridgeConfig.links[] entry; the .name path
146+
// should not appear, only the .links[] one. eth0.4000 is a
147147
// VLANConfig.name with no other references — should not appear at
148148
// all).
149149
if _, ok := findRef(refs, applycheck.RefKindLink, "eth0.4000"); ok {
150-
t.Errorf("VLANConfig.name (eth0.4000) leaked as a ref; only the .link parent should validate")
150+
t.Errorf("VLANConfig.name (eth0.4000) leaked as a ref; only the .parent should validate")
151151
}
152152

153153
// Layer2VIPConfig.link is a distinct ref site. Use a name that no other
@@ -197,6 +197,96 @@ func TestWalkRefs_EmptyInput_NoRefsNoError(t *testing.T) {
197197
}
198198
}
199199

200+
// TestWalkRefs_v1_12_RealTalosYAMLKeys pins the exact YAML field
201+
// names Talos's v1alpha1 schema uses (verified against
202+
// pkg/machinery/config/types/network/* in the cozystack/talos fork
203+
// v0.0.0-20260126122716):
204+
//
205+
// - bond.go BondLinks `yaml:"links"`
206+
// - bridge.go BridgeLinks `yaml:"links"` (NOT `ports`)
207+
// - vlan.go ParentLinkConfig `yaml:"parent"` (NOT `link`)
208+
// - layer2_vip.go LinkName `yaml:"link"`
209+
// - hcloud_vip.go LinkName `yaml:"link"`
210+
//
211+
// Without this contract a renamed YAML key in machinery would
212+
// silently turn the walker into a no-op for the affected document
213+
// class and Phase 1 would stop catching typos in those refs. This
214+
// case was caught only by exercising a real cluster.
215+
func TestWalkRefs_v1_12_RealTalosYAMLKeys(t *testing.T) {
216+
t.Parallel()
217+
218+
body := `apiVersion: v1alpha1
219+
kind: VLANConfig
220+
name: ens5.42
221+
parent: ens5
222+
vlanID: 42
223+
---
224+
apiVersion: v1alpha1
225+
kind: BridgeConfig
226+
name: br0
227+
links:
228+
- ens6
229+
- ens7
230+
---
231+
apiVersion: v1alpha1
232+
kind: BondConfig
233+
name: bond0
234+
links:
235+
- ens8
236+
- ens9
237+
---
238+
apiVersion: v1alpha1
239+
kind: Layer2VIPConfig
240+
name: 10.0.0.1
241+
link: bond0
242+
---
243+
apiVersion: v1alpha1
244+
kind: HCloudVIPConfig
245+
name: 10.0.0.2
246+
link: bond0
247+
`
248+
refs, err := applycheck.WalkRefs([]byte(body))
249+
if err != nil {
250+
t.Fatalf("WalkRefs: %v", err)
251+
}
252+
253+
want := []struct {
254+
name string
255+
source string // suffix
256+
}{
257+
{"ens5", ".parent"}, // VLANConfig.parent (NOT .link)
258+
{"ens6", ".links[0]"}, // BridgeConfig.links (NOT .ports)
259+
{"ens7", ".links[1]"},
260+
{"ens8", ".links[0]"}, // BondConfig.links
261+
{"ens9", ".links[1]"},
262+
{"bond0", ".link"}, // Layer2VIPConfig.link + HCloudVIPConfig.link
263+
}
264+
265+
for _, w := range want {
266+
var found bool
267+
268+
for _, r := range refs {
269+
if r.Kind == applycheck.RefKindLink && r.Name == w.name && strings.HasSuffix(r.Source, w.source) {
270+
found = true
271+
272+
break
273+
}
274+
}
275+
276+
if !found {
277+
t.Errorf("expected link ref %q with source suffix %q, got refs=%+v", w.name, w.source, refs)
278+
}
279+
}
280+
281+
// Virtual-link names (the doc's own .name on VLAN/Bond/Bridge)
282+
// must NOT surface as refs.
283+
for _, fake := range []string{"ens5.42", "br0", "bond0"} {
284+
if _, ok := findRef(refs, applycheck.RefKindLink, fake); ok && fake != "bond0" {
285+
t.Errorf("name %q leaked as a ref; only existing-link refs should be emitted", fake)
286+
}
287+
}
288+
}
289+
200290
// TestWalkRefs_AccidentalEncodings_Tolerated pins three real-world
201291
// shapes that come out of editors and unzipping operations: a UTF-8
202292
// BOM at the head of the file (common Windows artifact), CRLF line

0 commit comments

Comments
 (0)