Fix and simplify ALB/ELB protocol option logic

lib/aptible/cli/helpers/vhost/option_set_builder.rb

@@ -25,22 +25,21 @@ class OptionSetBuilder
             'TLSv1.3'
           ].freeze
 
-          SSL_PROTOCOL_PFS_VALUES =
-            SSL_PROTOCOL_VALUES.select { |v| v.include?(' PFS') }.freeze
+          ALB_PROTOCOL_VALUES = SSL_PROTOCOL_VALUES
 
-          SSL_PROTOCOL_NON_PFS_VALUES =
+          ELB_PROTOCOL_VALUES =
             SSL_PROTOCOL_VALUES.reject { |v| v.include?(' PFS') }.freeze
 
           SSL_PROTOCOL_ALB_DESC = (
             'Specify the allowed SSL protocols. Valid options: ' +
-            SSL_PROTOCOL_VALUES.map { |v| "\"#{v}\"" }.join(', ') +
+            ALB_PROTOCOL_VALUES.map { |v| "\"#{v}\"" }.join(', ') +
             '. PFS options require an HTTPS (ALB) endpoint. ' \
             'Use "default" to reset to the platform default'
           ).freeze
 
-          SSL_PROTOCOL_NON_ALB_DESC = (
+          SSL_PROTOCOL_ELB_DESC = (
             'Specify the allowed SSL protocols. Valid options: ' +
-            SSL_PROTOCOL_NON_PFS_VALUES.map { |v| "\"#{v}\"" }.join(', ') +
+            ELB_PROTOCOL_VALUES.map { |v| "\"#{v}\"" }.join(', ') +
             '. Use "default" to reset to the platform default'
           ).freeze
 
@@ -211,13 +210,13 @@ def declare_options(thor)
                         'on this Endpoint'
                 )
 
-                option(
-                  :ssl_protocols_override,
-                  type: :string,
-                  desc: SSL_PROTOCOL_NON_ALB_DESC
-                )
-
                 unless builder.alb?
+                  option(
+                    :ssl_protocols_override,
+                    type: :string,
+                    desc: SSL_PROTOCOL_ELB_DESC
+                  )
+
                   option(
                     :ssl_ciphers_override,
                     type: :string,
@@ -309,16 +308,17 @@ def prepare(account, options)
 
             if (proto = options[:ssl_protocols_override]) &&
                proto != 'default'
-              unless SSL_PROTOCOL_VALUES.include?(proto)
+              unless ALB_PROTOCOL_VALUES.include?(proto)
                 raise Thor::Error,
                       "Invalid --ssl-protocols-override: \"#{proto}\". " \
-                      "Valid options are: #{SSL_PROTOCOL_VALUES.join(', ')}"
+                      "Valid options are: #{ALB_PROTOCOL_VALUES.join(', ')}"
               end
 
-              if SSL_PROTOCOL_PFS_VALUES.include?(proto) && !alb?
+              if !ELB_PROTOCOL_VALUES.include?(proto) && !alb?
                 raise Thor::Error,
-                      '--ssl-protocols-override: PFS options are only ' \
-                      'available on HTTPS (ALB) endpoints'
+                      "Invalid --ssl-protocols-override: \"#{proto}\". " \
+                      'PFS options are only valid for an HTTPS (ALB) endpoint. ' \
+                      "Valid options are: #{ELB_PROTOCOL_VALUES.join(', ')}"
               end
             end
 

spec/aptible/cli/helpers/vhost/option_set_builder_spec.rb

@@ -0,0 +1,94 @@
+require 'spec_helper'
+
+describe Aptible::CLI::Helpers::Vhost::OptionSetBuilder do
+  def register_options(builder)
+    klass = Class.new(Thor) { include Aptible::CLI::Helpers::App }
+    builder.declare_options(klass)
+    klass.instance_variable_get(:@method_options)
+  end
+
+  describe '--ssl-protocols-override option description' do
+    context 'HTTPS endpoints (ALB, alb! flag set)' do
+      let(:builder) do
+        described_class.new do
+          app!
+          tls!
+          alb!
+        end
+      end
+
+      it 'includes PFS values' do
+        desc = register_options(builder)[:ssl_protocols_override].description
+        expect(desc).to include('PFS')
+      end
+    end
+
+    context 'TLS endpoints (ELB, tls! without alb!)' do
+      let(:builder) do
+        described_class.new do
+          app!
+          tls!
+        end
+      end
+
+      it 'does not include PFS values' do
+        desc = register_options(builder)[:ssl_protocols_override].description
+        expect(desc).not_to include('PFS')
+      end
+
+      it 'is still present' do
+        expect(register_options(builder)).to have_key(:ssl_protocols_override)
+      end
+    end
+
+    context 'gRPC endpoints (ELB, tls! without alb!)' do
+      let(:builder) do
+        described_class.new do
+          app!
+          port!
+          tls!
+        end
+      end
+
+      it 'does not include PFS values' do
+        desc = register_options(builder)[:ssl_protocols_override].description
+        expect(desc).not_to include('PFS')
+      end
+    end
+
+    context 'TCP endpoints (no tls! flag)' do
+      let(:builder) do
+        described_class.new do
+          app!
+          ports!
+        end
+      end
+
+      it 'is absent' do
+        expect(register_options(builder)).not_to have_key(:ssl_protocols_override)
+      end
+    end
+  end
+
+  describe 'SSL_PROTOCOL_ALB_DESC' do
+    subject { described_class::SSL_PROTOCOL_ALB_DESC }
+
+    it 'lists all PFS protocol values' do
+      pfs_values = described_class::SSL_PROTOCOL_VALUES.select { |v| v.include?('PFS') }
+      pfs_values.each { |v| is_expected.to include(v) }
+    end
+  end
+
+  describe 'SSL_PROTOCOL_ELB_DESC' do
+    subject { described_class::SSL_PROTOCOL_ELB_DESC }
+
+    it 'contains no PFS values' do
+      is_expected.not_to include('PFS')
+    end
+
+    it 'lists all non-PFS protocol values' do
+      non_pfs_values = described_class::SSL_PROTOCOL_VALUES.reject { |v| v.include?('PFS') }
+      non_pfs_values.each { |v| is_expected.to include(v) }
+    end
+  end
+end