diff --git a/jobs/gorouter/spec b/jobs/gorouter/spec index 9d694a029..1feb42f72 100644 --- a/jobs/gorouter/spec +++ b/jobs/gorouter/spec @@ -328,7 +328,7 @@ properties: this property and operators have to explicitly enable them. This is done to prevent breaking log parsing in existing setups by the introduction of new fields. Does not affect stdout / stderr logs. - Available fields are: local_address + Available fields are: backend_time, dial_time, dns_time, failed_attempts, failed_attempts_time, local_address, tls_time default: [] router.logging.format.timestamp: description: | diff --git a/jobs/gorouter/templates/gorouter.yml.erb b/jobs/gorouter/templates/gorouter.yml.erb index d0698016b..e54c5168e 100644 --- a/jobs/gorouter/templates/gorouter.yml.erb +++ b/jobs/gorouter/templates/gorouter.yml.erb @@ -391,11 +391,19 @@ if !['rfc3339', 'deprecated', 'unix-epoch'].include?(p('router.logging.format.ti raise "'#{p('router.logging.format.timestamp')}' is not a valid timestamp format for the property 'router.logging.format.timestamp'. Valid options are: 'rfc3339', 'deprecated', and 'unix-epoch'." end + +extra_access_log_fields = [] +# Support `enable_log_attempts_details` which was internally migrated to the new dynamic system. +if p('router.enable_log_attempts_details') + extra_access_log_fields.concat(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time']) +end + if_p('router.logging.extra_access_log_fields') do |extra_fields| - valid_extra_log_fields=['local_address'] + valid_extra_log_fields=['backend_time', 'dial_time', 'dns_time', 'failed_attempts', 'failed_attempts_time', 'local_address', 'tls_time'] if !extra_fields.all? { |el| valid_extra_log_fields.include?(el) } raise "router.logging.extra_access_log_fields (#{extra_fields}) contains invalid values, valid are #{valid_extra_log_fields}" end + extra_access_log_fields.concat(extra_fields) end @@ -410,13 +418,12 @@ params['logging'] = { 'syslog_addr' => p('router.logging.syslog_addr'), 'syslog_network' => p('router.logging.syslog_network'), 'level' => p('router.logging_level'), - 'extra_access_log_fields' => p('router.logging.extra_access_log_fields'), + 'extra_access_log_fields' => extra_access_log_fields, 'loggregator_enabled' => true, 'metron_address' => "localhost:#{p('metron.port')}", 'disable_log_forwarded_for' => p('router.disable_log_forwarded_for'), 'disable_log_source_ip' => p('router.disable_log_source_ip'), 'redact_query_params' => p('router.redact_query_parameters'), - 'enable_attempts_details' => p('router.enable_log_attempts_details'), 'format' => { 'timestamp' => timestamp_format, }, diff --git a/spec/gorouter_templates_spec.rb b/spec/gorouter_templates_spec.rb index 5cb1d351d..ce1d2a9ce 100644 --- a/spec/gorouter_templates_spec.rb +++ b/spec/gorouter_templates_spec.rb @@ -1334,8 +1334,8 @@ end context 'when enable_detailed_attempts_logging is not provided' do - it 'it defaults to false' do - expect(parsed_yaml['logging']['enable_attempts_details']).to eq(false) + it 'it does not set the correspoding extra access log values' do + expect(parsed_yaml['logging']['extra_access_log_fields']).not_to include(['backend_time', 'dial_time', 'dns_time', 'failed_attempts', 'failed_attempts_time', 'tls_time']) end end @@ -1344,7 +1344,7 @@ deployment_manifest_fragment['router']['enable_log_attempts_details'] = true end it 'it properly sets the value' do - expect(parsed_yaml['logging']['enable_attempts_details']).to eq(true) + expect(parsed_yaml['logging']['extra_access_log_fields']).to eq(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time']) end end @@ -1365,7 +1365,7 @@ end it 'raises an error' do - expect { parsed_yaml }.to raise_error(RuntimeError, 'router.logging.extra_access_log_fields (["foobar"]) contains invalid values, valid are ["local_address"]') + expect { parsed_yaml }.to raise_error(RuntimeError, /router\.logging\.extra_access_log_fields \(\["foobar"\]\) contains invalid values, valid are .*/) end end @@ -1375,11 +1375,22 @@ end it 'still raises an error' do - expect { parsed_yaml }.to raise_error(RuntimeError, 'router.logging.extra_access_log_fields (["local_address", "foobar"]) contains invalid values, valid are ["local_address"]') + expect { parsed_yaml }.to raise_error(RuntimeError, /router\.logging\.extra_access_log_fields \(\["local_address", "foobar"\]\) contains invalid values, valid are .*/) end end end + context 'when enable_detailed_attempts_logging and extra_access_log_fields is set' do + before do + deployment_manifest_fragment['router']['enable_log_attempts_details'] = true + deployment_manifest_fragment['router']['logging'] = { 'extra_access_log_fields' => ['local_address'] } + end + + it 'puts the extra_access_log_fields to the end to preserve order' do + expect(parsed_yaml['logging']['extra_access_log_fields']).to eq(['failed_attempts', 'failed_attempts_time', 'dns_time', 'dial_time', 'tls_time', 'backend_time', 'local_address']) + end + end + context 'when access log streaming via syslog is enabled' do before do deployment_manifest_fragment['router']['write_access_logs_locally'] = false