Skip to content

fix(processor): honour json struct tags when unmarshalling YAML - #247

Open
Git-PrinceNagar wants to merge 2 commits into
mainfrom
fix-4225-yaml-unmarshal
Open

fix(processor): honour json struct tags when unmarshalling YAML#247
Git-PrinceNagar wants to merge 2 commits into
mainfrom
fix-4225-yaml-unmarshal

Conversation

@Git-PrinceNagar

@Git-PrinceNagar Git-PrinceNagar commented Jul 30, 2026

Copy link
Copy Markdown

Overview/Summary

Custom role definitions authored in YAML are not loaded by alzlib. All fields are
silently discarded during unmarshalling, and the failure surfaces later as a validation
error that does not indicate the underlying cause:

no name provided
Failed to initialize AlzLib

An equivalent role definition authored in JSON loads correctly, so YAML support is
non-functional in practice despite being documented in role-definitions.md:

"Role definitions can be defined in either JSON or YAML format."

Root cause

assets.RoleDefinition anonymously embeds armauthorization.RoleDefinition, which
declares only json:"..." struct tags and implements a custom json.Unmarshaler. It
carries no yaml tags.

encoding/json promotes the fields of an anonymously embedded struct automatically;
gopkg.in/yaml.v3 does not, unless the field is tagged yaml:",inline", and it neither
honours json tags nor invokes json.Unmarshaler. As a result, yaml.Unmarshal could not
map any field of the document onto the destination type. The properties block was never
applied and every field retained its zero value. Validation subsequently observed an empty
roleName and returned ErrNoNameProvided — accurate, but reported far from the point of
failure.

Fixes Azure/Azure-Landing-Zones#4225

This PR fixes/adds/changes/removes

  1. Reimplements unmarshalYAML to route YAML through the JSON decoder. YAML is decoded
    into an untyped any, re-marshalled to JSON, and passed to the existing unmarshalJSON
    implementation. Both formats now converge on a single, already-tested decode path, which
    prevents them from diverging again.
  2. Adds normalizeYAMLToJSON. yaml.v3 decodes a mapping into map[string]any only
    when every key is a string; otherwise it yields map[any]any, which encoding/json
    cannot marshal. This helper recursively converts such keys to strings via fmt.Sprint,
    mirroring JSON semantics, where object keys are always strings.
  3. Adds contextual wrapping to errors returned from the YAML path, so failures identify
    the stage at which they occurred.
  4. Adds six tests to internal/processor/unmarshaler_test.go, covering the reported
    role definition failure, YAML/JSON parity, non-string keys, and regression cover for
    policy definitions, policy set definitions and policy assignments.

Breaking Changes

unmarshalYAML is shared by all asset processors, so the blast radius of this change was
treated as the primary risk and assessed accordingly.

  1. Custom UnmarshalYAML implementations are no longer invoked. Three internal types
    (libArchetype, libArchetypeOverride, libArchitecture) implement yaml.Unmarshaler
    and become unreachable now that YAML is routed through JSON. Their behaviour is preserved
    by the JSON path, as evidenced by the existing test suite passing unchanged. They have
    been left in place to keep this change set minimal, and can be removed in a follow-up if
    maintainers prefer.
  2. No other behavioural changes. Documents that previously parsed correctly continue to
    parse identically. The change is additive in effect: YAML documents that previously
    produced empty structs are now populated correctly.

Testing Evidence

Build, tests, vet and lint

Check Before the change After the change
New tests covering the defect Fail, reproducing the reported behaviour Pass
go build ./... Pass Pass
go vet ./... Clean Clean
go test ./... (all packages) Pass Pass
golangci-lint run 78 pre-existing findings 78 findings, identical set; no new findings

Lint output was compared against a baseline captured on the unmodified tree to confirm that
this change introduces no new findings. The findings are pre-existing and repo-wide:
go-lint.yml sets version: latest, and golangci-lint v2.12 extended goconst to
_test.go files, so they also appear on unmodified main.

End-to-end verification against an ALZ Accelerator deployment

Because the defect manifests during library initialisation rather than in isolation, it was
also verified at deployment level. Three runs were performed against the same Accelerator
configuration, varying only the role definition file format and the library build:

Run Role definition format Library Result
1 (control) JSON Unpatched Succeeded — Plan: 1213 to add
2 (reproduction) YAML, equivalent content Unpatched Failed — no name provided, Failed to initialize AlzLib
3 (verification) YAML, equivalent content Patched Succeeded — Plan: 1213 to add

Runs 1 and 3 produce an identical Terraform plan, establishing that the change restores
parity between the two formats rather than suppressing the error.

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Associated it with the relevant issue, for tracking and closure
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence — unit tests, go vet, golangci-lint, and an end-to-end Accelerator deployment
  • Updated relevant and associated documentation — no documentation change is required, as this change aligns the implementation with the documented behaviour

YAML-authored assets were decoded straight into Azure SDK types that only
declare `json` struct tags. gopkg.in/yaml.v3 does not promote anonymously
embedded struct fields or honour json tags, so documents such as custom role
definitions parsed into empty structs and failed later with a confusing
"no name provided" validation error.

Route YAML through JSON so both formats converge on the same decoder, and
normalize map[any]any keys to strings so non-string YAML keys remain
marshalable.

Fixes Azure/Azure-Landing-Zones#4225

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@codecov-commenter

codecov-commenter commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.75%. Comparing base (5f18137) to head (1b5456b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   47.87%   47.75%   -0.13%     
==========================================
  Files          54       54              
  Lines        4913     4938      +25     
==========================================
+ Hits         2352     2358       +6     
- Misses       2273     2294      +21     
+ Partials      288      286       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add tests for the two error branches introduced in unmarshalYAML: an
invalid YAML document, and a document that is valid YAML but has no JSON
representation (NaN), which fails at json.Marshal.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@Git-PrinceNagar

Copy link
Copy Markdown
Author

Lint code base — why it is failing

go-lint.yml sets version: latest for golangci-lint, so the version is resolved at run time. v2.12 extended goconst to _test.go files, surfacing 77 repeated-string findings that already existed in the repository. None originate from this PR.

Code under test golangci-lint goconst findings
origin/main @ 5f18137, unmodified v2.11.4 0
origin/main @ 5f18137, unmodified v2.12.2 77
this branch v2.11.4 0
this branch v2.12.2 77

#246 already pins the version to v2.11.4; once it merges this branch can be rebased. I have left the workflow untouched here to keep the change set limited to the fix.

@matt-FFFFFF

Copy link
Copy Markdown
Member

Hi, the library was designed to support YAML for the non-azure resources - e.g. archetypes, etc.

If support for yaml for azure resources is desired then it should be extended to all resource types. However this was a deliberate decision to not support this initially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: alzlib - YAML role definition files fail to parse due to yaml.v3 ignoring json struct tags

3 participants