Page MenuHomePhabricator
Differential D3177

[services] Backup - Fix primary key logic
ClosedPublic
Actions

Authored by • karol on Feb 11 2022, 3:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 17, 7:48 PM
Unknown Object (File)
Thu, Apr 17, 12:15 PM
Unknown Object (File)
Thu, Apr 17, 12:12 PM
Unknown Object (File)
Thu, Apr 17, 10:11 AM
Unknown Object (File)
Thu, Apr 17, 8:34 AM
Unknown Object (File)
Thu, Apr 17, 8:20 AM
Unknown Object (File)
Thu, Apr 17, 8:13 AM
Unknown Object (File)
Thu, Apr 17, 7:57 AM
View All Files
Subscribers
• adrian
ashoat
atul
• benschac
tomek

Details

Reviewers
tomek
• max
varun
ashoat
• jim
Commits
rCOMM67794813a6fa: [services] Backup - Fix primary key logic
Summary

Based on my research about the dynamo DB: https://phabricator.ashoat.com/D2950#81654

  • Every DynamoDB table has a unique primary key
  • A primary key [PKSK] must be composed of a partition key [PK], and can optionally have a sort key [SK].
  • A GetItem request returns one and only one item using its unique primary key [PKSK].
  • A Query does a fast lookup and must specify one and only one partition key [PK]. It can return multiple items.
  • A Scan evaluates every item in a table and may return a subset based on filter parameters. Scans are the correct choice in some circumstances but can be slow and costly if used incorrectly.

Depends on D3176

Test Plan

Backup service still builds

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• karol created this revision.Feb 11 2022, 3:41 AM
Herald added subscribers: atul, • adrian, tomek, ashoat. · View Herald TranscriptFeb 11 2022, 3:41 AM
• karol edited the summary of this revision. (Show Details)Feb 11 2022, 3:47 AM
• karol edited the test plan for this revision. (Show Details)
• karol added reviewers: tomek, • max, varun.
• karol edited the summary of this revision. (Show Details)
• karol added a parent revision: D3176: [services] Backup - Add DB tests for Logs.
• karol added a child revision: D3178: [services] Backup - Reuse remove logic.
Harbormaster completed remote builds in B6745: Diff 9562.Feb 11 2022, 3:53 AM
• karol requested review of this revision.Feb 11 2022, 3:53 AM
• karol mentioned this in D3179: [services] Blob - Fix logic for primary key.Feb 11 2022, 3:53 AM
• karol mentioned this in D3175: [services] Backup - Add DB operations for Logs.Feb 11 2022, 5:41 AM
tomek requested changes to this revision.Feb 11 2022, 7:43 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

It's ok to be explicit here and in PrimaryKey class, but we can consider making PrimaryKey an aggregate https://en.cppreference.com/w/cpp/language/aggregate_initialization. The consequences would be that here we could simply return {BackupItem::FIELD_USER_ID, BackupItem::FIELD_CREATED}.
The PrimaryKey can then be simply

struct PrimaryKey {
  const std::string partitionKey;
  std::unique_ptr<std::string> sortKey = nullptr;
};

What do you think about this idea?

