Proxy Protocol support for AWS ELB

[stuszynski]

This PR adds a new option for load_balancer resource to enable a Proxy Protocol Header on desired ports.

It requires a proxy_protocol item in load_balancer_options configuration hash. For eg.

proxy_protocol:  {
  instance_ports: [80, 443]
}

This will create an #{actual_elb.name}-proxy-protocol-policy policy and attach it to a backend port 80, and 443. It looks only for existing listeners with instance_port and it removes backend server policy settings on any other ports if not listed.

What do you think of it?

[tyler-ball]

Hey @stuszynski! Thanks for getting this PR setup. I have a few pieces of feedback but I think we can work on getting this merged.

First, we definitely want to get tests added for this. Have you ever tried to run the tests locally? You can run the load balancer tests by (in the repo) running bundle install && AWS_TEST_DRIVER=aws::us-east-1 bundle exec rspec spec/integration/load_balancer_spec.rb. This will use your default credentials and spin up things in the us-east-1 availability zone. This can be changed to any zone you want.

To be clear: do not do this any place it can corrupt actual data! I have a test account I use and I supply the profile name for that tester account with AWS_TEST_DRIVER=aws:tester:us-east-1. The tests should be isolated and idempotent (clean up after themselves) but nothing is guaranteed with integration tests. I'm going to try setting up some tests that I can help contribute in a second.

My other point of feedback is that we need this new attribute to be idempotent - it needs to be deleteable. Can you add logic so that if a recipe says proxy_protocol: false then the policy is deleted? I think the logic would look something like

if proxy_protocol == false
  # do stuff to delete the "#{actual_elb.name}-proxy-protocol-policy" object
elsif proxy_protocol
  # existing create logic
end

That isn't a fully idempotent attribute (it would be if it switched the policy to the correct ports like the sticky_sessions attribute does) but I think this would be a good first pass.

With this logic and test coverage I think we can definitely get this merged. Thanks!

[tyler-ball]

From 3b80a04c5992b59c568b5a42550e6702a518d566 Mon Sep 17 00:00:00 2001
From: tyler-ball <[email protected]>
Date: Thu, 2 Mar 2017 12:05:53 -0600
Subject: [PATCH] WIP: tests

---
 spec/integration/load_balancer_spec.rb | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/spec/integration/load_balancer_spec.rb b/spec/integration/load_balancer_spec.rb
index dba8dbf..52fc556 100644
--- a/spec/integration/load_balancer_spec.rb
+++ b/spec/integration/load_balancer_spec.rb
@@ -61,6 +61,9 @@ describe Chef::Resource::LoadBalancer do
                 cookie_name: 'test-cookie-name',
                 ports: [80, 443]
               },
+              proxy_protocol:  {
+                instance_ports: [80, 443]
+              },
               scheme: "internal",
               attributes: {
                 cross_zone_load_balancing: {
@@ -141,18 +144,36 @@ describe Chef::Resource::LoadBalancer do
           {
             policy_attribute_descriptions: [
               {attribute_value: "test-cookie-name", attribute_name: "CookieName"}
-          ],
+            ],
             policy_type_name: "AppCookieStickinessPolicyType",
             policy_name: "test-load-balancer-sticky-session-policy"
           }
         )
 
-        listener_descriptions = driver.elb_client.describe_load_balancers(load_balancer_names: ['test-load-balancer'])[:load_balancer_descriptions][0][:listener_descriptions]
+        proxy_policy = driver.elb_client.describe_load_balancer_policies(load_balancer_name: 'test-load-balancer')[:policy_descriptions].detect { |pd| pd[:policy_type_name] == 'ProxyProtocolPolicyType' }.to_h
+        expect(proxy_policy).to eq(
+          {
+            policy_name: "test-load-balancer-proxy-protocol-policy",
+            policy_type_name: "ProxyProtocolPolicyType",
+            policy_attribute_descriptions: [
+              {attribute_name: "ProxyProtocol", attribute_value: "true"}
+            ]
+          }
+        )
+
+        lb_descriptions = driver.elb_client.describe_load_balancers(load_balancer_names: ['test-load-balancer'])[:load_balancer_descriptions][0]
+        listener_descriptions = lb_descriptions[:listener_descriptions]
         expect(listener_descriptions.size).to eql(2)
         http_listener = listener_descriptions.detect { |ld| ld[:listener][:load_balancer_port] == 80 }
         https_listener = listener_descriptions.detect { |ld| ld[:listener][:load_balancer_port] == 443 }
         expect(http_listener[:policy_names]).to include('test-load-balancer-sticky-session-policy')
         expect(https_listener[:policy_names]).to include('test-load-balancer-sticky-session-policy')
+
+        backend_server_descriptions = lb_descriptions[:backend_server_descriptions]
+        expect(backend_server_descriptions.map &:to_h).to eq([
+          {:instance_port=>80, :policy_names=>["test-load-balancer-proxy-protocol-policy"]},
+          {:instance_port=>443, :policy_names=>["test-load-balancer-proxy-protocol-policy"]}
+        ])
       end
 
       context 'with an existing load balancer' do
@@ -199,6 +220,9 @@ describe Chef::Resource::LoadBalancer do
               cookie_name: 'test-cookie-name',
               ports: [80]
             },
+            proxy_protocol:  {
+              instance_ports: [80, 443]
+            },
             scheme: "internal",
             attributes: {
               cross_zone_load_balancing: {
@@ -252,6 +276,7 @@ describe Chef::Resource::LoadBalancer do
                   cookie_name: 'test-cookie-name2',
                   ports: [443]
                 },
+                proxy_protocol: false,
                 # scheme is immutable, we cannot update it
                 #scheme: "internet-facing",
                 attributes: {
@@ -333,8 +358,11 @@ describe Chef::Resource::LoadBalancer do
             }
           )
 
+          proxy_policy = driver.elb_client.describe_load_balancer_policies(load_balancer_name: 'test-load-balancer')[:policy_descriptions].detect { |pd| pd[:policy_type_name] == 'ProxyProtocolPolicyType' }
+          expect(proxy_policy).to eq(nil)
+
           listener_descriptions = driver.elb_client.describe_load_balancers(load_balancer_names: ['test-load-balancer'])[:load_balancer_descriptions][0][:listener_descriptions]
-          expect(listener_descriptions.size).to eql(1)
+          expect(listener_descriptions.size).to eql(2)
           https_listener = listener_descriptions.detect { |ld| ld[:listener][:load_balancer_port] == 443 }
           expect(https_listener[:policy_names]).to include('test-load-balancer-sticky-session-policy')
         end
-- 
2.11.1

Here is a patch file that takes a stab at the tests. They are failing right now because it looks like it is only attaching the policy to the first port?

[stuszynski]

@tyler-ball Hi! Great thanks for a feedback. I'll look into this in my spare time.