fix(process_updates): Fix expirations, cleanup syncing, and add integration tests

.envs/.local/.django

@@ -6,5 +6,5 @@ IPYTHONDIR=/app/.ipython
 # ------------------------------------------------------------------------------
 
 # Django
-DATABASE_URL=sqlite:///django.db
+DATABASE_URL=postgres://debug:debug@postgres:5432/scram
 DJANGO_SETTINGS_MODULE=config.settings.local

.github/workflows/ruff.yml

@@ -20,7 +20,7 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Install dependencies
-        run: pip install ruff
+        run: pip install ruff==0.14.10
 
       - name: Fail if we have any style errors
         run: ruff check --output-format=github

.pre-commit-config.yaml

@@ -1,5 +1,5 @@
 exclude: 'docs|migrations|.git|.tox'
-default_stages: [pre-commit]
+default_stages: [commit]
 fail_fast: true
 
 repos:
@@ -10,10 +10,8 @@ repos:
       - id: end-of-file-fixer
 
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.8.0
+    rev: v0.14.10
     hooks:
-      # Run the linter.
-      - id: ruff
-        args: [ --fix ]
-      # Run the formatter.
       - id: ruff-format
+      - id: ruff-check
+        args: [--fix, --exit-non-zero-on-fix]

Makefile

@@ -33,6 +33,11 @@ behave-all: compose.override.yml
 behave: compose.override.yml
 	@docker compose run --rm django python manage.py behave --no-input --simple -i $(FEATURE)
 
+## integration-tests: runs multi-instance system tests against docker compose running containers
+.Phony: integration-tests
+integration-tests: run
+	@docker compose exec -T django coverage run -a manage.py behave --no-input --use-existing-database scram/route_manager/tests/integration
+
 ## behave-translator
 .Phony: behave-translator
 behave-translator: compose.override.yml
@@ -46,11 +51,11 @@ build: compose.override.yml
 	@docker compose restart $(CONTAINER)
 
 ## coverage.xml: generate coverage from test runs
-coverage.xml: pytest behave-all behave-translator
+coverage.xml: pytest behave-all integration-tests behave-translator
 	@docker compose run --rm django coverage report
 	@docker compose run --rm django coverage xml
 
-## ci-test: runs all tests just like Gitlab CI does
+## ci-test: runs all tests just like Github CI does
 .Phony: ci-test
 ci-test: | toggle-local build migrate run coverage.xml
 

compose.override.local.yml

@@ -19,6 +19,7 @@ services:
       - ./.envs/.local/.postgres
     healthcheck:
       test: ["CMD", "curl", "-f", "http://django:8000/process_updates/"]
+      interval: 120s
     ports:
       - "8000"
       - 56780:56780
@@ -40,6 +41,7 @@ services:
       - ./.envs/.local/.postgres
     healthcheck:
       test: ["CMD", "curl", "-f", "http://django-secondary:8000/process_updates/"]
+      interval: 120s
     ports:
       - "8000"
       - 56781:56780

docs/decisions.md

@@ -79,11 +79,18 @@ If you want two or more instances of SCRAM to share data between themselves we h
 3. For normal syncing where both translators have been connected, we are currently using process_updates (since it runs 
    regularly) to grab new data out of the database that comes from other connected instances and reannounces those locally.
 
