Fix OptionsDeleter audit log keys#4704
Conversation
rawlinp
added
bug
| HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) | ||
| return | ||
| } | ||
| if !deleteKeyOptionExists { |
There was a problem hiding this comment.
The diff is kind of hard to read here, all I did was remove the if !deleteKeyOptionExists so that keys are still set with either Deleter or OptionsDeleter. This can be confirmed by clicking the wheel icon above and hiding whitespace changes.
|
I created and deleted a region and don't see the name in the audit log
|
|
Oh, maybe it wasn't clear, it would be either the ID or the name, depending on what the PK of the resource is. So regions uses IDs for deletion whereas Topologies uses the name, but I do agree that when deleting things we should probably always include the name (if it has one). I imagine that is a problem we have with a lot of our APIs currently. But for this PR, it fixes the regions audit log entry to include at least the ID and the topologies log entry to include the name. I think we should address the ID vs name thing separately. |
|
retest this please |
makes sense. |
|
Was this reviewed? |
| HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) | ||
| return | ||
| } | ||
| if !deleteKeyOptionExists { |
There was a problem hiding this comment.
We can't remove this condition here without re-adding the logic elsewhere. Normal delete requires all all keys exist, so Identifier.GetKeyFieldsInfo() generally only contains a single key.
HasDeleteKeyOptions.DeleteKeyOptions() is a separate array containing keys that can identify a resource, but only requires that one such key exists.
if !deleteKeyOptionExists {} is used to keep the handler from exiting if Identifier.GetKeyFieldsInfo() keys do not all exist but at least 1 HasDeleteKeyOptions.DeleteKeyOptions() key does.
DELETE regions?name={{name}} currently fails with "missing key: id".
There was a problem hiding this comment.
Opened a PR to add an API test for the failing route: #4707
* Fix OptionsDeleter audit log keys * Added TO API test to check audit log message * Undo whitespace addition in changelog message because it breaks tests * fixes timing of error message on delete Co-authored-by: Jeremy Mitchell <mitchell852@gmail.com> (cherry picked from commit 09bfc37)
* Fix OptionsDeleter audit log keys * Added TO API test to check audit log message * Undo whitespace addition in changelog message because it breaks tests * fixes timing of error message on delete Co-authored-by: Jeremy Mitchell <mitchell852@gmail.com> (cherry picked from commit 09bfc37)
3 participants
What does this PR (Pull Request) do?
Fixed the audit logging of
OptionsDeleterto set the keys on the to-be-deleted object so that they are included in the audit log entry when deleted.Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Run the unit tests and TO API tests, verify they pass. Observe the deletion audit log entries, verify they contain the key/name of the deleted resource (specifically Topologies and Regions, which both use the
OptionsDeleterinterface).If this is a bug fix, what versions of Traffic Control are affected?
The following criteria are ALL met by this PR