From 1aadf215673ee4d2f9702dde526587e044966637 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 7 Aug 2024 14:07:28 +0100 Subject: [PATCH 1/4] Repro QE bug and produce panic --- rest/adminapitest/admin_api_test.go | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 44b28ce56b..6071ba5fe8 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4298,19 +4298,34 @@ func TestDatabaseConfigAuditAPI(t *testing.T) { responseBody = nil require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &responseBody)) assert.Equal(t, true, responseBody["enabled"].(bool)) - assert.False(t, responseBody["events"].(map[string]interface{})[base.AuditIDISGRStatus.String()].(bool), "audit enabled event should be disabled by default") - assert.True(t, responseBody["events"].(map[string]interface{})[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be enabled by default") + eventsMap, ok := responseBody["events"].(map[string]interface{}) + require.True(t, ok) + assert.False(t, eventsMap[base.AuditIDISGRStatus.String()].(bool), "audit enabled event should be disabled by default") + assert.True(t, eventsMap[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be enabled by default") + + // use event IDs returned from GET response to disable all of them + for id := range eventsMap { + eventsMap[id] = false + } + eventsJSON, err := json.Marshal(eventsMap) + require.NoError(t, err) - // CBG-4111: Try to disable events on top of the default (nil) set... either PUT or POST where *all* of the given IDs are set to false. Bug results in a no-op. - resp = rt.SendAdminRequest(http.MethodPost, "/db/_config/audit", fmt.Sprintf(`{"enabled":true,"events":{"%s":false}}`, base.AuditIDPublicUserAuthenticated)) + // CBG-4111: Try to disable all events on top of the default (nil) set... either PUT or POST where *all* of the given IDs are set to false. Bug results in a no-op. + // CBG-????: Ensure ALL specified events were actually disabled. QE reported that some stay true! + resp = rt.SendAdminRequest(http.MethodPost, "/db/_config/audit", fmt.Sprintf(`{"enabled":true,"events":%s}`, eventsJSON)) rest.RequireStatus(t, resp, http.StatusOK) - // check event we just tried to disable + // check all events were actually disabled resp = rt.SendAdminRequest(http.MethodGet, "/db/_config/audit", "") rest.RequireStatus(t, resp, http.StatusOK) resp.DumpBody() responseBody = nil require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &responseBody)) - assert.False(t, responseBody["events"].(map[string]interface{})[base.AuditIDPublicUserAuthenticated.String()].(bool), "public user authenticated event should be disabled") + eventsMap = nil + eventsMap, ok = responseBody["events"].(map[string]interface{}) + require.True(t, ok) + for id, val := range eventsMap { + assert.False(t, val.(bool), "event %s should be disabled", id) + } // do a PUT to completely replace the full config (events not declared here will be disabled) // enable AuditEnabled event, but implicitly others From 8f0ce296bdbb686536a655f884c6afea5f816ed7 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 7 Aug 2024 14:07:57 +0100 Subject: [PATCH 2/4] Fix POST db audit events logic when most of the events are set to 'false' --- rest/admin_api.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/rest/admin_api.go b/rest/admin_api.go index de1965676e..ce528960cc 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -917,25 +917,21 @@ func mutateConfigFromDbAuditConfigBody(isReplace bool, existingAuditConfig *DbAu // initialize to non-nil set of defaults before modifying from request existingAuditConfig.EnabledEvents = &base.DefaultDbAuditEventIDs } - if existingAuditConfig.EnabledEvents != nil { - for i, event := range *existingAuditConfig.EnabledEvents { - if shouldEnable, ok := eventsToChange[base.AuditID(event)]; ok { - if shouldEnable { - // already enabled - } else { - // disable by removing - *existingAuditConfig.EnabledEvents = append((*existingAuditConfig.EnabledEvents)[:i], (*existingAuditConfig.EnabledEvents)[i+1:]...) - } - // drop from toChange so we don't duplicate IDs - delete(eventsToChange, base.AuditID(event)) - } + // build EnabledEvents back up in temp based on request - avoids mutating slice in-place during iteration + // slice[:0] reuses underlying array to avoid alloc of a new slice + newEnabledEvents := (*existingAuditConfig.EnabledEvents)[:0] + for _, event := range *existingAuditConfig.EnabledEvents { + if _, ok := eventsToChange[base.AuditID(event)]; !ok { + // existing enabled event and not in request - don't change + newEnabledEvents = append(newEnabledEvents, event) } - for id, enabled := range eventsToChange { - if enabled { - *existingAuditConfig.EnabledEvents = append(*existingAuditConfig.EnabledEvents, uint(id)) - } + } + for id, enabled := range eventsToChange { + if enabled { + newEnabledEvents = append(newEnabledEvents, uint(id)) } } + *existingAuditConfig.EnabledEvents = newEnabledEvents } } } From 845d6b68b21b5798a6fb56adfac4bac1b36deba1 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 7 Aug 2024 14:10:47 +0100 Subject: [PATCH 3/4] remove ineffassign in test --- rest/adminapitest/admin_api_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 6071ba5fe8..1cd8b60351 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4320,7 +4320,6 @@ func TestDatabaseConfigAuditAPI(t *testing.T) { resp.DumpBody() responseBody = nil require.NoError(t, json.Unmarshal(resp.Body.Bytes(), &responseBody)) - eventsMap = nil eventsMap, ok = responseBody["events"].(map[string]interface{}) require.True(t, ok) for id, val := range eventsMap { From d3f38b03d57bd075c046dc49bcc3719666cd9b07 Mon Sep 17 00:00:00 2001 From: Ben Brooks Date: Wed, 7 Aug 2024 14:49:31 +0100 Subject: [PATCH 4/4] Fix test comment Jira ID --- rest/adminapitest/admin_api_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest/adminapitest/admin_api_test.go b/rest/adminapitest/admin_api_test.go index 1cd8b60351..63fffe8339 100644 --- a/rest/adminapitest/admin_api_test.go +++ b/rest/adminapitest/admin_api_test.go @@ -4310,8 +4310,8 @@ func TestDatabaseConfigAuditAPI(t *testing.T) { eventsJSON, err := json.Marshal(eventsMap) require.NoError(t, err) - // CBG-4111: Try to disable all events on top of the default (nil) set... either PUT or POST where *all* of the given IDs are set to false. Bug results in a no-op. - // CBG-????: Ensure ALL specified events were actually disabled. QE reported that some stay true! + // CBG-4111: Try to disable events on top of the default (nil) set... either PUT or POST where *all* of the given IDs are set to false. Bug results in a no-op. + // CBG-4157: Ensure ALL specified events were actually disabled. Bug results in some events remaining 'true', and sometimes panicking by going out-of-bounds in a slice. resp = rt.SendAdminRequest(http.MethodPost, "/db/_config/audit", fmt.Sprintf(`{"enabled":true,"events":%s}`, eventsJSON)) rest.RequireStatus(t, resp, http.StatusOK) // check all events were actually disabled