Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Adds celery==5.2.7 #610

Merged
merged 16 commits into from
Jun 10, 2022
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ The types of changes are:
* Subject Request Details page [#563](https://github.com/ethyca/fidesops/pull/563)
* Restart Graph from Failure [#578](https://github.com/ethyca/fidesops/pull/578)
* Redis SSL Support [#611](https://github.com/ethyca/fidesops/pull/611)
* Celery as a dependency for use in the execution layer [#610](https://github.com/ethyca/fidesops/pull/610)
* Cache and Surface Resume/Restart Instructions [#591](https://github.com/ethyca/fidesops/pull/591)
Comment on lines 23 to 27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog is looking out of date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything specific you can see that's missing here? Merging in main to this branch isn't yielding any conflicts or extra lines.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought 1.5.3 had already been released -

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chatted in person, my mistake, 1.5.3 was released from a different branch


### Changed

* Refactor auth and enable static file serving [#577](https://github.com/ethyca/fidesops/pull/577)


## [1.5.2](https://github.com/ethyca/fidesops/compare/1.5.1...1.5.2)

### Added
Expand Down
2 changes: 2 additions & 0 deletions data/config/fidesops.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ DRP_JWT_SECRET = "testdrpsecret"
LOG_LEVEL = "DEBUG"

[execution]
CELERY_BROKER_URL = "redis://:testpassword@redis:6379"
CELERY_RESULT_BACKEND = "redis://:testpassword@redis:6379"
Comment on lines +29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this, is this other toml file just used for tests? Can we add a code comment about what this file is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also surprised to find this. Yes this file is just used in one test to check we can load the config in from a different path. The catch is that because of the way we validate configs, we need all the required vars in here (lest it throws a ValidationError as it should). Hence it looking the same as our actual config file.

Will add a comment 👍

TASK_RETRY_COUNT = 0
TASK_RETRY_DELAY = 1
TASK_RETRY_BACKOFF = 1
Expand Down
21 changes: 20 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ services:
expose:
- 8080
healthcheck:
test: ["CMD", "curl", "-f", "http://0.0.0.0:8080/health"]
test: [ "CMD", "curl", "-f", "http://0.0.0.0:8080/health" ]
interval: 30s
timeout: 10s
retries: 3
Expand Down Expand Up @@ -58,6 +58,25 @@ services:
ports:
- "0.0.0.0:6379:6379"

celery:
build:
context: .
dockerfile: Dockerfile
command: celery -A fidesops.tasks beat -l info --logfile=/var/log/celery.log
depends_on:
fidesops:
condition: service_healthy
db:
condition: service_started
redis:
condition: service_started
restart: always
volumes:
- type: bind
source: ./
target: /fidesops
read_only: False

docs:
build:
context: docs/fidesops/
Expand Down
7 changes: 5 additions & 2 deletions fidesops.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ DRP_JWT_SECRET = "secret"
LOG_LEVEL = "INFO"

[execution]
CELERY_BROKER_URL = "redis://:testpassword@redis:6379"
CELERY_RESULT_BACKEND = "redis://:testpassword@redis:6379"
MASKING_STRICT = true
REQUIRE_MANUAL_REQUEST_APPROVAL = false
TASK_RETRY_COUNT = 0
TASK_RETRY_DELAY = 1
TASK_RETRY_BACKOFF = 1
REQUIRE_MANUAL_REQUEST_APPROVAL = false
MASKING_STRICT = true

[root_user]
ANALYTICS_OPT_OUT = false
ANALYTICS_ID = "89187e1f107ebe56c35e4f864af83a90"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @eastandwestwind, as far as I'm aware we don't want to commit this correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, we don't want this committed. I'll update such that we don't generate it if we have the ENV var set in this ticket- #559

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @seanpreston just checking, do you have the env var FIDESOPS__ROOT_USER__ANALYTICS_ID set to internal? We should already skip writing the analytics_id for internal users, but lemme know if if still generates the id after you set the var.

1 change: 1 addition & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
alembic==1.6.5
APScheduler==3.8.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me we were using celery back in the day and switched to apscheduler for some onetrust things (we also use it for webhook cleanup tasks too). Nothing to change here, just noting we might revert back to celery one day here to consolidate dependencies.

Copy link
Contributor Author

@seanpreston seanpreston Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this too, though it seemed like something we can save for another increment.

There's also a serious discussion for us to have around ripping the Onetrust integration out entirely, since it's not used. This came up while removing PrivacyRequestRunner.submit() calls in the next PR, since we can't effectively test the Onetrust integration.

boto3~=1.18.14
celery[pytest]==5.2.7
click==8.1.3
cryptography~=3.4.8
dask==2021.10.0
Expand Down
2 changes: 2 additions & 0 deletions src/fidesops/core/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class ExecutionSettings(FidesSettings):
TASK_RETRY_BACKOFF: int
REQUIRE_MANUAL_REQUEST_APPROVAL: bool = False
MASKING_STRICT: bool = True
CELERY_BROKER_URL: str
CELERY_RESULT_BACKEND: str

class Config:
env_prefix = "FIDESOPS__EXECUTION__"
Expand Down
9 changes: 9 additions & 0 deletions src/fidesops/tasks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from celery import Celery

app = Celery("tasks")
app.config_from_object("fidesops.core.config", namespace="EXECUTION")
app.autodiscover_tasks(["fidesops.tasks", "fidesops.tasks.scheduled"])


if __name__ == "__main__":
app.worker_main()
6 changes: 6 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,9 @@ def _build_jwt(webhook: PolicyPreWebhook) -> Dict[str, str]:
@pytest.fixture(scope="session")
def integration_config() -> MutableMapping[str, Any]:
yield load_toml("fidesops-integration.toml")


@pytest.fixture(autouse=True, scope="session")
def celery_enable_logging():
"""Turns on celery output logs."""
return True
73 changes: 45 additions & 28 deletions tests/fixtures/application_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -744,42 +744,59 @@ def privacy_requests(db: Session, policy: Policy) -> Generator:
def _create_privacy_request_for_policy(
db: Session,
policy: Policy,
status: PrivacyRequestStatus = PrivacyRequestStatus.in_processing,
) -> PrivacyRequest:
data = {
"external_id": f"ext-{str(uuid4())}",
"requested_at": datetime(
2018,
12,
31,
hour=2,
minute=30,
second=23,
microsecond=916482,
tzinfo=timezone.utc,
),
"status": status,
"origin": f"https://example.com/",
"policy_id": policy.id,
"client_id": policy.client_id,
}
if status != PrivacyRequestStatus.pending:
data["started_processing_at"] = datetime(
2019,
1,
1,
hour=1,
minute=45,
second=55,
microsecond=393185,
tzinfo=timezone.utc,
)
return PrivacyRequest.create(
db=db,
data={
"external_id": f"ext-{str(uuid4())}",
"started_processing_at": datetime(
2019,
1,
1,
hour=1,
minute=45,
second=55,
microsecond=393185,
tzinfo=timezone.utc,
),
"requested_at": datetime(
2018,
12,
31,
hour=2,
minute=30,
second=23,
microsecond=916482,
tzinfo=timezone.utc,
),
"status": PrivacyRequestStatus.in_processing,
"origin": f"https://example.com/",
"policy_id": policy.id,
"client_id": policy.client_id,
},
data=data,
)


@pytest.fixture(scope="function")
def privacy_request(db: Session, policy: Policy) -> PrivacyRequest:
privacy_request = _create_privacy_request_for_policy(db, policy)
privacy_request = _create_privacy_request_for_policy(
db,
policy,
)
yield privacy_request
privacy_request.delete(db)


@pytest.fixture(scope="function")
def privacy_request_status_pending(db: Session, policy: Policy) -> PrivacyRequest:
privacy_request = _create_privacy_request_for_policy(
db,
policy,
PrivacyRequestStatus.pending,
)
yield privacy_request
privacy_request.delete(db)

Expand Down
10 changes: 10 additions & 0 deletions tests/tasks/test_celery.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
def test_create_task(celery_app, celery_worker):
@celery_app.task
def multiply(x, y):
return x * y

# Force `celery_app` to register our new task
# See: https://github.com/celery/celery/issues/3642#issuecomment-369057682
celery_worker.reload()
assert multiply.run(4, 4) == 16
assert multiply.delay(4, 4).get(timeout=10) == 16
Comment on lines +1 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self, this uses Celery's pytest fixtures https://docs.celeryq.dev/en/stable/userguide/testing.html#function-scope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a testing issue I'm having in the next PR where celery_app the Pytest fixture — which will automatically use the virtualised celery_worker instead of looking for the one via the broker (Redis, which isn't created as part of the test suite) — isn't being automatically used to process tasks invoked in tests.

That doesn't show up here since the new task is added to celery_app directly. I've circumvented it in that in the next PR by addressing the task directly from celery_app.tasks[...], then calling that directly.