From deaf1c6949af5466794bf02a7e1b268d273beba9 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Mon, 13 Jul 2020 16:25:06 +0200 Subject: [PATCH 1/9] Ensure team size is correct even when team is suspended --- services/brig/src/Brig/User/Search/Index.hs | 2 +- services/brig/test/integration/API/Team.hs | 20 +++++++++++++++----- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/services/brig/src/Brig/User/Search/Index.hs b/services/brig/src/Brig/User/Search/Index.hs index 8525be2a8f7..c32c3957477 100644 --- a/services/brig/src/Brig/User/Search/Index.hs +++ b/services/brig/src/Brig/User/Search/Index.hs @@ -768,7 +768,7 @@ reindexRowToIndexUser (u, mteam, name, t0, status, t1, handle, t2, colour, t4, a [ case status of Just Active -> True Nothing -> True - Just Suspended -> False + Just Suspended -> True Just Deleted -> False Just Ephemeral -> False, activated, -- FUTUREWORK: how is this adding to the first case? diff --git a/services/brig/test/integration/API/Team.hs b/services/brig/test/integration/API/Team.hs index 4edcfa45033..0073cedfb70 100644 --- a/services/brig/test/integration/API/Team.hs +++ b/services/brig/test/integration/API/Team.hs @@ -50,6 +50,7 @@ import Imports import Network.HTTP.Client (Manager) import qualified Network.Wai.Test as WaiTest import qualified Network.Wai.Utilities.Error as Error +import Numeric.Natural (Natural) import Test.Tasty hiding (Timeout) import qualified Test.Tasty.Cannon as WS import Test.Tasty.HUnit @@ -107,11 +108,20 @@ testTeamSize :: Brig -> Http () testTeamSize brig = do (tid, _, _) <- createPopulatedBindingTeam brig 10 SearchUtil.refreshIndex brig - void $ - get (brig . paths ["i", "teams", toByteString' tid, "size"]) TeamId -> Natural -> Http () + assertSize tid expectedSize = void $ + get (brig . paths ["i", "teams", toByteString' tid, "size"]) Date: Tue, 14 Jul 2020 17:26:53 +0200 Subject: [PATCH 2/9] Ensure suspended users doen't show up in search results --- services/brig/src/Brig/User/Search/Index.hs | 44 ++++++++++++------- .../brig/src/Brig/User/Search/Index/Types.hs | 12 +++-- .../unit/Test/Brig/User/Search/Index/Types.hs | 11 +++-- 3 files changed, 43 insertions(+), 24 deletions(-) diff --git a/services/brig/src/Brig/User/Search/Index.hs b/services/brig/src/Brig/User/Search/Index.hs index c32c3957477..64a79f07e99 100644 --- a/services/brig/src/Brig/User/Search/Index.hs +++ b/services/brig/src/Brig/User/Search/Index.hs @@ -167,15 +167,6 @@ searchIndex :: m (SearchResult Contact) searchIndex u teamSearchInfo q = queryIndex (defaultUserQuery u teamSearchInfo q) --- [Note: suspended] TODO: has this happened by now and we can resolve this note? --- ~~~~~~~~~~~~~~~~~ --- --- The suspended field in the ES index is an artifact of the legacy system, --- which would not remove data from the index upon user suspension. The filter --- clause can be removed after the first full re-index, as this will have --- cleared the data of suspended users. --- - queryIndex :: (MonadIndexIO m, FromJSON r) => IndexQuery r -> @@ -258,11 +249,17 @@ mkUserQuery (review _TextId -> self) teamSearchInfo q = IndexQuery q $ ES.Filter . ES.QueryBoolQuery $ boolQuery - { ES.boolQueryMustNotMatch = - [ termQ "suspended" "1", -- [Note: suspended] - termQ "_id" self - ], - ES.boolQueryMustMatch = [optionallySearchWithinTeam teamSearchInfo] + { ES.boolQueryMustNotMatch = [termQ "_id" self], + ES.boolQueryMustMatch = + [ optionallySearchWithinTeam teamSearchInfo, + ES.QueryBoolQuery + boolQuery + { ES.boolQueryShouldMatch = + [ termQ "account_status" "active", + ES.QueryBoolQuery boolQuery {ES.boolQueryMustNotMatch = [ES.QueryExistsQuery (ES.FieldName "account_status")]} -- TODO: Write note about this being legacy + ] + } + ] } where termQ f v = @@ -531,6 +528,7 @@ userDoc iu = { udId = _iuUserId iu, udTeam = _iuTeam iu, udName = _iuName iu, + udAccountStatus = _iuAccountStatus iu, udNormalized = normalized . fromName <$> _iuName iu, udHandle = _iuHandle iu, udColourId = _iuColourId iu @@ -615,6 +613,14 @@ indexMapping = mpIndex = False, mpAnalyzer = Nothing, mpFields = mempty + }, + "account_status" + .= MappingProperty + { mpType = MPKeyword, + mpStore = False, + mpIndex = True, + mpAnalyzer = Nothing, + mpFields = mempty } ] ] @@ -755,19 +761,23 @@ reindexRowToIndexUser (u, mteam, name, t0, status, t1, handle, t2, colour, t4, a pure $ if shouldIndex then - iu & set iuTeam mteam + iu + & set iuTeam mteam . set iuName (Just name) . set iuHandle handle . set iuColourId (Just colour) - else iu + . set iuAccountStatus status + else + iu + & set iuAccountStatus status where version :: [Maybe (Writetime Name)] -> m IndexVersion version = mkIndexVersion . getMax . mconcat . fmap Max . catMaybes shouldIndex = and [ case status of - Just Active -> True Nothing -> True + Just Active -> True Just Suspended -> True Just Deleted -> False Just Ephemeral -> False, diff --git a/services/brig/src/Brig/User/Search/Index/Types.hs b/services/brig/src/Brig/User/Search/Index/Types.hs index 251c4f468bc..490b1e9746f 100644 --- a/services/brig/src/Brig/User/Search/Index/Types.hs +++ b/services/brig/src/Brig/User/Search/Index/Types.hs @@ -19,6 +19,7 @@ module Brig.User.Search.Index.Types where +import Brig.Types.Intra (AccountStatus) import Brig.Types.User import Control.Lens (makeLenses) import Control.Monad.Catch @@ -45,7 +46,8 @@ data IndexUser = IndexUser _iuTeam :: Maybe TeamId, _iuName :: Maybe Name, _iuHandle :: Maybe Handle, - _iuColourId :: Maybe ColourId + _iuColourId :: Maybe ColourId, + _iuAccountStatus :: Maybe AccountStatus } data IndexQuery r = IndexQuery Query Filter @@ -72,7 +74,8 @@ data UserDoc = UserDoc udName :: Maybe Name, udNormalized :: Maybe Text, udHandle :: Maybe Handle, - udColourId :: Maybe ColourId + udColourId :: Maybe ColourId, + udAccountStatus :: Maybe AccountStatus } deriving (Eq, Show) @@ -84,7 +87,8 @@ instance ToJSON UserDoc where "name" .= udName ud, "normalized" .= udNormalized ud, "handle" .= udHandle ud, - "accent_id" .= udColourId ud + "accent_id" .= udColourId ud, + "account_status" .= udAccountStatus ud ] instance FromJSON UserDoc where @@ -95,6 +99,7 @@ instance FromJSON UserDoc where <*> o .:? "normalized" <*> o .:? "handle" <*> o .:? "accent_id" + <*> o .:? "account_status" makeLenses ''IndexUser @@ -109,6 +114,7 @@ mkIndexUser u v = IndexUser { _iuUserId = u, _iuVersion = v, + _iuAccountStatus = Nothing, _iuTeam = Nothing, _iuName = Nothing, _iuHandle = Nothing, diff --git a/services/brig/test/unit/Test/Brig/User/Search/Index/Types.hs b/services/brig/test/unit/Test/Brig/User/Search/Index/Types.hs index 39b5f26cc01..a431006b14f 100644 --- a/services/brig/test/unit/Test/Brig/User/Search/Index/Types.hs +++ b/services/brig/test/unit/Test/Brig/User/Search/Index/Types.hs @@ -20,6 +20,7 @@ module Test.Brig.User.Search.Index.Types where import Brig.Types.Common +import Brig.Types.Intra (AccountStatus (..)) import Brig.User.Search.Index import Data.Aeson import Data.Handle @@ -39,7 +40,7 @@ tests = "failed" (eitherDecode' (encode userDoc1)) (Right userDoc1), - testCase "aeson half-roundtrip: UserDoc" $ + testCase "backwards comptibility test: UserDoc" $ assertEqual "failed" (toJSON userDoc1) @@ -59,14 +60,15 @@ userDoc1 = udName = Just . Name $ "Carl Phoomp", udNormalized = Just $ "carl phoomp", udHandle = Just . fromJust . parseHandle $ "phoompy", - udColourId = Just . ColourId $ 32 + udColourId = Just . ColourId $ 32, + udAccountStatus = Just Active } userDoc1Value :: Value userDoc1Value = fromJust (decode userDoc1ByteString) userDoc1ByteString :: LByteString -userDoc1ByteString = "{\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"handle\":\"phoompy\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\"}" +userDoc1ByteString = "{\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"handle\":\"phoompy\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\",\"account_status\":\"active\"}" indexUser1 :: IndexUser indexUser1 = @@ -76,5 +78,6 @@ indexUser1 = _iuTeam = udTeam userDoc1, _iuName = udName userDoc1, _iuHandle = udHandle userDoc1, - _iuColourId = udColourId userDoc1 + _iuColourId = udColourId userDoc1, + _iuAccountStatus = udAccountStatus userDoc1 } From 0050b9fb90d63c5a3bba598bc59e2fde335893f2 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Wed, 15 Jul 2020 16:18:15 +0200 Subject: [PATCH 3/9] Index/Types.hs: Move AccountStatus to last line for consistency --- services/brig/src/Brig/User/Search/Index/Types.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/brig/src/Brig/User/Search/Index/Types.hs b/services/brig/src/Brig/User/Search/Index/Types.hs index 490b1e9746f..3fd98eee288 100644 --- a/services/brig/src/Brig/User/Search/Index/Types.hs +++ b/services/brig/src/Brig/User/Search/Index/Types.hs @@ -114,9 +114,9 @@ mkIndexUser u v = IndexUser { _iuUserId = u, _iuVersion = v, - _iuAccountStatus = Nothing, _iuTeam = Nothing, _iuName = Nothing, _iuHandle = Nothing, - _iuColourId = Nothing + _iuColourId = Nothing, + _iuAccountStatus = Nothing } From f6e6b6629417bb07e46afc2233177f1a6dd724d7 Mon Sep 17 00:00:00 2001 From: Matthias Heinzel Date: Wed, 15 Jul 2020 17:06:01 +0200 Subject: [PATCH 4/9] simplify elasticsearch migrations --- services/brig/brig.cabal | 3 +- .../brig/index/src/Brig/Index/Migrations.hs | 30 +++++++++--------- .../index/src/Brig/Index/Migrations/Types.hs | 6 ---- .../V1_ReIndexForSearchTeamMembers.hs | 31 ------------------- 4 files changed, 16 insertions(+), 54 deletions(-) delete mode 100644 services/brig/index/src/Brig/Index/Migrations/V1_ReIndexForSearchTeamMembers.hs diff --git a/services/brig/brig.cabal b/services/brig/brig.cabal index 44b580ed515..9fe0cc99d48 100644 --- a/services/brig/brig.cabal +++ b/services/brig/brig.cabal @@ -4,7 +4,7 @@ cabal-version: 2.0 -- -- see: https://github.com/sol/hpack -- --- hash: 569c2e513825afb99305f83cb7146a6dd4c42b06b991923f534ad980a8bc8c2e +-- hash: f150b19d31eda4f6f32ad2782dccc7deae05b01ffd46558fe1f403ba49500307 name: brig version: 1.35.0 @@ -227,7 +227,6 @@ library brig-index-lib Brig.Index.Eval Brig.Index.Migrations Brig.Index.Migrations.Types - Brig.Index.Migrations.V1_ReIndexForSearchTeamMembers Brig.Index.Options Main other-modules: diff --git a/services/brig/index/src/Brig/Index/Migrations.hs b/services/brig/index/src/Brig/Index/Migrations.hs index e816966f3ba..56df13b0551 100644 --- a/services/brig/index/src/Brig/Index/Migrations.hs +++ b/services/brig/index/src/Brig/Index/Migrations.hs @@ -21,8 +21,8 @@ module Brig.Index.Migrations where import Brig.Index.Migrations.Types -import qualified Brig.Index.Migrations.V1_ReIndexForSearchTeamMembers as V1_ReIndexForSearchTeamMembers import qualified Brig.Index.Options as Opts +import qualified Brig.User.Search.Index as Search import qualified Cassandra as C import qualified Cassandra.Settings as C import Control.Lens ((^.), view) @@ -45,7 +45,11 @@ migrate l es cas = do runMigrationAction env $ do failIfIndexAbsent (es ^. Opts.esIndex) createMigrationsIndexIfNotPresent - runMigrations [V1_ReIndexForSearchTeamMembers.migration] + runMigration expectedMigrationVersion + +-- | Increase this number any time you want to force reindexing. +expectedMigrationVersion :: MigrationVersion +expectedMigrationVersion = MigrationVersion 1 indexName :: ES.IndexName indexName = ES.IndexName "wire_brig_migrations" @@ -105,20 +109,16 @@ failIfIndexAbsent targetIndex = (throwM $ TargetIndexAbsent targetIndex) -- | Runs only the migrations which need to run -runMigrations :: [Migration] -> MigrationActionT IO () -runMigrations migrations = do +runMigration :: MigrationVersion -> MigrationActionT IO () +runMigration ver = do vmax <- latestMigrationVersion - let pendingMigrations = filter (\m -> version m > vmax) migrations - if null pendingMigrations - then info "No new migrations." - else info "New migrations found." - mapM_ runMigration pendingMigrations - -runMigration :: Migration -> MigrationActionT IO () -runMigration (Migration ver txt mig) = do - info $ "Running: [" <> show (migrationVersion ver) <> "] " <> Text.unpack txt - mig - persistVersion ver + if ver > vmax + then do + info "Migration necessary." + Search.reindexAllIfSameOrNewer + persistVersion ver + else do + info "No migration necessary." persistVersion :: (Monad m, MonadThrow m, MonadIO m) => MigrationVersion -> MigrationActionT m () persistVersion v = diff --git a/services/brig/index/src/Brig/Index/Migrations/Types.hs b/services/brig/index/src/Brig/Index/Migrations/Types.hs index 8c026ab253f..33e404ff0be 100644 --- a/services/brig/index/src/Brig/Index/Migrations/Types.hs +++ b/services/brig/index/src/Brig/Index/Migrations/Types.hs @@ -32,12 +32,6 @@ import Numeric.Natural (Natural) import qualified System.Logger as Logger import System.Logger.Class (MonadLogger (..)) -data Migration = Migration - { version :: MigrationVersion, - text :: Text, - action :: MigrationActionT IO () - } - newtype MigrationVersion = MigrationVersion {migrationVersion :: Natural} deriving (Show, Eq, Ord) diff --git a/services/brig/index/src/Brig/Index/Migrations/V1_ReIndexForSearchTeamMembers.hs b/services/brig/index/src/Brig/Index/Migrations/V1_ReIndexForSearchTeamMembers.hs deleted file mode 100644 index b6b47d288c9..00000000000 --- a/services/brig/index/src/Brig/Index/Migrations/V1_ReIndexForSearchTeamMembers.hs +++ /dev/null @@ -1,31 +0,0 @@ --- This file is part of the Wire Server implementation. --- --- Copyright (C) 2020 Wire Swiss GmbH --- --- This program is free software: you can redistribute it and/or modify it under --- the terms of the GNU Affero General Public License as published by the Free --- Software Foundation, either version 3 of the License, or (at your option) any --- later version. --- --- This program is distributed in the hope that it will be useful, but WITHOUT --- ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS --- FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more --- details. --- --- You should have received a copy of the GNU Affero General Public License along --- with this program. If not, see . - -module Brig.Index.Migrations.V1_ReIndexForSearchTeamMembers - ( migration, - ) -where - -import qualified Brig.Index.Migrations.Types as Types -import qualified Brig.User.Search.Index as Search - -migration :: Types.Migration -migration = Types.Migration ver txt mig - where - ver = Types.MigrationVersion 1 - txt = "Reindex all users for search within team" - mig = Search.reindexAllIfSameOrNewer From 5857305eaac531efc2001fca4e083944d569c6bb Mon Sep 17 00:00:00 2001 From: Matthias Heinzel Date: Wed, 15 Jul 2020 17:10:56 +0200 Subject: [PATCH 5/9] force ES reindexing after adding account_status --- services/brig/index/src/Brig/Index/Migrations.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/brig/index/src/Brig/Index/Migrations.hs b/services/brig/index/src/Brig/Index/Migrations.hs index 56df13b0551..10987ddfaca 100644 --- a/services/brig/index/src/Brig/Index/Migrations.hs +++ b/services/brig/index/src/Brig/Index/Migrations.hs @@ -49,7 +49,7 @@ migrate l es cas = do -- | Increase this number any time you want to force reindexing. expectedMigrationVersion :: MigrationVersion -expectedMigrationVersion = MigrationVersion 1 +expectedMigrationVersion = MigrationVersion 2 indexName :: ES.IndexName indexName = ES.IndexName "wire_brig_migrations" From 929425615ab2a39cff640265a17066b117d0159c Mon Sep 17 00:00:00 2001 From: Matthias Heinzel Date: Thu, 16 Jul 2020 11:39:19 +0200 Subject: [PATCH 6/9] FUTUREWORK --- services/brig/src/Brig/User/Search/Index.hs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/brig/src/Brig/User/Search/Index.hs b/services/brig/src/Brig/User/Search/Index.hs index 64a79f07e99..29633c73a48 100644 --- a/services/brig/src/Brig/User/Search/Index.hs +++ b/services/brig/src/Brig/User/Search/Index.hs @@ -465,6 +465,9 @@ updateMapping = liftIndexIO $ do ex <- ES.indexExists idx unless ex $ throwM (IndexError "Index does not exist.") + -- FUTUREWORK: check return code (ES.isSuccess) and fail if appropriate. + -- But to do that we have to consider the consequences of this failing in our helm chart: + -- https://github.com/wireapp/wire-server-deploy/blob/92311d189818ffc5e26ff589f81b95c95de8722c/charts/elasticsearch-index/templates/create-index.yaml void $ traceES "Put mapping" $ ES.putMapping idx (ES.MappingName "user") indexMapping From a1e27c81bbb8352f1a3326195890eaeff5401ab2 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 16 Jul 2020 15:43:36 +0200 Subject: [PATCH 7/9] Ensure ES index is not dynamically adding fields --- services/brig/src/Brig/User/Search/Index.hs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/services/brig/src/Brig/User/Search/Index.hs b/services/brig/src/Brig/User/Search/Index.hs index 29633c73a48..2c21ddb2e7a 100644 --- a/services/brig/src/Brig/User/Search/Index.hs +++ b/services/brig/src/Brig/User/Search/Index.hs @@ -570,10 +570,19 @@ userDoc iu = -- searched tokens are not bigger than 30 characters by truncating them, this is -- necessary as our "prefix_index" analyzer only creates edge_ngrams until 30 -- characters. +-- +-- About the dynamic field: When this is not set and we add another field to our +-- user document, elasticsearch will try to guess how it is supposed to be indexed. +-- Changes to this require creating a new index and a cumbersome migration. So it is +-- important that we set this field to `false`. This will make new fields will just +-- not be indexed. After we decide what they should look like, we can just run a +-- reindex to make them usable. More info: +-- https://www.elastic.co/guide/en/elasticsearch/reference/7.7/dynamic.html indexMapping :: Value indexMapping = object - [ "properties" + [ "dynamic" .= False, + "properties" .= object [ "normalized" -- normalized user name .= MappingProperty From cbb90eb4e88959c738233387525838a96b0b5c13 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Thu, 16 Jul 2020 16:10:21 +0200 Subject: [PATCH 8/9] Add upgrade notes for next release --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cadf2e1d4d..86dd512c2d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +# Next Release + +## Release Notes + +* This release makes a couple of changes to the elasticsearch mapping and requires a data migration. The correct order of upgrade is: + 1. [Update mapping](./docs/reference/elastic-search.md#update-mapping) + 1. Upgrade brig as usual + 1. [Run data migration](./docs/reference/elastic-search.md#migrate-data) + Search should continue to work normally during this upgrade. + +## Bug Fixes + +* Fix member count of suspended teams in journal events (#1171) + # 2020-07-13 ## Release Notes From 8b1f1085d6e90052df97a4d326242e8bc2b7c57b Mon Sep 17 00:00:00 2001 From: Matthias Heinzel Date: Mon, 20 Jul 2020 11:09:11 +0200 Subject: [PATCH 9/9] add comments --- services/brig/src/Brig/User/Search/Index.hs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/services/brig/src/Brig/User/Search/Index.hs b/services/brig/src/Brig/User/Search/Index.hs index 2c21ddb2e7a..b3ba41883c0 100644 --- a/services/brig/src/Brig/User/Search/Index.hs +++ b/services/brig/src/Brig/User/Search/Index.hs @@ -256,7 +256,16 @@ mkUserQuery (review _TextId -> self) teamSearchInfo q = boolQuery { ES.boolQueryShouldMatch = [ termQ "account_status" "active", - ES.QueryBoolQuery boolQuery {ES.boolQueryMustNotMatch = [ES.QueryExistsQuery (ES.FieldName "account_status")]} -- TODO: Write note about this being legacy + -- Also match entries where the account_status field is not present. + -- These must have been inserted before we added the account_status + -- and at that time we only inserted active users in the first place. + -- This should be unnecessary after re-indexing, but let's be lenient + -- here for a while. + ES.QueryBoolQuery + boolQuery + { ES.boolQueryMustNotMatch = + [ES.QueryExistsQuery (ES.FieldName "account_status")] + } ] } ] @@ -781,6 +790,8 @@ reindexRowToIndexUser (u, mteam, name, t0, status, t1, handle, t2, colour, t4, a . set iuAccountStatus status else iu + -- We insert a tombstone-style user here, as it's easier than deleting the old one. + -- It's mostly empty, but having the status here might be useful in the future. & set iuAccountStatus status where version :: [Maybe (Writetime Name)] -> m IndexVersion