Skip to content

sources/azure: validate IMDS network configuration metadata#1257

Merged
TheRealFalcon merged 1 commit into
canonical:mainfrom
cjp256:azure-log-missing-metadata
Feb 14, 2022
Merged

sources/azure: validate IMDS network configuration metadata#1257
TheRealFalcon merged 1 commit into
canonical:mainfrom
cjp256:azure-log-missing-metadata

Conversation

@cjp256
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 commented Feb 10, 2022

Due to race conditions and caching, IMDS may return stale or incomplete
metadata. Add some validation to detect these scenarios and report
appropriate telemetry.

Signed-off-by: Chris Patterson cpatterson@microsoft.com

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from 378e256 to c34f236 Compare February 10, 2022 18:49


def normalize_mac_address(mac: str):
"""Normalize mac address with colons and lower-case."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What "non-colon" formats is this expected to normalize? Just curious since I don't see any new tests in this PR that appear to make use of this. This would obviously fail the format 'HHHH.HHHH.HHHH' that some network gear vendors use, but I assume that's out of scope for the inputs that this is supposed to normalize.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added tests yet, this was just a draft for early review :D

I expect get_interfaces() will always be in this lower-case, colon format - but I figured I'd pass it through regardless.

IMDS metadata uses upper-case w/o colons, so it needs the normalization for comparison.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this method here too:

mac = ":".join(re.findall(r"..", intf["macAddress"]))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks!

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from bd6a3d8 to 0ebae8c Compare February 11, 2022 19:56
@cjp256 cjp256 marked this pull request as ready for review February 11, 2022 19:57
@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch from 0ebae8c to 984d5b4 Compare February 11, 2022 22:40
Copy link
Copy Markdown
Contributor

@anhvoms anhvoms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch 3 times, most recently from 443cb30 to eee1ee9 Compare February 13, 2022 22:54
Due to race conditions and caching, IMDS may return stale or incomplete
metadata.  Add some validation to detect these scenarios and report
appropriate telemetry.

Introduce normalize_mac_address() to allow for comparison of mac
addresses, replacing that found inline in:
_generate_network_config_from_imds_metadata()

Add validation of final fetch of IMDS metadata.

Signed-off-by: Chris Patterson <cpatterson@microsoft.com>
@cjp256 cjp256 force-pushed the azure-log-missing-metadata branch from eee1ee9 to f893801 Compare February 14, 2022 13:52
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TheRealFalcon TheRealFalcon merged commit 32fcbb5 into canonical:main Feb 14, 2022
@cjp256 cjp256 deleted the azure-log-missing-metadata branch April 5, 2022 19:25
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.

4 participants