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 policy management methods to the ELB driver #253

Closed
wants to merge 3 commits into from

Conversation

@npsolve
Copy link

npsolve commented Feb 22, 2014

@Kami I am not able to debug the elb.py individually. I have followed the testing guidlines from the official documentation.

P.S. From now onward i will create pull request after i have made few changes. I am not used to it, because in fedora community we used to discuss things on fpaste and then create pr.

Rahul Ranjan added 2 commits Feb 18, 2014
added test suite for the ex_rename() extention method

changed ex_rename() to ex_rename_node() extention method

Added the MockHttptestcase from ex_rename_node() extension method

changed ex_rename() to ex_rename_node() extention method

corrected the MockHttptestcase for ex_rename_node() method

Added a fixture for ex_rename_node() method
@@ -63,6 +63,42 @@ def list_balancers(self):
data = self.connection.request(ROOT, params=params).object
return self._to_balancers(data)

def ex_list_balancer_policy(self):

This comment has been minimized.

@Kami

Kami Feb 22, 2014 Member

This method returns a list so it should have a plural name (policy -> policies).

Please also add a docstring which documents what the method does and what it returns.

This comment has been minimized.

@npsolve

npsolve Feb 23, 2014 Author

Yeah I was using doc string initially but found that no other methods have and so I removed them. Now I will add them.

return [findtext(element=el, xpath='PolicyName', namespace=NS)
for el in findall(element=data, xpath=xpath, namespace=NS)]

def _to_ex_policy_types(self, data):

This comment has been minimized.

@Kami

Kami Feb 22, 2014 Member

Same here (plural name).

Also, you no need to have ex_ prefix in a "private" method. _to_policy is fine here.

'PolicyName': policy_name
}

data = self.connection.request(ROOT, params=params).object

This comment has been minimized.

@Kami

Kami Feb 22, 2014 Member

What status code is returned on a successful deletion?

Should do something like:

return response.status == httplib.DELETED
@Kami
Copy link
Member

Kami commented Feb 23, 2014

@rahulrrixe I'm not sure exactly what you mean with "I am not able to debug the elb.py individually".

You don't have an AWS account? Or is it something else?

@@ -132,6 +168,16 @@ def balancer_detach_member(self, balancer, member):
def balancer_list_members(self, balancer):
return balancer._members

def _to_ex_policy(self, data):

This comment has been minimized.

@Kami

Kami Feb 23, 2014 Member

Same here (plural name).

data = self.connection.request(ROOT, params=params).object
return True

def ex_destroy_balancer_policy(self, balancer, policy_name):

This comment has been minimized.

@Kami

Kami Feb 23, 2014 Member

Recently we started using delete instead of destroy for delete operation. So in this case, ex_destroy_balancer_policy -> ex_delete_balancer_policy

params = {
'Action': 'CreateLoadBalancerPolicy',
'LoadBalancerName': balancer.id,
#'PolicyAttributes.member.1.AttributeName': '' ,

This comment has been minimized.

@Kami

Kami Feb 23, 2014 Member

Those two parameters are only mandatory for custom (non pre-defined) policies, right?

This comment has been minimized.

@npsolve

npsolve Feb 23, 2014 Author

yeah you are right; that's why i have commented them for now.

'PolicyTypeName' : str(policy_type)
}

data = self.connection.request(ROOT, params=params).object

This comment has been minimized.

@Kami

Kami Feb 23, 2014 Member

Should do return response.status == httplib.OK

@Kami
Copy link
Member

Kami commented Feb 23, 2014

To make this useful, we also need to implement SetLoadBalancerPoliciesOfListener method (http://docs.aws.amazon.com/ElasticLoadBalancing/latest/APIReference/API_SetLoadBalancerPoliciesOfListener.html).

This method actually applies a policy to the load balancer.

From the article:

elb-create-lb-policy  myELBName        \
  --policy-type SSLNegotiationPolicyType \
  --policy-name=mySSLPolicyName        \
  --attribute "name=Reference-Security-Policy, value=ELBSecurityPolicy-2014-01"

$ elb-set-lb-policies-of-listener  myELBName \
  --lb-port 443 \
  --policy-names mySSLPolicyName,[PolicyName2,...]
@npsolve
Copy link
Author

npsolve commented Feb 23, 2014

@Kami Thanks I will make the required changes.

regarding debugging I have fixed everything and its working fine and now working on writing test suits.

@npsolve
Copy link
Author

npsolve commented Feb 25, 2014

@Kami I have checked all the methods with the aws instance and they all are working fine.

@@ -17,7 +17,7 @@
'ElasticLBDriver'
]


