SQL refactor #287

DJAndries · 2024-09-07T01:32:18Z

Resolves #216

…of items upon update, create DB helpers, add godotenv

…unts accordingly

AlexeyBarabash

LGTM, great work

I left one question at spec discussion.

Tried locally - works fine, I saw TRANSIENT_ERROR/SQL rollout not ready - but then client status turned to SUCCESS.

command/helpers.go

github-actions · 2024-10-09T20:29:44Z

docker-compose.yml

@@ -70,3 +75,14 @@ services:
      - ALLOW_EMPTY_PASSWORD=yes
    networks:
      - sync
+  postgres:


_{reported by reviewdog 🐶}
[semgrep] Service 'postgres' allows for privilege escalation via setuid or setgid binaries. Add 'no-new-privileges=true' in 'security_opt' to prevent this.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/services/docker-compose-no-new-privileges.yaml

Cc @thypon kdenhartog

The combination of this one with the additional one below do look like they'll present issues. Should be an easy fix to add the no-new-privileges=true configuration to this file. If there's a reason we intentionally haven't done this though @DJAndries let me know and we can determine together what the best way forward is.

github-actions · 2024-10-09T20:29:44Z

docker-compose.yml

@@ -70,3 +75,14 @@ services:
      - ALLOW_EMPTY_PASSWORD=yes
    networks:
      - sync
+  postgres:


_{reported by reviewdog 🐶}
[semgrep] Service 'postgres' is running with a writable root filesystem. This may allow malicious applications to download and run additional payloads, or modify container files. If an application inside a container has to save something temporarily consider using a tmpfs. Add 'read_only: true' to this service to prevent this.

Source: https://semgrep.dev/r/yaml.docker-compose.security.writable-filesystem-service.writable-filesystem-service

Cc @thypon kdenhartog

Same as above, seems like a simple solution to solve this is to add the read_only: true configuration. If you can't let me know and we'll figure out the best way forward @DJAndries.

kdenhartog · 2024-10-09T23:49:55Z

command/helpers.go

