From ee75c773dc87f342dfd94209eadfb6bf921d3e03 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Wed, 17 Jun 2026 22:50:28 -0700 Subject: [PATCH] Fix metadata grain KeyError on http.query error responses Since 3006.3 salt.utils.http.query (tornado backend) returns ``body`` on HTTPError but does not populate ``headers``. salt.grains.metadata._search() indexed ``linedata["headers"]`` unconditionally, so any recursive lookup that hit a 4xx/5xx (e.g. IMDS rejecting a sub-path produced by the legacy ``=``-splitter on an EC2 user-data line) crashed the whole grain load with:: [CRITICAL] Failed to load grains defined in grain file metadata.metadata ... KeyError: 'headers' Guard the headers indexing the same way ``body`` is already guarded: when ``headers`` is absent, treat it as "no Content-Type information" and fall through to the existing parsing path. Add three pytest regressions covering the missing-headers case (both on the initial query and on recursion) and a sanity guard for the existing ``application/octet-stream`` short-circuit. Fixes #65184 --- changelog/65184.fixed.md | 1 + salt/grains/metadata.py | 10 +- tests/pytests/unit/grains/test_metadata.py | 109 +++++++++++++++++++++ 3 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 changelog/65184.fixed.md diff --git a/changelog/65184.fixed.md b/changelog/65184.fixed.md new file mode 100644 index 000000000000..e0ff0d9c528b --- /dev/null +++ b/changelog/65184.fixed.md @@ -0,0 +1 @@ +Fixed the EC2/cloud metadata grain crashing with ``KeyError: 'headers'`` when ``salt.utils.http.query`` returns an error response (4xx/5xx with a body, e.g. when the IMDS rejects a recursive sub-path lookup). Since 3006.3 the tornado backend has populated ``body`` on HTTPError without also populating ``headers``; the grain now treats the missing ``headers`` key as "no Content-Type information" instead of letting the lookup blow up the whole grain load. diff --git a/salt/grains/metadata.py b/salt/grains/metadata.py index edd18e94409c..bd7798a023f1 100644 --- a/salt/grains/metadata.py +++ b/salt/grains/metadata.py @@ -49,10 +49,12 @@ def _search(prefix="latest/"): if "body" not in linedata: return ret body = salt.utils.stringutils.to_unicode(linedata["body"]) - if ( - linedata["headers"].get("Content-Type", "text/plain") - == "application/octet-stream" - ): + # Since 3006.3, salt.utils.http.query (tornado backend) returns ``body`` + # on HTTPError but does not populate ``headers``. Treat a missing + # ``headers`` key as "no Content-Type information" rather than letting + # KeyError propagate and break the whole grain load (#65184). + response_headers = linedata.get("headers") or {} + if response_headers.get("Content-Type", "text/plain") == "application/octet-stream": return body for line in body.split("\n"): if line.endswith("/"): diff --git a/tests/pytests/unit/grains/test_metadata.py b/tests/pytests/unit/grains/test_metadata.py index 9b6ea99f8652..1bba86770f78 100644 --- a/tests/pytests/unit/grains/test_metadata.py +++ b/tests/pytests/unit/grains/test_metadata.py @@ -5,6 +5,18 @@ instead of falling through to the ``=`` line-splitter, which previously corrupted any user-data payload containing ``=`` characters (e.g. cloud-init ``#cloud-config`` blocks with ``key=value`` lines). + +Regression coverage for #65184: when ``salt.utils.http.query`` returns an +error response (4xx/5xx with a body, e.g. AWS IMDS returning HTTP 400 for a +bogus path produced by the legacy ``=``-splitter), the tornado backend +populates ``body`` and ``status`` but does NOT set ``headers``. +``salt.grains.metadata._search()`` previously indexed ``linedata["headers"]`` +unconditionally and crashed with ``KeyError: 'headers'``, causing the entire +metadata grain to fail to load with:: + + [CRITICAL] Failed to load grains defined in grain file metadata.metadata + ... + KeyError: 'headers' """ import logging @@ -176,3 +188,100 @@ def test_equals_lines_other_than_user_data_still_parse_via_splitter(): sc = result["meta-data"]["iam"]["security-credentials"] assert "role-arn-suffix" in sc, sc assert "myrole-user-data=role-arn-suffix" not in sc, sc + + +def test_search_handles_error_response_without_headers_65184(): + """ + Regression for #65184: a recursive ``http.query`` call that returns an + error-shaped response (``body`` present, ``headers`` absent — the shape + produced by the tornado backend on HTTPError since 3006.3) must not + crash ``_search()`` with ``KeyError: 'headers'``. + + The reporter's traceback shows the crash happens on the recursive call + triggered by a top-level metadata listing entry (the ``prefix == "latest/"`` + branch), where the recursive ``_search`` then calls ``http.query`` for + ``latest/dynamic/`` (or similar) and gets back an error response without a + ``headers`` key. Before the fix the indexing ``linedata["headers"]`` raised. + After the fix the missing-headers case is treated like "no Content-Type + information" and parsing proceeds. + """ + responses = { + "http://169.254.169.254/latest/": { + "body": "dynamic", + "headers": {"Content-Type": "text/plain"}, + }, + # Recursive call: error-shape response. Body + status + error, NO + # headers key. This is exactly what salt.utils.http.query returns on + # tornado HTTPError since commit 43b7fb52842 (3006.3). + "http://169.254.169.254/latest/dynamic/": { + "body": "

400 Bad request

\n", + "status": 400, + "error": "HTTP 400: Bad request", + }, + } + + with patch( + "salt.utils.http.query", + create_autospec( + http.query, autospec=True, side_effect=_make_mock_http(responses) + ), + ): + # Must not raise KeyError. Whatever it returns for the bad leaf is + # secondary; the contract is "do not crash the whole grain load". + result = metadata.metadata() + + assert isinstance(result, dict) + assert "dynamic" in result + + +def test_search_handles_missing_headers_on_initial_query_65184(): + """ + Companion to the above: the very first call inside ``_search()`` can also + produce a no-headers response (e.g. the metadata service returns 4xx for + the top-level listing). The function must still return a dict instead of + raising. + """ + responses = { + "http://169.254.169.254/latest/": { + "body": "some-error-body", + "status": 400, + "error": "HTTP 400: Bad request", + }, + } + + with patch( + "salt.utils.http.query", + create_autospec( + http.query, autospec=True, side_effect=_make_mock_http(responses) + ), + ): + result = metadata.metadata() + + # Either an empty dict or a parsed body is acceptable; the contract is + # "no KeyError". + assert isinstance(result, (dict, str)) + + +def test_search_octet_stream_still_returns_body_verbatim(): + """ + Sanity guard: the existing ``application/octet-stream`` short-circuit + (return body verbatim) must keep working. The fix for #65184 must not + regress that path. + """ + responses = { + "http://169.254.169.254/latest/": { + "body": "raw-octet-stream-payload", + "headers": {"Content-Type": "application/octet-stream"}, + }, + } + + with patch( + "salt.utils.http.query", + create_autospec( + http.query, autospec=True, side_effect=_make_mock_http(responses) + ), + ): + result = metadata.metadata() + + # Body returned verbatim, not wrapped in a dict. + assert result == "raw-octet-stream-payload"