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

Libcloud 544 gce metadata fix #349

Closed
wants to merge 6 commits into from

Conversation

@raphtheb
Copy link
Contributor

raphtheb commented Aug 15, 2014

This is a slightly improved version of the fix i submitted minutes ago which had some trace-generating code in it. Removed my own silly tests.

rtheberge added 2 commits Aug 15, 2014
…on of the bugfix. We now perform a simple sanity check to know whether the metadata dictionary has already been formatted to GCE's specs.
for k, v in ex_metadata.items()]}
if not ex_metadata or not isinstance(ex_metadata, dict):
# Should output a relevant error message if non-dict is received?
ex_metadata = None

This comment has been minimized.

@Kami

Kami Aug 15, 2014 Member

We should throw ValueError if user passes in an invalid value (not a dictionary) instead of silently ignoring it.

items = []
for k, v in ex_metadata.items():
items.append({'key': k, 'value': v})
if 'items' not in ex_metadata:

This comment has been minimized.

@Kami

Kami Aug 15, 2014 Member

I think we need a better solution of handling this - we should probably throw if ex_metadata dictionary already contains a key named items instead of just overwriting this key.

This comment has been minimized.

@Kami

Kami Aug 15, 2014 Member

Actually, why can't we do something like this:

metadata = {'items': []}
for key, value in ex_metadata.items():
    metadata['items'].append({'key': key, 'value': value})

This way, ex_metadata argument can still contain key named items...

This comment has been minimized.

@raphtheb

raphtheb Aug 15, 2014 Author Contributor

I've already added the ValueError, that's good and i have it staged locally.

I do believe we can't do the aforementioned code as that would re-introduce the issue i'm attempting to fix.This is the output of that code that gets sent to GCE while running it:

{'items': [{'value': [{'value': '1', 'key': 'items'}, {'value': 'two', 'key': '2'}, {'value': 'testbox', 'key': 'salt-cloud-profile'}], 'key': 'items'}]}

This is what salt sends to libcloud:

{'items': [{'value': '1', 'key': 'items'}, {'value': 'two', 'key': '2'}, {'value': 'testbox', 'key': 'salt-cloud-profile'}]}

I'll have a closer, fresher look on Monday and find a better way to handle this.

Cheers and have a great weekend!

rtheberge added 4 commits Aug 18, 2014
metadata field is provided.
rtheberge
 1- Raise error correctly and separeta the "None" check from the malformed check
 2- Provide comments explaining the odd GCE dictionary format
 3- Perform check on metadata dictionary structure. We expect one "items" key
    and a tuple of arbitrary values.
 4- Prefix the key/value provided by a simple dictionary by "items", if not found,
    enforcing point 3's structure.
@erjohnso
Copy link
Member

erjohnso commented Aug 28, 2014

@raphtheb - thanks for this. I've just tested it with SaltStack and this seems to work just fine. I'm guessing this is good to merge, but if I know @Kami, he'll ask you to squash your commits before merging. :)

@raphtheb
Copy link
Contributor Author

raphtheb commented Aug 29, 2014

Thanks for havign a look, @erjohnso

For clarity, i am opening a new pull-request (#353) with only one clean, squashed commit from an alternate branch. The end diff is the same, but will allow for a cleaner history. This one can safely be ignored/destroyed/rejected.

@raphtheb raphtheb closed this Aug 29, 2014
@raphtheb raphtheb deleted the raphtheb:LIBCLOUD-544_gce_metadata_fix branch Aug 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.