Skip to content

Commit

Permalink
config: delayed privilege alert doesn't stuck now
Browse files Browse the repository at this point in the history
The declarative configuration has the `credentials` section that
describes users and their privileges. It is OK to have privileges for a
space/function/sequence that does not exist. Such a privilege will lead
to an alert that states that the privilege will be granted, when the
object is created.

The problem that is fixed by this commit is that such an alert was not
dropped, when the object is created and the relevant privileges are
granted.

There are several ways to solve the problem. Let's look on them.

1. When a privilege is granted, drop an alert if any.
2. After the config-database privilege synchronization, revisit alerts
   to drop all obsolete ones.
3. Drop all the alerts regarding missed privileges before the
   config-database privilege synchronization and issue actual alerts
   afterwards.

The first way is the simplest, but it doesn't cover one specific
scenario: an object rename.

Let's assume that the object T has privileges declared in the
configuration and the object doesn't exist. There is an alert regarding
it. Now, object S is renamed to T. Let's assume that S had some or all
the privileges needed for T according to the configuration.

In the given scenario, we don't need to grant some or all of the
privileges and, so, the first solution doesn't work. We don't reach the
code that grants the privileges and, so, dropping alerts at this point
has no effect.

The second and the third solutions are similar and mainly differs in how
complicated the code is. The third one is implemented here with idea of
simplifying the code.

The internal `aboard` module has the following changes.

1. The `aboard` module now ignores underscored fields of an alert on its
   serialization to allow a caller to store a machine-readable
   information in them.
2. The new method `:drop_if()` is added to perform a conditional alert
   drop.

Several unit test cases are updated, because now we always need
initialized `config._aboard` for testing of the credentials applier.

Fixes #9574

NO_DOC=bugfix

(cherry picked from commit 1c1ee4d)
  • Loading branch information
Totktonada committed Feb 5, 2024
1 parent 91bf940 commit 9d68538
Show file tree
Hide file tree
Showing 5 changed files with 423 additions and 18 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## bugfix/config

* An alert regarding delayed privilege granting is now cleared when the
privilege is granted (gh-9574).
65 changes: 59 additions & 6 deletions src/box/lua/config/applier/credentials.lua
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,6 @@ local privileges_action_f = function(grant_or_revoke, role_or_user, name, privs,
:format(role_or_user, grant_or_revoke, name, privs, obj_type,
obj_name, err)
config._aboard:set({type = 'error', message = err})
else
local msg = "credentials.apply: %s %q hasn't been created yet, " ..
"'box.schema.%s.%s(%q, %q, %q, %q)' will be applied later"
msg = msg:format(obj_type, obj_name, role_or_user, grant_or_revoke,
name, privs, obj_type, obj_name)
config._aboard:set({type = 'warn', message = msg})
end
end

Expand All @@ -565,6 +559,18 @@ local function sync_privileges(credentials, obj_to_sync)
obj_to_sync.type, obj_to_sync.name)
end

-- Drop missed privilege alerts.
--
-- The actual alerts will be issued in the sync() function
-- below.
--
-- Important: we don't follow the `obj_to_sync` filter here
-- for simplicity. Just revisit all the missed privilege
-- alerts: drop all of them and issue again the actual ones.
config._aboard:drop_if(function(_key, alert)
return alert._trait == 'missed_privilege'
end)

-- The privileges synchronization between A and B is performed in 3 steps:
-- 1. Grant all privileges that are present in B,
-- but not present in A (`grant(B - A)`).
Expand Down Expand Up @@ -613,6 +619,50 @@ local function sync_privileges(credentials, obj_to_sync)
to_revoke.obj_name)
end
end

-- Collect updated information about privileges in the
-- database and recalculate the difference from the target
-- configuration.
box_privileges = box.schema[role_or_user].info(name)
box_privileges = privileges_from_box(box_privileges)
local missed_grants = privileges_subtract(config_privileges,
box_privileges)