+	if !h.SQLDB.Variations().Ready {
+		return nil, nil
+	}
+	if rand.Float32() > h.SQLDB.MigrateIntervalPercent() {


non blocking question: @DJAndries what's the purpose in this validation check? The logic is non-deterministic in the sense that rand could return a very large number forcing this to fail the migration. Is this intentional?

to reduce DB load, we only want to migrate chunks of data from ddb to pg on a certain percentage of "get update" requests from the client, which are sent every 30 seconds. the check enforces this requirement.

will add a comment for clarity

If you want a more uniform randomness, crypto/rand can provide that guarantee, but based on that answer it doesn't seem like we really need that so feel free to ignore that sec-actions comment.

github-actions · 2024-10-10T00:42:59Z

command/helpers.go

+
+import (
+	"fmt"
+	"math/rand/v2"


_{reported by reviewdog 🐶}
[semgrep] Do not use math/rand. Use crypto/rand instead.

Source: https://semgrep.dev/r/go.lang.security.audit.crypto.math_random.math-random-used

Cc @thypon @kdenhartog

pg.Dockerfile

@@ -0,0 +1,9 @@
+FROM public.ecr.aws/docker/library/postgres:16
+
+RUN apt update && apt install -y git make gcc postgresql-server-dev-16


github-actions · 2024-10-22T03:37:21Z

[puLL-Merge] - brave/go-sync@287

Description

This PR introduces significant changes to the codebase, primarily focusing on adding support for SQL databases alongside the existing DynamoDB implementation. It also includes improvements to the data migration process, updates to the project structure, and enhancements to the testing suite.

Changes

Changes

Database Support:
- Added SQL database support (datastore/sql.go, datastore/sync_entity_sql.go)
- Created interfaces for DynamoDB and SQL datastores (datastore/interfaces.go)
- Implemented migration functionality from DynamoDB to SQL (command/migrate_test.go)
Project Structure:
- Separated DynamoDB-specific code (datastore/sync_entity_dynamo.go)
- Added SQL migrations (datastore/migrations/)
- Created new files for SQL-related functionality (datastore/sql_variations.go, datastore/item_count_sql.go)
Command Package:
- Refactored command/command.go to support both DynamoDB and SQL operations
- Added new helper functions in command/helpers.go
- Implemented item count tracking (command/item_count.go)
Testing:
- Updated existing tests to work with both DynamoDB and SQL
- Added new tests for SQL functionality and migrations
Configuration and Environment:
- Added support for SQL database connection strings
- Introduced new environment variables for SQL configuration
Docker and Development:
- Updated docker-compose.yml to include PostgreSQL service
- Added pg.Dockerfile for custom PostgreSQL image with extensions
Dependencies:
- Updated go.mod and go.sum with new dependencies, including SQL-related libraries
Rollout Management:
- Implemented rollout configuration management (server/rollout.go)

Possible Issues

The migration process from DynamoDB to SQL might cause temporary inconsistencies during the transition period.
The new SQL implementation might introduce performance differences compared to the previous DynamoDB-only setup.
The additional complexity of supporting both databases could make future maintenance more challenging.

Security Hotspots

SQL query construction in datastore/sync_entity_sql.go should be carefully reviewed to prevent SQL injection vulnerabilities.
The new database connection strings and credentials management in datastore/sql.go should be audited for proper security practices.
The rollout confirmation mechanism in server/rollout.go should be reviewed to ensure it doesn't introduce any potential security weaknesses.

DJAndries added 2 commits September 5, 2024 21:22

SQL refactor POC

7069f40

Various changes to SQL POC

5db0339

mihaiplesa requested a review from mschfh September 10, 2024 17:11

DJAndries added 13 commits September 10, 2024 21:03

Add variations for rolling out SQL to percentage of users, migration …

88ddc92

…of items upon update, create DB helpers, add godotenv

Add chunked migration from dynamo to SQL, add Prometheus metrics for SQL

94f28dd

Misc SQL fixes, insert server defined entities in SQL if applicable

92f4000

Fix SQL history update issue, add clear server data support for SQL

c247b64

Add rollout confirmation/waiting to reduce risk of conflicts

10af175

Embed SQL migrations, fix existing tests

8c55bac

Fix lint errors

430249a

Restore deleted return value from update functions, to adjust item co…

0c7f2d9

…unts accordingly

Add additional sync entity tests

f15ac94

Run command test suite for both Dynamo and SQL

69d56f9

Add item count test

9d5f4fe

Add migration and SQL variations test, improve other tests

fa45895

Update SQL related comments

efdf198

DJAndries marked this pull request as ready for review September 21, 2024 00:04

DJAndries requested review from fmarier, yrliou and AlexeyBarabash September 21, 2024 00:14

AlexeyBarabash approved these changes Oct 1, 2024

View reviewed changes

Reorder columns for data alignment

1562893

github-actions bot added the puLL-Merge label Oct 9, 2024

github-actions bot reviewed Oct 9, 2024

View reviewed changes

github-actions bot added the needs-security-review label Oct 9, 2024

github-actions bot assigned kdenhartog and thypon Oct 9, 2024

kdenhartog reviewed Oct 9, 2024

View reviewed changes

kdenhartog removed the needs-security-review label Oct 10, 2024

Add partitioning

c751555

github-actions bot reviewed Oct 10, 2024

View reviewed changes

github-advanced-security bot found potential problems Oct 17, 2024

View reviewed changes

DJAndries force-pushed the sql branch from 03361ec to e0b98cc Compare October 17, 2024 23:48

Add RDS IAM authentication support

61bcf21

DJAndries force-pushed the sql branch from e0b98cc to 61bcf21 Compare October 18, 2024 03:55

Use different method for refreshing IAM token

74abc2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL refactor #287

SQL refactor #287

DJAndries commented Sep 7, 2024 •

edited

Loading

AlexeyBarabash left a comment

github-actions bot Oct 9, 2024

kdenhartog Oct 9, 2024

github-actions bot Oct 9, 2024

kdenhartog Oct 9, 2024

kdenhartog Oct 9, 2024 •

edited

Loading

DJAndries Oct 10, 2024

DJAndries Oct 10, 2024

kdenhartog Oct 10, 2024

github-actions bot Oct 10, 2024

github-actions bot commented Oct 22, 2024

Changes

		@@ -0,0 +1,9 @@
		FROM public.ecr.aws/docker/library/postgres:16

		RUN apt update && apt install -y git make gcc postgresql-server-dev-16

SQL refactor #287

Are you sure you want to change the base?

SQL refactor #287

Conversation

DJAndries commented Sep 7, 2024 • edited Loading

AlexeyBarabash left a comment

Choose a reason for hiding this comment

github-actions bot Oct 9, 2024

Choose a reason for hiding this comment

kdenhartog Oct 9, 2024

Choose a reason for hiding this comment

github-actions bot Oct 9, 2024

Choose a reason for hiding this comment

kdenhartog Oct 9, 2024

Choose a reason for hiding this comment

kdenhartog Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

DJAndries Oct 10, 2024

Choose a reason for hiding this comment

DJAndries Oct 10, 2024

Choose a reason for hiding this comment

kdenhartog Oct 10, 2024

Choose a reason for hiding this comment

github-actions bot Oct 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2024

Description

Changes

Possible Issues

Security Hotspots

DJAndries commented Sep 7, 2024 •

edited

Loading

kdenhartog Oct 9, 2024 •

edited

Loading