Bug #16826

AuditLogs.UnloggedAttributes is being ignored

Added by Lucas Di Pentima about 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
09/11/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

When adding "manifest_text" to this config list, collection's manifests are still being logged.
A customer reported that the issue is fixed by using strings on keys instead of symbols on ArvadosModel::logged_attributes.


Subtasks

Task #16835: Review 16826-unlogged-attrs-fixResolvedLucas Di Pentima

Associated revisions

Revision e2d623bd
Added by Lucas Di Pentima about 1 year ago

Merge branch '16826-unlogged-attrs-fix'
Closes #16826

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Lucas Di Pentima about 1 year ago

  • Release set to 25

#2 Updated by Lucas Di Pentima about 1 year ago

  • Description updated (diff)

#3 Updated by Lucas Di Pentima about 1 year ago

Fix at c8a640d52 - branch 16826-unlogged-attrs-fix
Test run: https://ci.arvados.org/job/developer-run-tests/2089/

The problem was caused by the fact that the ActiveSupport::OrderedOptions class we use on the config.yml loading code always return keys as symbols even if they're assigned as strings:

2.5.5 :001 > require 'active_support'
 => true
2.5.5 :002 > opts = ActiveSupport::OrderedOptions.new
 => {}
2.5.5 :003 > opts['someKey'] = 'someValue'
 => "someValue" 
2.5.5 :004 > opts.keys
 => [:someKey]

This was tested on rails 5.0, 5.2 & 6.0

  • Added a test that confirms loaded config maps are OrderedOptions classes by checking Users.AutoSetupUsernameBlacklist that has predefined members by default, with a check that the keys are in fact being returned as symbols.
  • Set up the AuditLogs.UnloggedAttributes related tests with OrderedOptions instead of hashes to expose the bug.
  • Applied the fix proposed by the customer
  • Also set up other config map related tests accordingly to see if any test started to fail (no failures, fortunately)

#4 Updated by Tom Clegg about 1 year ago

This LGTM, thanks.

I'm a little concerned that it's too easy to repeat the same mistake (assigning regular hashes to config) in future. Maybe restore_configuration() in source:services/api/test/test_helper.rb could do a recursive "symbol keys" test on Rails.configuration to catch that?

#5 Updated by Lucas Di Pentima about 1 year ago

Tom Clegg wrote:

I'm a little concerned that it's too easy to repeat the same mistake (assigning regular hashes to config) in future. Maybe restore_configuration() in source:services/api/test/test_helper.rb could do a recursive "symbol keys" test on Rails.configuration to catch that?

Good idea!!

Updates at f4d0ea9ca
Test run: https://ci.arvados.org/job/developer-run-tests/2092/

  • Adds config checks on TestCase.teardown.
  • Moves restore_configuration() to TestCase.setup to avoid additional test failures after a config check assertion fail.
  • Fixes illegal test config settings discovered by the new check.
  • Fixes API.DisabledAPIs auto-setting depending on Containers.JobsAPI.Enable's value.

#6 Updated by Tom Clegg about 1 year ago

Great, more hidden bugs revealed & squashed! LGTM, thanks.

#7 Updated by Anonymous about 1 year ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF