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

Fix bug in azure plugin/update to recent metadata version. #1154

Merged
merged 11 commits into from
Mar 9, 2018

Conversation

kriszentner
Copy link
Contributor

Description

This fixes a bug discovered for the Azure plugin, and updates to the recent azure metadata api date/version.

Issues Resolved

This fixes a bug I discovered which will crash the azure plugin if it's missing metadata. I discovered, at least on an ND24rs instance I was on, that no network metadata is received.

Also updated to the recent Azure metadata API which gives additional metadata including the resource group name, availability set info, subscription id, and VM scale set info.

In Azure, receiving the network section of the JSON output is not guaranteed, and will crash this plugin in these cases.
New info includes resourceGroupName, availability zone info, subscriptionId, and VMSS info.
@kriszentner kriszentner requested a review from a team March 7, 2018 18:17
@tas50
Copy link
Contributor

tas50 commented Mar 7, 2018

@kriszentner Thanks for taking the time to refactor this all. I'd love to get this into Chef 13/14. Can you run chefstyle -a to cleanup the style issues and then sign off the commits for the DCO so we can merge it in?

Thanks,
Tim

@@ -22,7 +22,7 @@ module Mixin
module AzureMetadata

AZURE_METADATA_ADDR = "169.254.169.254" unless defined?(AZURE_METADATA_ADDR)
AZURE_METADATA_URL = "/metadata/instance?api-version=2017-04-02" unless defined?(AZURE_METADATA_URL)
AZURE_METADATA_URL = "/metadata/instance?api-version=2017-12-01" unless defined?(AZURE_METADATA_URL)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the ec2 metadata resource we have some magic that looks for the latest supported metadata from an array of known supported versions. Do you think we should maybe look at doing something like that here or are we guaranteed to always have 2017-12-01?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at the documentation and it seems that they support older versions as they age. I'll have to look into seeing if there's a per instance api version checker. Maybe I can bug someone in Azure. I found 12-01 isn't supported in UK so I backed it off to the one that's supported everywhere.

@tas50
Copy link
Contributor

tas50 commented Mar 7, 2018

@kriszentner There's a rspec failure if you check the Travis/Appveyor runs

Was missing passing metadata to the network metadata initialization function
@tas50
Copy link
Contributor

tas50 commented Mar 7, 2018

This looks good to me. We just need the DCO signoff on the commits so we can accept it. There's details here: https://github.com/chef/chef/blob/master/CONTRIBUTING.md#developer-certification-of-origin-dco

kriszentner and others added 6 commits March 7, 2018 14:30
In Azure, receiving the network section of the JSON output is not guaranteed, and will crash this plugin in these cases.

Signed-off-by: Kris Zentner <krisz@microsoft.com>
New info includes resourceGroupName, availability zone info, subscriptionId, and VMSS info.

Signed-off-by: Kris Zentner <krisz@microsoft.com>
Signed-off-by: Kris Zentner <krisz@microsoft.com>
Was missing passing metadata to the network metadata initialization function

Signed-off-by: Kris Zentner <krisz@microsoft.com>
Signed-off-by: Kris Zentner <krisz@microsoft.com>
@kriszentner
Copy link
Contributor Author

Thanks for helping me out with this. Learn something new every time when doing these contribs!

@tas50 tas50 merged commit a31a614 into chef:master Mar 9, 2018
@tas50
Copy link
Contributor

tas50 commented Mar 9, 2018

Thanks for working through it. This is a nice step in the right direction for how we handle this metadata.

thommay added a commit that referenced this pull request Mar 19, 2018
Fix bug in azure plugin/update to recent metadata version. [ backport #1154]
@lock
Copy link

lock bot commented Jan 25, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants