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 aliyun ecs driver: version mismatch error #800

Closed
wants to merge 3 commits into from
Closed

Fix aliyun ecs driver: version mismatch error #800

wants to merge 3 commits into from

Conversation

netgenius18
Copy link
Contributor

@netgenius18 netgenius18 commented Jun 6, 2016

Currently for Aliyun ECS, we encounter version mismatch error when we run example_aliyun_ecs.py


File "example_aliyun_ecs.py", line 28, in
sizes = ecs.list_sizes()
File "/usr/lib/python2.7/site-packages/apache_libcloud-1.0.0_rc2-py2.7.egg/libcloud/compute/drivers/ecs.py", line 540, in list_sizes
resp_body = self.connection.request(self.path, params).object
File "/usr/lib/python2.7/site-packages/apache_libcloud-1.0.0_rc2-py2.7.egg/libcloud/common/base.py", line 862, in request
response = responseCls(**kwargs)
File "/usr/lib/python2.7/site-packages/apache_libcloud-1.0.0_rc2-py2.7.egg/libcloud/common/base.py", line 179, in init
headers=self.headers)
libcloud.common.exceptions.BaseHTTPError: {'host_id': 'ecs.aliyuncs.com', 'message': 'The specified parameter "Action or Version" is not valid.', 'code': 'InvalidParameter', 'request_id': '8DE2CC6D-1FD5-4A71-8FFF-66253E0BA5CE'}


We find the aliyun.py code passed the wrong parameter

self.signer = signer_cls(access_key=self.user_id,
access_secret=self.key,
version)

Actually, parameter version should be ECS_API_VERSION, but not DEFAULT_SIGNATURE_VERSION

@Kami
Copy link
Member

Kami commented Jun 6, 2016

Thanks.

Looks good to me.

/cc @samsong8610

@samsong8610
Copy link
Contributor

samsong8610 commented Jun 6, 2016

@net613 SignedAliyunConnection is derived by ECSConnection and SLBConnection. They have different api version, so we should not use a constant ECS_API_VERSION here.
In the derived connection classes, there is a version property, assigned the correct api version for different services respectively. We should use this as the version parameter for the signer.
It's better to add some defensive code to check self.version is not None before passing into the signer init method.

@netgenius18
Copy link
Contributor Author

@samsong8610 , I have finally fixed this bug according to your suggestion. Pls check it.

@samsong8610
Copy link
Contributor

LGTM, and I've verified the PR.
Thanks @net613

@netgenius18
Copy link
Contributor Author

@samsong8610, Currently for Aliyun ECS, if you did not create a security group through web browser, you would encounter 'None security group' error when running example_aliyun_ecs.py to create a VM instance. So I have added the ex_create_security_group and ex_delete_security_group functions in ecs.py to solve it

@Kami
Copy link
Member

Kami commented Jun 9, 2016

@net613 can you please rebase latest master on top of you branch / sync your branch with latest upstream master?

I have issue applying the patch directly (there are some conflicts).

Thanks.

net613 added 2 commits June 10, 2016 12:28
fix#800 E302 expected 2 blank lines

support ECSConnection and SLBConnection

expected 1 blank lines

fix no member named api_version

lint: commands failed

expected 2 blank lines

Aliyun ecs: Add support for creating/deleting security group

fix E501 line too long (83 > 79 characters)

fix E203 whitespace before ':'
@netgenius18
Copy link
Contributor Author

@Kami I have rebased, please try again

@Kami
Copy link
Member

Kami commented Jun 10, 2016

@net613 Thanks.

Looks like rebase removed (9bf6448) change, was this intentional?

The patch also contains *.pyc files which we don't commit and include in the repository - can you please remove those as well?

@Kami
Copy link
Member

Kami commented Jun 10, 2016

Thanks.

How about the removed change? It seems like it shouldn't have been removed, or?

@netgenius18
Copy link
Contributor Author

@Kami , I have removed all *.pyc files, please try again. Thanks
The removed (9bf6448) code was not intentional. In fact, It was another bug , and I would pull a new request to solve it.

@asfgit asfgit closed this in 2583d62 Jun 10, 2016
asfgit pushed a commit that referenced this pull request Jun 10, 2016
Closes #800
asfgit pushed a commit that referenced this pull request Jun 10, 2016
@Kami
Copy link
Member

Kami commented Jun 10, 2016

There was still one .pyc file left (libcloud/__init__.pyc). I removed that and merged branch into trunk.

In the future I would recommend you to base your changes on a new and separate branch in your fork and not trunk. This will make it easier to synchronize your fork with the latest upstream version and prevent conflicts.

Thanks.

@netgenius18
Copy link
Contributor Author

Thanks @Kami

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

3 participants