services/backup/docker-server/contents/server/src/DatabaseManager.cpp
36–38 ↗(On Diff #9562)

We probably need to consider the whole primary key here, not only the partition

79–80 ↗(On Diff #9562)

Not sure, maybe we should explicitly use userID column here instead of the partition key. We want to find an item for a user and the coincidence that this is also a partition key should be an implementation detail at this point

146–147 ↗(On Diff #9562)

The same here. Do you think it makes sense to use user id column here explicitly?

This revision now requires changes to proceed.Feb 11 2022, 7:43 AM
ashoat requested changes to this revision.Feb 13 2022, 8:45 PM
ashoat added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

See feedback in D3160... I'm concerned that with this database design, it may not be possible to efficiently look up a backup based on BACKUP_ID, which is a requirement of the current architecture design

Herald added a subscriber: • benschac. · View Herald TranscriptFeb 13 2022, 8:45 PM
• karol added inline comments.Feb 14 2022, 3:28 AM
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

The consequences would be that here we could simply return {BackupItem::FIELD_USER_ID, BackupItem::FIELD_CREATED}.

That's not going to work. We'd have to manually call make_unique. So the current code lets you simply call a constructor with two strings. On the other hand, we could consider an empty string as a lack of sortKey . All these solutions are valid and more or less equal. I can change to something else although I cannot see any significant gain from doing so.

Ashoat - see my comments in D3160

services/backup/docker-server/contents/server/src/DatabaseManager.cpp
36–38 ↗(On Diff #9562)

That's a part of this solution, I just broke it down into two separate diffs, this one and D3178

79–80 ↗(On Diff #9562)

Yes, well, I'm not sure about this either... You're probably right, I'm gonna change this.

• karol updated this revision to Diff 9599.Feb 14 2022, 3:40 AM

update

Harbormaster completed remote builds in B6777: Diff 9599.Feb 14 2022, 3:45 AM
• karol planned changes to this revision.Feb 14 2022, 4:30 AM

changes upcoming after https://phabricator.ashoat.com/D3178#84511

• karol updated this revision to Diff 9603.Feb 14 2022, 4:31 AM

changes after https://phabricator.ashoat.com/D3178#84511

Harbormaster completed remote builds in B6781: Diff 9603.Feb 14 2022, 4:35 AM
tomek accepted this revision.Feb 14 2022, 6:18 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp
79 ↗(On Diff #9562)

That's not going to work. We'd have to manually call make_unique. So the current code lets you simply call a constructor with two strings. On the other hand, we could consider an empty string as a lack of sortKey . All these solutions are valid and more or less equal. I can change to something else although I cannot see any significant gain from doing so.

Ok, that makes sense. If we need to use make_unique the simplification isn't that significant - let's keep your approach

services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

What is the purpose of this statement? What happens when we don't have it?

ashoat accepted this revision.Feb 14 2022, 8:58 PM
ashoat added a reviewer: • jim.
This revision is now accepted and ready to land.Feb 14 2022, 8:58 PM
• karol updated this revision to Diff 9658.Feb 15 2022, 4:36 AM

update

• karol requested review of this revision.Feb 15 2022, 4:39 AM
• karol added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

This is a constructor inheritance feature. It allows us to use constructors from the base class in the children classes.

Harbormaster failed remote builds in B6819: Diff 9658!Feb 15 2022, 4:51 AM
• karol planned changes to this revision.Feb 15 2022, 5:16 AM
• karol updated this revision to Diff 9660.Feb 15 2022, 5:16 AM

update

Harbormaster failed remote builds in B6821: Diff 9660!Feb 15 2022, 5:18 AM
ashoat accepted this revision.Feb 15 2022, 7:41 PM
This revision is now accepted and ready to land.Feb 15 2022, 7:41 PM
tomek accepted this revision.Feb 16 2022, 2:45 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h
30 ↗(On Diff #9603)

Thanks!

• karol updated this revision to Diff 9733.Feb 16 2022, 7:29 AM

ci

Harbormaster completed remote builds in B6884: Diff 9733.Feb 16 2022, 7:35 AM
Closed by commit rCOMM67794813a6fa: [services] Backup - Fix primary key logic (authored by • karol). · Explain WhyMar 4 2022, 4:05 AM
This revision was automatically updated to reflect the committed changes.
• karol added a commit: rCOMM67794813a6fa: [services] Backup - Fix primary key logic.
• karol mentioned this in rCOMM4823d46d6df2: [services] Blob - Fix logic for primary key.

Revision Contents
Changeset List

PathSize
services/
backup/
docker-server/
contents/
server/
src/
DatabaseEntities/
2 lines
4 lines
16 lines
2 lines
4 lines
7 lines

Diff 9599

View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.h

  • This file was added.
// @flow
import olm from '@matrix-org/olm';
import fs from 'fs';
import path from 'path';
import uuid from 'uuid';
import { main } from './utils';
const olmConfigRelativePath = './secrets/olm_config.json';
async function generateOlmConfig() {
await olm.init();
const account = new olm.Account();
account.create();
const picklingKey = uuid.v4();
tomekUnsubmitted
Not Done
Inline Actions

It was discussed during a meeting that we do not expect pickling key to provide additional security layer so

I'd feel safer if this pickling key was less deterministic, but on the other hand I'm not sure if it is possible for someone to acquire only the pickled account without the key. If an attacker has both, then key security doesn't matter. So we can probably keep the current approach.

tomek: > It was discussed during a meeting that we do not expect pickling key to provide additional…
const pickledAccount = account.pickle(picklingKey);
const olmConfig = {
picklingKey: picklingKey,
pickledAccount: pickledAccount,
};
const scriptWorkingDirectory = path.resolve();
if (!scriptWorkingDirectory.endsWith('comm/server')) {
throw new Error('Script must be run in server directory in comm project.');
}
const olmConfigFilePath = path.join(
scriptWorkingDirectory,
olmConfigRelativePath,
);
fs.writeFileSync(olmConfigFilePath, JSON.stringify(olmConfig));
}
main([generateOlmConfig]);
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/BackupItem.cpp

  • This file was added.
// @flow
import olm from '@matrix-org/olm';
import fs from 'fs';
import path from 'path';
import uuid from 'uuid';
import { main } from './utils';
const olmConfigRelativePath = './secrets/olm_config.json';
async function generateOlmConfig() {
await olm.init();
const account = new olm.Account();
account.create();
const picklingKey = uuid.v4();
tomekUnsubmitted
Not Done
Inline Actions

It was discussed during a meeting that we do not expect pickling key to provide additional security layer so

I'd feel safer if this pickling key was less deterministic, but on the other hand I'm not sure if it is possible for someone to acquire only the pickled account without the key. If an attacker has both, then key security doesn't matter. So we can probably keep the current approach.

tomek: > It was discussed during a meeting that we do not expect pickling key to provide additional…
const pickledAccount = account.pickle(picklingKey);
const olmConfig = {
picklingKey: picklingKey,
pickledAccount: pickledAccount,
};
const scriptWorkingDirectory = path.resolve();
if (!scriptWorkingDirectory.endsWith('comm/server')) {
throw new Error('Script must be run in server directory in comm project.');
}
const olmConfigFilePath = path.join(
scriptWorkingDirectory,
olmConfigRelativePath,
);
fs.writeFileSync(olmConfigFilePath, JSON.stringify(olmConfig));
}
main([generateOlmConfig]);
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/Item.h

  • This file was added.
// @flow
import olm from '@matrix-org/olm';
import fs from 'fs';
import path from 'path';
import uuid from 'uuid';
import { main } from './utils';
const olmConfigRelativePath = './secrets/olm_config.json';
async function generateOlmConfig() {
await olm.init();
const account = new olm.Account();
account.create();
const picklingKey = uuid.v4();
tomekUnsubmitted
Not Done
Inline Actions

It was discussed during a meeting that we do not expect pickling key to provide additional security layer so

I'd feel safer if this pickling key was less deterministic, but on the other hand I'm not sure if it is possible for someone to acquire only the pickled account without the key. If an attacker has both, then key security doesn't matter. So we can probably keep the current approach.

tomek: > It was discussed during a meeting that we do not expect pickling key to provide additional…
const pickledAccount = account.pickle(picklingKey);
const olmConfig = {
picklingKey: picklingKey,
pickledAccount: pickledAccount,
};
const scriptWorkingDirectory = path.resolve();
if (!scriptWorkingDirectory.endsWith('comm/server')) {
throw new Error('Script must be run in server directory in comm project.');
}
const olmConfigFilePath = path.join(
scriptWorkingDirectory,
olmConfigRelativePath,
);
fs.writeFileSync(olmConfigFilePath, JSON.stringify(olmConfig));
}
main([generateOlmConfig]);
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h

  • This file was added.
// @flow
import olm from '@matrix-org/olm';
import fs from 'fs';
import path from 'path';
import uuid from 'uuid';
import { main } from './utils';
const olmConfigRelativePath = './secrets/olm_config.json';
async function generateOlmConfig() {
await olm.init();
const account = new olm.Account();
account.create();
const picklingKey = uuid.v4();
tomekUnsubmitted
Not Done
Inline Actions

It was discussed during a meeting that we do not expect pickling key to provide additional security layer so

I'd feel safer if this pickling key was less deterministic, but on the other hand I'm not sure if it is possible for someone to acquire only the pickled account without the key. If an attacker has both, then key security doesn't matter. So we can probably keep the current approach.

tomek: > It was discussed during a meeting that we do not expect pickling key to provide additional…
const pickledAccount = account.pickle(picklingKey);
const olmConfig = {
picklingKey: picklingKey,
pickledAccount: pickledAccount,
};
const scriptWorkingDirectory = path.resolve();
if (!scriptWorkingDirectory.endsWith('comm/server')) {
throw new Error('Script must be run in server directory in comm project.');
}
const olmConfigFilePath = path.join(
scriptWorkingDirectory,
olmConfigRelativePath,
);
fs.writeFileSync(olmConfigFilePath, JSON.stringify(olmConfig));
}
main([generateOlmConfig]);
View Options

services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp

Loading...
View Options

services/backup/docker-server/contents/server/src/DatabaseManager.cpp

Loading...