from libcloud.utils.py3 import httplib

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Deleting those empty lines breaks lint (pep8 step) - https://travis-ci.org/apache/libcloud/jobs/19573774

This comment has been minimized.

@npsolve

npsolve Feb 25, 2014 Author

@Kami I will take care of it in future.

@@ -63,6 +71,162 @@ def list_balancers(self):
data = self.connection.request(ROOT, params=params).object
return self._to_balancers(data)

def ex_list_balancer_policies(self, balancer):

This comment has been minimized.

@@ -63,6 +71,162 @@ def list_balancers(self):
data = self.connection.request(ROOT, params=params).object
return self._to_balancers(data)

def ex_list_balancer_policies(self, balancer):
"""
Return a list of policy description structures.

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

"structures" makes it sounds like a list item is a complex structure (e.g. map / object / whatever), but in reality it appears that the list item is just a simple string (policy name).

Docstring should be updated to reflect that.


def ex_list_balancer_policy_types(self):
"""
Return a list of policy type description structures.

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Same here.

}

for index, (name, value) in enumerate(policy_attributes.iteritems(), 1):
params['PolicyAttributes.member.%d.AttributeName' % index] = name

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Please use parenthesis around values for string interpolation - ... % (index).

params['PolicyAttributes.member.%d.AttributeName' % index] = name
params['PolicyAttributes.member.%d.AttributeValue' % index] = value
else:
params['PolicyAttributes'] = ''

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Is it necessary to send an empty string in this case?

Or can this item be omitted all together if there are no attributes specified?

data = self.connection.request(ROOT, params=params).object
return self._to_policy_types(data)

def ex_create_balancer_policy(self, balancer, policy_name, policy_type, policy_attributes):

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

policy_attributes is optional, right?

If so, it should default to None.

This comment has been minimized.

@npsolve

npsolve Feb 25, 2014 Author

Yeah, your are right. I missed that.

@Kami
Copy link
Member

Kami commented Feb 25, 2014

There are still lint and test failures btw - https://travis-ci.org/apache/libcloud/builds/19573773

@@ -55,6 +54,15 @@ def __init__(self, access_id, secret, region):
self.region = region
self.connection.host = HOST % (region)

def create_list_params(self, params, items, label):

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

I would prefer this item to return a new params list instead of mutate params argument in place.

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Also, please make this a "private" method and move it to the end of the class.

:param LoadBalancerName: Name of load balancer
:type LoadBalancerName: ``str``
:param Listeners.member.N: List of LoadBalancerPort, InstancePort, Protocol, and SSLCertificateId items

This comment has been minimized.

@Kami

Kami Feb 25, 2014 Member

Hm, does each list item need to be a tuple with 4 elements?

Or is this only necessary for the ssl listener?

This comment has been minimized.

@npsolve

npsolve Feb 25, 2014 Author

It can be of three but the certificate should be given when HTTPS or SSL connection is used.

@npsolve
Copy link
Author

npsolve commented Feb 26, 2014

@Kami I have made all the changes that you have suggested and also written test suits for the newly added extension methods. I am getting lint failure at some places, will fix them soon.

Can you give me your feedback on these changes?

response = self.connection.request(ROOT, params=params)
return response.status == httplib.OK

def ex_set_balancer_policies_listener(self, balancer, balancer_port, policies):

This comment has been minimized.

@Kami

Kami Feb 26, 2014 Member

balancer_port should probably be port to be consistent with other methods.

This comment has been minimized.

@npsolve

npsolve Feb 26, 2014 Author

@Kami balancer_port I have given specifically in the method to differentiate it from instance_port