-- The most frequent scenario is when a privilege
-- couldn't be granted, because the object (space/
-- function/sequence) doesn't exist at the moment.
--
-- That's normal and the alert will be dropped on next
-- sync_privileges() call. If the object appears and the
-- privileges from the configuration are given, the alert
-- will not be issued again in this call.
--
-- If a serious error occurs on the privilege granting
-- (not a 'no such an object' one), then an alert of the
-- 'error' type is issued in the privileges_action_f().
--
-- The 'error' alerts aren't dropped here. The only way
-- to get rid of them is to try to re-apply the
-- configuration using config:reload() (or using automatic
-- reload from a remote config storage in Tarantool
-- Enterprise Edition).
--
-- Important: this code deliberately ignores the
-- `obj_to_sync` filter. It should be in-sync with the
-- :drop_if() call above.
for _, grant in ipairs(missed_grants) do
local alert = {
type = 'warn',
_trait = 'missed_privilege',
}
local msg = 'box.schema.%s.%s(%q, %q, %q, %q) has failed ' ..
'because either the object has not been created yet, ' ..
'or the privilege write has failed (separate alert reported)'
local privs = table.concat(grant.privs, ',')
alert.message = msg:format(role_or_user, 'grant', name, privs,
grant.obj_type, grant.obj_name)
config._aboard:set(alert)
end
end

-- Note that the logic inside the sync() function, i.e. privileges dump
Expand Down Expand Up @@ -908,6 +958,9 @@ return {
apply = apply,
-- Exported for testing purposes.
_internal = {
set_config = function(config_module)
config = config_module
end,
privileges_from_box = privileges_from_box,
privileges_from_config = privileges_from_config,
privileges_subtract = privileges_subtract,
Expand Down
40 changes: 38 additions & 2 deletions src/box/lua/config/utils/aboard.lua
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ local log = require('internal.config.utils.log')
-- }
--
-- More fields may be placed into the table and all these fields
-- are returned from :alerts().
-- are returned from :alerts() (except ones that start from the
-- underscore character).
--
-- The alert board takes an ownership of the `alert` table and
-- modifies it.
Expand Down Expand Up @@ -67,6 +68,30 @@ local function aboard_drop(self, key)
end
end

-- Drop alerts that fit the given criteria.
--
-- | local function filter_f(key, alert)
-- | return <boolean>
-- | end
--
-- The filter function is called on each alert. An alert is
-- dropped if the function returns `true`.
--
-- The `on_drop` callback (see the aboard.new() function) is
-- called for each of the dropped alert.
local function aboard_drop_if(self, filter_f)
local to_drop = {}
for key, alert in pairs(self._alerts) do
if filter_f(key, alert) then
table.insert(to_drop, key)
end
end

for _, key in ipairs(to_drop) do
self:drop(key)
end
end

-- Drop all the alerts.
--
-- The `on_drop` callback is NOT called.
Expand All @@ -79,11 +104,21 @@ end
--
-- The alerts are sorted by a time of the :set() method call and
-- returned as an array-like table.
--
-- Fields of the alert object that start from the underscore
-- character are NOT shown.
local function aboard_alerts(self)
-- Don't return alert keys.
local alerts = {}
for _, alert in pairs(self._alerts) do
table.insert(alerts, alert)
-- Don't show fields that start from an underscore.
local serialized = {}
for k, v in pairs(alert) do
if not k:startswith('_') then
serialized[k] = v
end
end
table.insert(alerts, serialized)
end
-- Sort by timestamps.
table.sort(alerts, function(a, b)
Expand Down Expand Up @@ -122,6 +157,7 @@ local mt = {
set = aboard_set,
get = aboard_get,
drop = aboard_drop,
drop_if = aboard_drop_if,
clean = aboard_clean,
alerts = aboard_alerts,
status = aboard_status,
Expand Down
6 changes: 5 additions & 1 deletion test/config-luatest/appliers_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ local g = t.group()
local appliers_script = [[
local configdata = require('internal.config.configdata')
local cluster_config = require('internal.config.cluster_config')
local aboard = require('internal.config.utils.aboard')
local cconfig = {
credentials = {
users = {
Expand Down Expand Up @@ -40,7 +41,10 @@ local appliers_script = [[
},
}
local iconfig = cluster_config:instantiate(cconfig, 'instance-001')
config = {_configdata = configdata.new(iconfig, cconfig, 'instance-001')}
config = {
_configdata = configdata.new(iconfig, cconfig, 'instance-001'),
_aboard = aboard.new(),
}
local mkdir = require('internal.config.applier.mkdir')
mkdir.apply(config)
local box_cfg = require('internal.config.applier.box_cfg')
Expand Down

0 comments on commit 9d68538

Please sign in to comment.