Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flexibility to IMDS api-version #793

Merged
merged 24 commits into from
Mar 3, 2021

Conversation

trstringer
Copy link
Contributor

@trstringer trstringer commented Jan 27, 2021

Proposed Commit Message

Add flexibility to IMDS api-version by having both a desired IMDS api-version and a minimum api-version. The desired api-version will be used first, and if that fails it will fall back to the minimum api-version.

Additional Context

This PR will allow us to rollout features in cloud-init to take advantage of newer features in IMDS.

Test Steps

The following shell script was used to validate this end-to-end both using an existing desired IMDS api-version and then using an impossible (future) api-version, forcing a fallback to the minimum version. This script relies on helper functions from my az-cli-helpers repo.

#!/bin/bash -i

# First create a VM and install the updated cloud-init from my build. Ensure
# that we are able to utilize the "desired" IMDS api version 2020-09-01.
RESOURCE_NAME=$(resource_name)
echo "$(date) - Using resource name $RESOURCE_NAME"
az_vm_create_default
az_vm_deb_install "$RESOURCE_NAME" ~/dev/cloud-init/cloud-init_all.deb
OLD_RESOURCE_NAME=$RESOURCE_NAME
RESOURCE_NAME=$(resource_name)
az_vm_create_from_vm "$OLD_RESOURCE_NAME"

if az_vm_ssh "$RESOURCE_NAME" "grep 'Falling back to IMDS api-version' /var/log/cloud-init.log"; then
    echo "$(date) Failed: api-version not found"
    exit 1
else
    echo "$(date) api-version found successfully"
fi

if az_vm_ssh "$RESOURCE_NAME" "grep 'UrlError' /var/log/cloud-init.log"; then
    echo "$(date) Failed: found UrlError"
    exit 1
else
    echo "$(date) no UrlError in logs"
fi

# For the second test, modify the source code directly so that the desired
# IMDS api-version is impossible to be used (a future date). This test
# should verify that we fall back to the minimum api-version for IMDS.
echo "$(date) Modifying to impossible IMDS version for wanted"
az_vm_ssh "$RESOURCE_NAME" "sudo sed -i 's/2020-09-01/2022-09-01/g' /usr/lib/python3/dist-packages/cloudinit/sources/DataSourceAzure.py"
az_vm_ssh "$RESOURCE_NAME" "sudo cloud-init clean --logs"
OLD_RESOURCE_NAME=$RESOURCE_NAME
RESOURCE_NAME=$(resource_name)
az_vm_create_from_vm "$OLD_RESOURCE_NAME"

if ! az_vm_ssh "$RESOURCE_NAME" "grep 'Falling back to IMDS api-version' /var/log/cloud-init.log"; then
    echo "$(date) Failed: no fall back for IMDS version"
    exit 1
else
    echo "$(date) Success for fall back IMDS version"
fi

echo "$(date) - All tests succeded"

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@github-actions
Copy link

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

@github-actions github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Feb 11, 2021
@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label Feb 11, 2021
@TheRealFalcon TheRealFalcon self-assigned this Feb 11, 2021
Copy link
Member

@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.

Overall looks good. Comments are inline.

cloudinit/sources/DataSourceAzure.py Show resolved Hide resolved
cloudinit/sources/DataSourceAzure.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceAzure.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceAzure.py Outdated Show resolved Hide resolved
cloudinit/sources/DataSourceAzure.py Outdated Show resolved Hide resolved
@trstringer
Copy link
Contributor Author

Thanks for the review, @TheRealFalcon! I've made those requested changes and retested with the original test script (had to modify log searches, but essentially it remained the same). Let me know if you see anything else you want changed!

Copy link
Member

@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.

Just one more inline comment, but otherwise looks good to me!

cloudinit/sources/DataSourceAzure.py Outdated Show resolved Hide resolved
Copy link
Member

@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.

This looks good. Thanks!

@TheRealFalcon TheRealFalcon merged commit 3be6663 into canonical:master Mar 3, 2021
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.

None yet

2 participants