Fix and simplify ALB/ELB protocol option logic#413
Fix and simplify ALB/ELB protocol option logic#413UserNotFound wants to merge 1 commit intomasterfrom
Conversation
benjodo
left a comment
There was a problem hiding this comment.
Alex, I took a look and it looks functionally good to me. The following are findings that gpt 5.5 found, but I'm considering these nits; I'm approving as-is, and you're welcome to address these if you agree with them and send it back to me for re-review if you choose.
Finding (nit): The regression is not covered by tests; current specs bypass the Thor option declaration/help surface where this bug lived.
The branch changes which --ssl-protocols-override description is declared for ALB+TLS endpoints:
option(
:ssl_protocols_override,
type: :string,
desc: SSL_PROTOCOL_ALB_DESC
)
# ...
unless builder.alb?
option(
:ssl_protocols_override,
type: :string,
desc: SSL_PROTOCOL_ELB_DESC
)But the existing endpoint specs only verify runtime handling after options has already been stubbed:
context '--ssl-protocols-override' do
it 'passes a valid non-PFS value' do
wanted = { 'SSL_PROTOCOLS_OVERRIDE' => 'TLSv1.2' }
expect_create_vhost(service, {}, { settings: wanted })
stub_options(ssl_protocols_override: 'TLSv1.2')
subject.send(method, 'web')
endThat means the exact issue described in the PR, conflicting declaration logic for ALB+TLS help/options, can regress without a failing test.
Recommended fix: add focused coverage for the generated option descriptions, either by inspecting Thor’s registered options for endpoints:https:create, endpoints:https:modify, endpoints:tls:*, endpoints:grpc:*, and endpoints:tcp:*, or by adding CLI help-output specs. At minimum, assert HTTPS includes PFS values and TLS/gRPC do not.
Finding (nit): The invalid-protocol error path still prints SSL_PROTOCOL_VALUES instead of the semantic constant used for validation.
unless ALB_PROTOCOL_VALUES.include?(proto)
raise Thor::Error,
"Invalid --ssl-protocols-override: \"#{proto}\". " \
"Valid options are: #{SSL_PROTOCOL_VALUES.join(', ')}"
endToday ALB_PROTOCOL_VALUES aliases SSL_PROTOCOL_VALUES, so this is not a functional bug, but it weakens the cleanup by keeping one path coupled to the old generic name.
Recommended fix: change the message to use ALB_PROTOCOL_VALUES.join(', '), matching the validation branch.
There was conflicting logic for which option set was used when a vhost was both ALB and TLS. HTTP endpoints properly list the PFS options now.