Refactoring assignment of environment variables for dependencies that…

… expose more than one port

app/models/service.rb

@@ -6,8 +6,6 @@ class Service
 
       using FleetAdapter::StringExtensions
 
-      MAX_PORT = 65536
-
       attr_accessor :id, :name, :source, :links, :command, :ports,
         :expose, :environment, :volumes, :deployment, :prefix
 
@@ -105,9 +103,6 @@ def unit_block
       def service_block
         docker_rm = "-/usr/bin/docker rm #{prefix}"
         service_registration = "/usr/bin/etcdctl set app/#{name.upcase}/#{name.upcase}_SERVICE_HOST ${COREOS_PRIVATE_IPV4}"
-        if min_port
-          service_registration += " ; /usr/bin/etcdctl set app/#{name.upcase}/#{name.upcase}_SERVICE_PORT #{min_port}"
-        end
 
         {
           # A hack to be able to have two ExecStartPre values
@@ -143,17 +138,6 @@ def docker_run_string
         ].flatten.compact.join(' ').strip
       end
 
-      def min_port
-        if ports.empty?
-          min_container_port = MAX_PORT
-        else
-          min_container_port = ports.sort_by { |port_binding| port_binding[:containerPort]}.first[:containerPort]
-        end
-        min_exposed = expose.sort.first || MAX_PORT
-        min_port = [min_container_port.to_i, min_exposed.to_i].min
-        min_port unless min_port == MAX_PORT
-      end
-
       def port_flags
         return unless ports
 
@@ -183,36 +167,40 @@ def link_flags
           link_alias = link[:alias].upcase if link[:alias]
           link_name = link[:name].sanitize.upcase
 
+          min_port = link[:exposed_ports].sort_by { |exposed_port| exposed_port[:containerPort] }.first
+
           link_vars.push(
             {
               variable: (link_alias ? "#{link_alias}_SERVICE_HOST" : "#{link_name}_SERVICE_HOST").upcase,
               value: "`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`"
             },
-            {
-              variable: (link_alias ? "#{link_alias}_SERVICE_PORT" : "#{link_name}_SERVICE_PORT").upcase,
-              value: "`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_PORT`"
-            },
             {
               variable: (link_alias ? "#{link_alias}_PORT" : "#{link_name}_PORT").upcase,
-              value: "#{link[:protocol]}://`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`:`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_PORT`"
-            },
-            {
-              variable: (link_alias ? "#{link_alias}_PORT_#{link[:port]}_#{link[:protocol]}" : "#{link_name}_PORT_#{link[:port]}_#{link[:protocol]}").upcase,
-              value: "#{link[:protocol]}://`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`:`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_PORT`"
-            },
-            {
-              variable: (link_alias ? "#{link_alias}_PORT_#{link[:port]}_#{link[:protocol]}_PROTO" : "#{link_name}_PORT_#{link[:port]}_#{link[:protocol]}_PROTO").upcase,
-              value: link[:protocol]
-            },
-            {
-              variable: (link_alias ? "#{link_alias}_PORT_#{link[:port]}_#{link[:protocol]}_PORT" : "#{link_name}_PORT_#{link[:port]}_#{link[:protocol]}_PORT").upcase,
-              value: "`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_PORT`"
-            },
-            {
-              variable: (link_alias ? "#{link_alias}_PORT_#{link[:port]}_#{link[:protocol]}_ADDR" : "#{link_name}_PORT_#{link[:port]}_#{link[:protocol]}_ADDR").upcase,
-              value: "`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`"
+              value: "#{min_port[:protocol]}://`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`:#{min_port[:hostPort]}"
             }
           )
+
+          # Docker-esque container linking variables
+          link[:exposed_ports].each do |exposed_port|
+            link_vars.push(
+              {
+                variable: (link_alias ? "#{link_alias}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}" : "#{link_name}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}").upcase,
+                value: "#{exposed_port[:protocol]}://`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`:#{exposed_port[:hostPort]}"
+              },
+              {
+                variable: (link_alias ? "#{link_alias}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_PROTO" : "#{link_name}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_PROTO").upcase,
+                value: exposed_port[:protocol]
+              },
+              {
+                variable: (link_alias ? "#{link_alias}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_PORT" : "#{link_name}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_PORT").upcase,
+                value: exposed_port[:hostPort]
+              },
+              {
+                variable: (link_alias ? "#{link_alias}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_ADDR" : "#{link_name}_PORT_#{exposed_port[:containerPort]}_#{exposed_port[:protocol]}_ADDR").upcase,
+                value: "`/usr/bin/etcdctl get app/#{link_name}/#{link_name}_SERVICE_HOST`"
+              }
+            )
+          end
         end
 
         link_vars.map { |link| "-e #{link[:variable]}=#{link[:value]}" }