"""
Associates, updates, or disables a policy with a listener on the load balancer
:param LoadBalancerName: Name of load balancer

This comment has been minimized.

@Kami

Kami Feb 26, 2014 Member

Also, just noticed the docstrings aren't correct. They need to refer to the existing arguments names, so, for example:

:param balancer: Balancer to create the policy for.
:param port: Port to use.
...

This comment has been minimized.

@Kami

Kami Feb 26, 2014 Member

On top of that, this argument docstring seems to incorrectly reference a name instead of loadbalancer.

:param LoadBalancerPort: load balancer Port number
:type LoadBalancerPort: ``str``
:param PolicyNames.member.1: List of policies to be associated with the listener.

This comment has been minimized.

@Kami

Kami Feb 26, 2014 Member

In this case, it should look like this:

:param policies: List of policies to be associated with the listener.
:type policies: ``list``
@Kami
Copy link
Member

Kami commented Feb 26, 2014

It would also be good to add some documentation and examples of how to use that.

Adding an example for common scenario like creating a listener and setting a policy for SSL would be good.

@npsolve
Copy link
Author

npsolve commented Feb 26, 2014

@Kami I will add the documentation for the examples and also update different docstring's.

@Kami
Copy link
Member

Kami commented Feb 26, 2014

Just want to make sure we are both on the same page.

I meant adding documentation and examples to docs/loadbalancer/drivers/elb.rst :)

@npsolve
Copy link
Author

npsolve commented Feb 27, 2014

@Kami I have also added the documentation for the elb driver.

@Kami
Copy link
Member

Kami commented Feb 28, 2014

@rahulrrixe Awesome!

I will review it and add some comments a bit later on.

@@ -0,0 +1,109 @@
========================================

This comment has been minimized.

@Kami

Kami Feb 28, 2014 Member

Please use consistent heading styles - for example, see docs/compute/drivers/cloudsigma.rst.

This tutorial will focus on libcloud Elastic Load Balancing interface for AWS.

Refer more about ELB at AWS Site
http://docs.aws.amazon.com/ElasticLoadBalancing/latest/APIReference/Welcome.html

This comment has been minimized.

@Kami

Kami Feb 28, 2014 Member

You can create link on the AWS Site text. For example:

Refer more about ELB at `AWS Site`_
.. _`AWS Site`: http://docs.aws.amazon.com/ElasticLoadBalancing/latest/APIReference/Welcome.html

The first step in accessing ELB is to create a connection to the service.

>>> from libcloud.loadbalancer.types import Provider, State

This comment has been minimized.

@Kami

Kami Feb 28, 2014 Member

Please move code snippets into files and include them using literalinclude:: directive.

members=members)
[<LoadBalancer: id=balancer_id, name=balancer_name, state=balancer_state>]
Creating Load Balancer Policy

This comment has been minimized.

@Kami

Kami Feb 28, 2014 Member

It would be good to explain a bit how policies, listeners, etc. work and add one big example which shows how to put everything together by setting an SSL listener with custom cipher suites.

This comment has been minimized.

@npsolve

npsolve Feb 28, 2014 Author

@Kami I will do that.

@Kami
Copy link
Member

Kami commented Feb 28, 2014

@rahulrrixe You can do:

xpath = 'DescribeLoadBalancerPolicyTypesResult'
xpath += '/PolicyTypeDescriptions/member' 
@npsolve
Copy link
Author

npsolve commented Mar 2, 2014

@Kami I have updated the documentation but its facing some indentation error. Can you please suggest how can i debug it.

@Kami
Copy link
Member

Kami commented Mar 2, 2014

@rahulrrixe It looks like you are using invalid syntax. You should remove pipes (|) before the code snippets.

@npsolve
Copy link
Author

npsolve commented Mar 2, 2014

@Kami build successful 👍
Checked all the methods and test suits again on local system, everything is working fine. Also I tried to improve the documentation and will do more in future.

@Kami
Copy link
Member

Kami commented Mar 2, 2014

@rahulrrixe Great.

Can you please squash all the commits and all go ahead and merge the changes.

@npsolve
Copy link
Author

npsolve commented Mar 2, 2014

@Kami sure

…thod

Added test suite for ex_create_balancer_policy() and ex_destroy_balancer_policy() method

Added ex_set_balancer_policies_listener(self, balancer) method

added all the methods required for adding & deleting policies

corrected the docstring and positioning of methods

added testsuits for extension methods

fixture for ex_create_balancer_policy() method

fixture for ex_list_balancer_policies() method

fixture for ex_list_balancer_policy_types() method

fixture for ex_set_balancer_policies_backend_server() method

fixture for ex_set_balancer_policies_listener() method

removed basestring for python 3 support

PEP8 guidelines fixture

added the documentation for the extension methods

corrected various docstrings

corrected various docstrings and lint

correct the lint and added argument names

pep8 fixtures

pep8 fixtures

made changes in the doc

python code of creating connection for the doc

made changes in the doc

python code for creating policy for doc ref

python code for creating load balancer for doc ref

python code for creating balancer listeners for doc ref

python code for deleting balancer policy for doc ref

python code for listing  balancer policies for doc ref

python code for listing  balancer policy types for doc ref

python code for set balancer policy types for backend server

python code for set balancer policy types for listeners

list all the load balancers

all method implemetation of ELB driver

improved the documentation

pep8 correction

removed indendation error

update the docstrings

update the docstrings

removed invalid syntax

removed sphinx build errors
@npsolve
Copy link
Author

npsolve commented Mar 2, 2014

@Kami done 👍

@Kami
Copy link
Member

Kami commented Mar 3, 2014

I made some minor changes and fixed to the code examples and merged this patch into trunk.

Thanks.

@asfgit asfgit closed this in 68f8d22 Mar 3, 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

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