-Honestly, step 3 is kind of gross and we realize this. We are probably looking at a task runner or something to handle
-this moving forward, but we needed to get this fixed in the meantime. Status can be tracked with
-[Github Issue 125](https://github.com/esnet-security/SCRAM/issues/125) 
+##### The Unblocking Problem® (PR #157)
+
+We had a bug where entries that had expired or been deleted on one SCRAM instance were never being unblocked on other instances. The original sync logic only looked at the `when` field (creation time), so it would happily find new entries but completely miss any that were modified, expired, or de-activated after creation. This was bad because things could get stuck being blocked forever (until gobgp and translator are restarted) and not show up in the web UI (because the database says it's not blocked.) Eventually, we should add "ghost" routes to the UI to show someone if there is something that's advertised by its translator but not in the database. We could even use a cute little ghost icon for it!
+
+The fix takes advantage of the already existing `django-simple-history` models that track any entry modifications. To sync, we query the history models to see if something has changed in any way, and we just go ahead and re-send that to the translator, ensuring eventual consistency (since translator is idempotent).
+
+##### Future Work
+
+Honestly, the use of compose health checks is kind of gross and we realize this. The `process_updates` polling approach works, but it's not elegant. We're probably looking at Celery or some other task runner to handle this properly. it'd be good to have something that can react to database changes on a message bus rather than polling every 30 seconds. But this gets things fixed for now, and the history-based seems solid enough for our needs.
 
 #### Entries Page
+
 We intentionally chose to only list the active entries. Our thinking is that the home page shows the most recent additions.
 Then, if you went to the entries page, it would be overwhelmingly huge to show all the historical entries including the 
 ones that timed out/were deactivated. If you wanted to know about a specific entry even if it were not currently active

pyproject.toml

@@ -1,139 +1,141 @@
 # ==== pytest ====
 [tool.pytest.ini_options]
-minversion = "6.0"
 addopts = "--ds=config.settings.test --reuse-db"
+minversion = "6.0"
 python_files = [
-    "tests.py",
-    "test_*.py",
+  "tests.py",
+  "test_*.py",
 ]
 
 # ==== Coverage ====
 [tool.coverage.run]
+branch = true
+data_file = "coverage.coverage"
 include = ["scram/*", "config/*", "translator/*"]
 omit = ["**/migrations/*", "scram/contrib/*", "*/tests/*"]
 plugins = ["django_coverage_plugin"]
-branch = true
-data_file = "coverage.coverage"
 
 [tool.coverage.report]
 exclude_also = [
-    "if debug:",
-    "if self.debug:",
-    "if settings.DEBUG",
-    "raise AssertionError",
-    "raise NotImplementedError",
-    "if __name__ == .__main__.:",
-    ]
+  "if debug:",
+  "if self.debug:",
+  "if settings.DEBUG",
+  "raise AssertionError",
+  "raise NotImplementedError",
+  "if __name__ == .__main__.:",
+]
 
 # ===== ruff ====
 [tool.ruff]
 exclude = [
-    "migrations",
+  "migrations",
 ]
 
 line-length = 119
-target-version = 'py312'
 preview = true
+target-version = 'py312'
 
 [tool.ruff.lint]
-select = [
-    "A",     # builtins
-    "ASYNC", # async
-    "B",     # bugbear
-    "BLE",   # blind-except
-    "C4",    # comprehensions
-    "C90",   # complexity
-    "COM",   # commas
-    "D",     # pydocstyle
-    "DJ",    # django
-    "DOC",   # pydoclint
-    "DTZ",   # datetimez
-    "E",     # pycodestyle
-    "EM",    # errmsg
-    "ERA",   # eradicate
-    "F",     # pyflakes
-    "FBT",   # boolean-trap
-    "FLY",   # flynt
-    "G",     # logging-format
-    "I",     # isort
-    "ICN",   # import-conventions
-    "ISC",   # implicit-str-concat
-    "LOG",   # logging
-    "N",     # pep8-naming
-    "PERF",  # perflint
-    "PIE",   # pie
-    "PL",    # pylint
-    "PTH",   # use-pathlib
-    "Q",     # quotes
-    "RET",   # return
-    "RSE",   # raise
-    "RUF",   # Ruff
-    "S",     # bandit
-    "SIM",   # simplify
-    "SLF",   # self
-    "SLOT",  # slots
-    "T20",   # print
-    "TRY",   # tryceratops
-    "UP",    # pyupgrade
-]
 ignore = [
-    "COM812", # handled by the formatter
-    "DOC501", # add possible exceptions to the docstring (TODO)
-    "ISC001", # handled by the formatter
-    "RUF012", # need more widespread typing
-    "SIM102", # use a single `if` statement instead of nested `if` statements
-    "SIM108", # use ternary operator instead of `if`-`else`-block
-    "PERF401", # list comprehensions are harder to read in our opinion and not worth the performance gain
-    "PERF403", # dict comprehensions are harder to read in our opinion and not worth the performance gain
+  "COM812", # handled by the formatter
+  "DOC501", # add possible exceptions to the docstring (TODO)
+  "ISC001", # handled by the formatter
+  "RUF012", # need more widespread typing
+  "SIM102", # use a single `if` statement instead of nested `if` statements
+  "SIM108", # use ternary operator instead of `if`-`else`-block
+  "PERF401", # list comprehensions are harder to read in our opinion and not worth the performance gain
+  "PERF403", # dict comprehensions are harder to read in our opinion and not worth the performance gain
+]
+select = [
+  "A", # builtins
+  "ASYNC", # async
+  "B", # bugbear
+  "BLE", # blind-except
+  "C4", # comprehensions
+  "C90", # complexity
+  "COM", # commas
+  "D", # pydocstyle
+  "DJ", # django
+  "DOC", # pydoclint
+  "DTZ", # datetimez
+  "E", # pycodestyle
+  "EM", # errmsg
+  "ERA", # eradicate
+  "F", # pyflakes
+  "FBT", # boolean-trap
+  "FLY", # flynt
+  "G", # logging-format
+  "I", # isort
+  "ICN", # import-conventions
+  "ISC", # implicit-str-concat
+  "LOG", # logging
+  "N", # pep8-naming
+  "PERF", # perflint
+  "PIE", # pie
+  "PL", # pylint
+  "PTH", # use-pathlib
+  "Q", # quotes
+  "RET", # return
+  "RSE", # raise
+  "RUF", # Ruff
+  "S", # bandit
+  "SIM", # simplify
+  "SLF", # self
+  "SLOT", # slots
+  "T20", # print
+  "TRY", # tryceratops
+  "UP", # pyupgrade
 ]
 
 [tool.ruff.lint.mccabe]
 max-complexity = 7 # our current code adheres to this without too much effort
 
 [tool.ruff.lint.per-file-ignores]
-"**/{tests}/*" = [
-    "DOC201", # documenting return values
-    "DOC402", # documenting yield values
-    "PLR6301", # could be a static method
-    "S101", # use of assert
-    "S106", # hardcoded password
-    "PLR2004" # magic value used in comparison
+"**/tests/**" = [
+  "C901", # function is too complex (applies to test helpers)
+  "DOC201", # documenting return values
+  "DOC402", # documenting yield values
+  "FBT002", # boolean default argument in function definition
+  "PLR6301", # could be a static method
+  "S101", # use of assert
+  "S106", # hardcoded password
+  "PLR2004", # magic value used in comparison
 ]
-"test.py" = [
-    "S105", # hardcoded password as argument
+"**/views.py" = [
+  "DOC201", # documenting return values; it's fairly obvious in a View
 ]
 "scram/route_manager/**" = [
-    "DOC201", # documenting return values
+  "DOC201", # documenting return values
 ]
 "scram/users/**" = [
-    "DOC201", # documenting return values
-    "FBT001", # minimal issue; don't need to mess with in the User app
-    "PLR2004", # magic values when checking HTTP status codes
+  "DOC201", # documenting return values
+  "FBT001", # minimal issue; don't need to mess with in the User app
+  "PLR2004", # magic values when checking HTTP status codes
 ]
-"**/views.py" = [
-    "DOC201", # documenting return values; it's fairly obvious in a View
+"test.py" = [
+  "S105", # hardcoded password as argument
 ]
 
 [tool.ruff.lint.pydocstyle]
 convention = "google"
 
 # ==== mypy ====
 [tool.mypy]
-python_version = "3.11"
 check_untyped_defs = true
 ignore_missing_imports = true
-warn_unused_ignores = true
-warn_redundant_casts = true
-warn_unused_configs = true
 plugins = [
-    "mypy_django_plugin.main",
-    "mypy_drf_plugin.main",
+  "mypy_django_plugin.main",
+  "mypy_drf_plugin.main",
 ]
+python_version = "3.11"
+warn_redundant_casts = true
+warn_unused_configs = true
+warn_unused_ignores = true
 
 [[tool.mypy.overrides]]
 # Django migrations should not produce any errors:
-module = "*.migrations.*"
 ignore_errors = true
+module = "*.migrations.*"
 
 [tool.django-stubs]
 django_settings_module = "config.settings.test"