-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(test): implement unit tests #11
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,3 +4,4 @@ build/ | |
dist/ | ||
*.spec | ||
.venv/ | ||
.coverage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
[project] | ||
name = "signalilo-scrubbed" | ||
description = "Signalilo with alert scrubber included" | ||
version = "0.0.0" | ||
authors = [ | ||
{ name = "Adfinis", email = "[email protected]" } | ||
] | ||
|
||
dependencies = [ | ||
"Flask==3.0.3", | ||
"requests==2.32.3", | ||
"pyinstaller==6.8.0", | ||
"waitress==3.0.0", | ||
] | ||
|
||
[project.optional-dependencies] | ||
test = [ | ||
"ruff==0.5.0", | ||
"types-waitress==3.0.0.20240423", | ||
"types-requests==2.32.0.20240622", | ||
"pytest==8.2.2", | ||
"pytest-cov==5.0.0", | ||
"requests-mock==1.12.1", | ||
] | ||
|
||
[tool.flit.module] | ||
name = "scrubbed" | ||
|
||
[build-system] | ||
build-backend = "flit_core.buildapi" | ||
requires = ["flit_core >=3.2,<4"] |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,9 +37,10 @@ | |
COMMON_ANNOTATIONS = os.environ.get("SCRUBBED_COMMON_ANNOTATIONS", "").split() | ||
|
||
# Service configuration | ||
HOST = os.environ.get("SCRUBBED_LISTEN_HOST", "0.0.0.0") # noqa: S104 # listening on all instances is fine for now | ||
HOST = os.environ.get("SCRUBBED_LISTEN_HOST", "127.0.0.1") | ||
PORT = os.environ.get("SCRUBBED_LISTEN_PORT", 8080) | ||
URL = os.environ.get("SCRUBBED_DESTINATION_URL", "http://localhost:6725") | ||
TIMEOUT = 60 | ||
|
||
|
||
def redact_fields(fields: dict[str, str], keys_to_keep: list[str]): | ||
|
@@ -75,14 +76,12 @@ def webhook(): | |
|
||
logger.debug("sending: \n%s", alert) | ||
|
||
session = requests.Session() | ||
|
||
# Copy headers | ||
session.headers.clear() | ||
for h in request.headers: | ||
session.headers[h] = request.headers.get(h) | ||
|
||
r = session.post(URL, json=alert) | ||
r = requests.post( | ||
URL, | ||
json=alert, | ||
headers=request.headers, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i had a bad feeling when changing this just to make testing easier, but the longer i think about it, the more it seems like the two implementations achieve the exact same thing under the hood. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved to session exactly because headers=request.headers didn't work directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I know... and i'm pretty sure i've done this before as well but i can't seem to remember why it didn't work directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
is whole
request headers is just plain dict, while Flask request is werzeug.EnvironHeaders inheriting from werkzeug.Headers, closest those classes do is implement |
||
timeout=TIMEOUT, | ||
) | ||
msg = "alert received and processed" | ||
response = { | ||
"status": "success", | ||
|
@@ -108,12 +107,12 @@ def webhook(): | |
|
||
|
||
@app.route("/healthz") | ||
def health_check(): | ||
def health_check(): # pragma: nocover | ||
"""Endpoint for health probes.""" | ||
return "OK", 200 | ||
|
||
|
||
if __name__ == "__main__": | ||
if __name__ == "__main__": # pragma: nocover | ||
from waitress import serve | ||
|
||
serve(app, host=HOST, port=PORT) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
import json | ||
|
||
import pytest | ||
from werkzeug.test import Client | ||
|
||
from scrubbed.main import app | ||
|
||
|
||
@pytest.fixture() | ||
def client() -> Client: | ||
"""Test client for calling server endpoints.""" | ||
return Client(app) | ||
|
||
|
||
def test_invalid_get(client): | ||
response = client.get("/webhook") | ||
|
||
assert response.status == "405 METHOD NOT ALLOWED" | ||
assert response.content_type == "text/html; charset=utf-8" | ||
|
||
|
||
def test_invalid_content_type(client): | ||
response = client.post( | ||
"/webhook", content_type="text/html", data="<h1>Hello World!</h1>" | ||
) | ||
|
||
assert response.status == "400 BAD REQUEST" | ||
assert response.content_type == "application/json" | ||
assert response.json.get("message") == "request must be in JSON format" | ||
assert response.json.get("status") == "error" | ||
|
||
|
||
def test_invalid_data(client): | ||
response = client.post( | ||
"/webhook", content_type="application/json", data="<h1>Hello World!</h1>" | ||
) | ||
|
||
assert response.status == "500 INTERNAL SERVER ERROR" | ||
assert response.content_type == "application/json" | ||
assert ( | ||
response.json.get("message") | ||
== "400 Bad Request: The browser (or proxy) sent a request that this server could not understand." # noqa: E501 | ||
) | ||
assert response.json.get("status") == "error" | ||
|
||
|
||
def test_scrubbing_alert(requests_mock, client): | ||
upstream_request = requests_mock.post("http://localhost:6725") | ||
|
||
alerts = { | ||
"version": "4", | ||
"groupKey": "groupkey", | ||
"truncatedAlerts": 0, | ||
"status": "firing", | ||
"receiver": "test", | ||
"groupLabels": { | ||
"KEY": "SECRET", | ||
}, | ||
"commonLabels": { | ||
"KEY": "SECRET", | ||
}, | ||
"commonAnnotations": { | ||
"KEY": "SECRET", | ||
}, | ||
"externalURL": "https://SECRET.alertmanager.example.com", | ||
"alerts": [ | ||
{ | ||
"status": "firing", | ||
"labels": {"KEY": "SECRET"}, | ||
"annotations": {"KEY": "SECRET"}, | ||
"startsAt": "<rfc3339>", | ||
"endsAt": "<rfc3339>", | ||
"generatorURL": "https://SECRET.generator.example.com", | ||
"fingerprint": "fingerprint", | ||
} | ||
], | ||
} | ||
response = client.post( | ||
"/webhook", | ||
content_type="application/json", | ||
headers={ | ||
"KEY": "SECRET", | ||
}, | ||
data=json.dumps(alerts), | ||
) | ||
|
||
assert response.status == "200 OK" | ||
assert response.content_type == "application/json" | ||
assert upstream_request.call_count == 1 | ||
assert "SECRET" not in upstream_request.last_request.text | ||
assert upstream_request.last_request.json() == { | ||
"version": "4", | ||
"groupKey": "REDACTED", | ||
"truncatedAlerts": 0, | ||
"status": "firing", | ||
"receiver": "test", | ||
"groupLabels": { | ||
"KEY": "REDACTED", | ||
}, | ||
"commonLabels": { | ||
"KEY": "REDACTED", | ||
}, | ||
"commonAnnotations": { | ||
"KEY": "REDACTED", | ||
}, | ||
"externalURL": "REDACTED", | ||
"alerts": [ | ||
{ | ||
"annotations": { | ||
"KEY": "REDACTED", | ||
}, | ||
"endsAt": "<rfc3339>", | ||
"fingerprint": "fingerprint", | ||
"generatorURL": "REDACTED", | ||
"labels": { | ||
"KEY": "REDACTED", | ||
}, | ||
"startsAt": "<rfc3339>", | ||
"status": "firing", | ||
}, | ||
], | ||
} | ||
assert upstream_request.last_request.headers == { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This list doesn't reflect what probably should be happening... >>> import requests
>>> requests.sessions.Session().headers
{'User-Agent': 'python-requests/2.31.0', 'Accept-Encoding': 'gzip, deflate, br', 'Accept': '*/*', 'Connection': 'keep-alive'} |
||
"Host": "localhost", | ||
"Content-Type": "application/json", | ||
"Content-Length": "451", | ||
"Key": "SECRET", # TODO: redact this? | ||
Check failure on line 127 in tests/test_webhook.py GitHub Actions / ruffRuff (TD002)
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "Key" header is a bit of a stand-in. Let's check if there are any sensitive headers that we want to scrub or potentially just implement an allow list to be on the really safe side. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually lean towards using
poetry
for everything to align myself with other efforts in the enterprise.flit
is much more lightweight in comparison, and it doesn't really matter during runtime anyway.I did not know about/remember
pyinstaller
and the only reason i touched these parts is because it helped me get pytest up and running without more complex folders whatnot.For sure something we want to revisit before merging this, I'll gladly refactor back to requirement*txt if we want to stick with that route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going with pyproject lgtm too