Set non-interactive default for set_security #150
Conversation
|
The changes to fabric_rpc and fabric_rpc2 are the same. Only fabric_rpc is used in the actual codebase, but I thought that it couldn't hurt being consistent. |
| @@ -217,6 +217,7 @@ get_update_seq(DbName) -> | |||
| with_db(DbName, [], {couch_db, get_update_seq, []}). | |||
|
|
|||
| set_security(DbName, SecObj, Options) -> | |||
| erlang:put(io_ignore_mm, true), | |||
There was a problem hiding this comment.
Would it be nicer to wrap the call to with_db in a pair of calls to set the IO priority to a non-interactive value? That'd simplify the diff as well as prevent the maintenance mode bypass from leaking out of this call.
There was a problem hiding this comment.
Point taken, but I don't think that's quite the right way to do that. IO priority is passed via the options to with_db. So, how about setting a non-interactive default in the fabric_rpc handler? Just pushed a thing.
|
Though that last change makes the commit msg not quite right, so I'd have to change that. |
| @@ -216,7 +216,13 @@ get_doc_count(DbName) -> | |||
| get_update_seq(DbName) -> | |||
| with_db(DbName, [], {couch_db, get_update_seq, []}). | |||
|
|
|||
| set_security(DbName, SecObj, Options) -> | |||
| set_security(DbName, SecObj, Options0) -> | |||
| Options = case lists:keyfind(io_priority, 1, Options0) of | |||
There was a problem hiding this comment.
@rnewson lists:keystore will overwrite the supplied default. Could do Options = lists:keymerge(1, Options0, [{io_priority, thing}]), but that'd require sorting Options0. Or just could leave as-is.
There was a problem hiding this comment.
ooh. let's at least have this function written once then.
There was a problem hiding this comment.
By "supplied default" I mean the io_priority which is possibly set in Options0.
There was a problem hiding this comment.
Get rid of fabric_rpc2:set_security since it isn't used in the codebase?
There was a problem hiding this comment.
enh, never mind. it's more work to pull this duplicate code into one place, +1
There was a problem hiding this comment.
Okay. Squashing, amending commit message, and merging. Squamerging?
|
+1 after addressing lists:keystore and commit msg update. |
Prior to this commit, fabric:set_security would default to interactive IO priority. This would cause all nodes to set the security header except shards on any nodes which were in MM. Subsequent calls to fabric:get_security would cause inconsistent security properties to be applied to the request. This patch sets a non-interactive default for fabric:set_security, which will cause security updates to be applied to all nodes regardless of MM by default. BugzID: 28847
|
That was the squamend. Merging. |
Set non-interactive default for set_security
Prior to this commit, a node would respect MM for clustered set_security
requests while still contributing to get_security requests. This would
lead to a situation where a user would change the security header for a
clustered DB, but the update would only reach a subset of the shards. On
subsequent requests, intermittant permission errors would be returned
due to security headers being inconsistent between shards.
BugzID: 28847