app/models/service_sorter.rb

@@ -60,36 +60,22 @@ def set_link_port_and_protocol(service, dependency)
           return if service[:links].to_a.empty?
 
           exposed_ports = ports_and_protocols_for(dependency)
-          unless min_port = exposed_ports.sort_by { |exposed_port| exposed_port[:port] }.first
-            raise ArgumentError, "#{dependency[:name]} does not expose a port"
-          end
 
-          service[:links].find { |link| link[:name] == dependency[:name] }
-                         .merge!({ port: min_port[:port], protocol: min_port[:protocol] })
+          service[:links].find { |link| link[:name] == dependency[:name] }.merge!({ exposed_ports: exposed_ports })
         end
 
         # Finds the explicitly exposed ports (:ports and :expose) on the dependency and
         # creates a hash of the port and protocol for each
         def ports_and_protocols_for(service)
-          ports = []
-
-          if service[:ports]
-            mapped_ports = service[:ports].map do |exposed_port|
-              { port: exposed_port[:hostPort], protocol: (exposed_port[:protocol] || 'tcp') }
-            end
-            ports.push(mapped_ports).flatten!
+          if service[:ports].to_a.empty?
+            raise ArgumentError, "#{service[:name]} does not have an explicit port binding"
           end
 
-          if service[:expose]
-            exposed_ports = service[:expose].map do |exposed_port|
-              { port: exposed_port, protocol: 'tcp' }
-            end
-            ports.push(exposed_ports).flatten!
+          mapped_ports = service[:ports].map do |exposed_port|
+            exposed_port.merge!({ protocol: 'tcp' }) unless exposed_port.has_key?(:proto)
           end
-
-          return ports
+          mapped_ports.flatten
         end
-
       end
     end
   end

spec/api/services_spec.rb

@@ -60,7 +60,7 @@
       end
 
       it 'includes an error message indicating the service should expose a port' do
-        expected = { error: 'dependency does not expose a port' }.to_json
+        expected = { error: 'dependency does not have an explicit port binding' }.to_json
         post '/v1/services', request_body
         expect(last_response.body).to eq expected
       end

spec/models/service_sorter_spec.rb

@@ -20,23 +20,9 @@
     it 'adds the port and protocol of the dependency to the dependent link hash' do
       expect(subject.last[:links].first[:name]).to eq 'DB'
       expect(subject.last[:links].first[:alias]).to eq 'DB_1'
-      expect(subject.last[:links].first[:port]).to eq 3306
-      expect(subject.last[:links].first[:protocol]).to eq 'tcp'
-    end
-
-    context 'when a dependent container exposes ports' do
-      before do
-        @db_service = services.find { |service| service[:name] == 'DB' }
-        @db_service[:expose] = [80]
-      end
-
-      it 'uses the lowest numbered exposed port as the link port' do
-        expect(subject.last[:links].first[:port]).to eq 80
-      end
-
-      it "uses 'tcp' as the link protocol" do
-        expect(subject.last[:links].first[:protocol]).to eq 'tcp'
-      end
+      expect(subject.last[:links].first[:exposed_ports].first[:hostPort]).to eq 1111
+      expect(subject.last[:links].first[:exposed_ports].first[:containerPort]).to eq 3306
+      expect(subject.last[:links].first[:exposed_ports].first[:protocol]).to eq 'tcp'
     end
 
     context 'when a container links to itself' do
@@ -66,18 +52,6 @@
       end
 
     end
-
-    context 'when a dependency does not expose a port through expose or port bindings' do
-      before do
-        @db_service = services.find { |service| service[:name] == 'DB' }
-        @db_service[:expose] = []
-        @db_service[:ports] = []
-      end
-
-      it 'raises an exception' do
-        expect{ described_class.sort(services) }.to raise_error(ArgumentError, /does not expose a port/)
-      end
-    end
   end
 
   describe '.get_service_names_for' do
@@ -95,26 +69,19 @@
   end
 
   describe '.ports_and_protocols_for' do
-    it 'responds with mapped ports when there are only mapped ports' do
-      service = { ports: [{ hostPort: 80, protocol: 'tcp' }] }
-      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([{ port: 80, protocol: 'tcp' }])
-    end
-
-
-    it 'responds with exposed ports when there are only exposed ports' do
-      service = { expose: [80] }
-      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([{ port: 80, protocol: 'tcp' }])
+    it 'raises an exception if there are no ports' do
+      service = { ports: [] }
+      expect{ described_class.send(:ports_and_protocols_for, service) }.to raise_error(ArgumentError, /does not have an explicit port binding/)
     end
 
-    it 'combines mapped and exposed ports if there are both' do
-      service = { expose: [80], ports: [{ hostPort: 8080, protocol: 'tcp' }] }
-      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([{ port: 80, protocol: 'tcp' },
-                                                                                      { port: 8080, protocol: 'tcp' }])
+    it 'responds with mapped ports' do
+      service = { ports: [{ hostPort: 80, containerPort: 1234, protocol: 'tcp' }] }
+      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([{ hostPort: 80, containerPort: 1234, protocol: 'tcp' }])
     end
 
-    it 'returns an empty array if there no mapped or exposed ports' do
-      service = { expose: [], ports: [] }
-      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([])
+    it 'adds the tcp protocol if no protocol type is defined' do
+      service = { ports: [{ hostPort: 80, containerPort: 1234 }] }
+      expect(described_class.send(:ports_and_protocols_for, service)).to match_array([{ hostPort: 80, containerPort: 1234, protocol: 'tcp' }])
     end
   end
 end

spec/models/service_spec.rb

@@ -9,7 +9,7 @@
       name: 'foo',
       source: 'bar',
       command: '/bin/bash',
-      ports: [{ containerPort: 3306 }],
+      ports: [{ hostPort: 3306, containerPort: 3306 }],
       expose: [3306],
       environment: [{ variable: 'DB_PASSWORD', value: 'password' }],
       volumes: [{ hostPath: '/foo/bar', containerPath: '/bar/baz' }],
@@ -171,6 +171,9 @@
   end
 
   describe '#docker_run_string' do
+    before do
+      subject.links = []
+    end
     context 'when the service specifies exposed ports' do
       it 'generates a docker command with --expose' do
         expect(subject.send(:docker_run_string)).to include '--expose 3306'
@@ -243,18 +246,17 @@
 
     context 'when the service specifies docker links' do
       it 'translates docker links to environment variables' do
-        subject.links = [{ name: 'db', alias: 'db_1', protocol: 'tcp', port: 3306 }]
+        subject.links = [{ name: 'db', alias: 'db_1', exposed_ports: [{ protocol: 'tcp', containerPort: 3306, hostPort: 3306 }] }]
         expect(subject.send(:docker_run_string)).to include '-e DB_1_SERVICE_HOST=`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`'
-        expect(subject.send(:docker_run_string)).to include '-e DB_1_SERVICE_PORT=`/usr/bin/etcdctl get app/DB/DB_SERVICE_PORT`'
-        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT=tcp://`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`:`/usr/bin/etcdctl get app/DB/DB_SERVICE_PORT`'
+        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT=tcp://`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`:3306'
         expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP_PROTO=tcp'
         expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP_ADDR=`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`'
-        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP=tcp://`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`:`/usr/bin/etcdctl get app/DB/DB_SERVICE_PORT`'
-        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP_PORT=`/usr/bin/etcdctl get app/DB/DB_SERVICE_PORT`'
+        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP=tcp://`/usr/bin/etcdctl get app/DB/DB_SERVICE_HOST`:3306'
+        expect(subject.send(:docker_run_string)).to include '-e DB_1_PORT_3306_TCP_PORT=3306'
       end
 
       it 'sanitizes the link names when creating the env vars' do
-        subject.links = [{ name: 'db_@:.-', alias: 'db_1', protocol: 'tcp', port: 3306 }]
+        subject.links = [{ name: 'db_@:.-', alias: 'db_1', exposed_ports: [{ protocol: 'tcp', containerPort: 3306, hostPort: 3306 }] }]
         expect(subject.send(:docker_run_string)).to include '-e DB_1_SERVICE_HOST=`/usr/bin/etcdctl get app/DB-----/DB-----_SERVICE_HOST`'
       end
     end
@@ -272,6 +274,9 @@
   end
 
   describe '#service_def' do
+    before do
+      subject.links = [{ name: 'db', alias: 'db', exposed_ports: [{ hostPort: 1, containerPort: 2, protocol: 'tcp' }] }]
+    end
     context 'when the service has links' do
       it 'assigns dependencies to the unit_block' do
         expect(subject.send(:service_def)['Unit']['After']).to eq('db.service')

spec/support/fixtures/post-services.json

@@ -16,7 +16,7 @@
     "environment": [
       {"variable": "MYSQL_ROOT_PASSWORD", "value": "pass@word01"}
     ],
-    "ports": [ {"hostPort": 3306, "containerPort": 3306} ],
+    "ports": [ {"hostPort": 1111, "containerPort": 3306} ],
     "deployment": { "count": 3 }
   }
 ]