feat: activations with webhooks #595

mkanoor · 2024-01-10T18:14:52Z

https://issues.redhat.com/browse/AAP-20844
Allow WebHooks to be added in the eda server that can be shared with different activations.

Support for HMAC and token authentication.

https://docs.google.com/document/d/1YJ5yQPKXNO7_Nqa_QEJ_NwXS-u6ZE-rLBx6tnM10Wxo/edit

jshimkus-rh · 2024-01-10T20:03:04Z

src/aap_eda/api/filters/webhook.py

@@ -0,0 +1,29 @@
+# Copyright 2023 Red Hat, Inc.


2024, instead? Other new files, also.

jshimkus-rh · 2024-01-10T21:02:26Z

src/aap_eda/api/views/activation.py

+ extra_var_obj = models.ExtraVar.objects.get(
+ pk=response.extra_var_id
+ )


This get() of the extra var looks to be redundant based on lines 87-91 which has already retrieved it when response.extra_var_id is set.

hsong-rh · 2024-01-11T14:25:16Z

@mkanoor what's the rbac settings for webhooks? do you need to pre-seed them into DB? I missed it when adding the source model.

Alex-Izquierdo

Migrations are not sync.

mkanoor · 2024-01-11T20:12:10Z

Migrations are not sync.

Updated

Alex-Izquierdo · 2024-02-20T15:17:11Z

pyproject.toml

@@ -49,6 +49,8 @@ django-ansible-base = { git = "https://github.com/ansible/django-ansible-base.gi
 jinja2 = ">=3.1.3,<3.2"
 django-split-settings = "^1.2.0"
 pexpect = "^4.9.0"
+psycopg = "*"


should be replaced the old psycopg? Have you check if there are overlaps or conflicts?
https://github.com/ansible/eda-server/blob/main/pyproject.toml#L35

The previous version is called psycopg2? pyscopg is the psycopg3, there was a memory leak that was fixed in pyscopg related to PG Notify.

@mkanoor What I'm saying is that we should remove psycopg2 and only use psycopg.

Alex-Izquierdo · 2024-02-20T15:22:36Z

src/aap_eda/api/views/activation.py

+ if activation.webhooks:
+ extra_var = {}
+ if activation.extra_var_id:
+ extra_var_obj = models.ExtraVar.objects.get(


Not a big deal since right now the extra_vars are not deleted explicitly but this would raise an exception if the id if doesn't exist, don't? It might be less prone error to use filter or a try/except block.

Alex-Izquierdo · 2024-02-20T16:48:02Z

src/aap_eda/api/views/external_webhook.py

+ logger.error(f"Headers {request.headers}")
+ logger.error(f"Body {request.body}")


Is this a mistake? Why print it always as error? should it be debug?

Alex-Izquierdo · 2024-02-20T16:50:56Z

src/aap_eda/core/models/webhook.py

+ secret = EncryptedTextField(
+ null=False, help_text="The secret for the webhook"
+ )
+ auth_type = models.TextField(


why not an enum?

Alex-Izquierdo · 2024-02-20T17:00:34Z

src/aap_eda/api/views/external_webhook.py

+ @action(detail=True, methods=["POST"], rbac_action=None)
+ def post(self, request, *args, **kwargs):
+ try:
+ self.webhook = Webhook.objects.get(uuid=kwargs["pk"])


Why a parameter? I think it is more usual take the UUID from the uri. Also it would make easier for admins to apply custom logic for specific endpoints in load-balancers.

@Alex-Izquierdo I am not sure if the admins would be routing based on each individual webhook, the routing would happen at a higher level from the NGINX configuration which will come from the UI NGINX in this PR https://github.com/ansible/ansible-ui/pull/1738/files

Alex-Izquierdo · 2024-02-20T17:05:49Z

tools/docker/docker-compose-stage.yaml

+ command:
+ - /bin/bash
+ - -c
+ - aap-eda-manage runserver 0.0.0.0:8000


stage should use gunicorn.

Alex-Izquierdo · 2024-02-20T17:07:58Z

tools/docker/docker-compose-mac.yml

@@ -165,5 +171,26 @@ services:
 retries: 3
 start_period: 5s

+ eda-webhook-api:


Without modify the nginx conf (aside of the url_prefix options) , this new service would be just a resource consumer. Anyway, if you want to add it, docker-compose-dev should have it too.

Alex-Izquierdo · 2024-02-20T17:10:31Z

src/aap_eda/settings/default.py

+SERVER_UUID = settings.get("SERVER_UUID", "abc-def-123-34567")
+WEBHOOK_URL_PREFIX = settings.get(
+ "WEBHOOK_URL_PREFIX",
+ f"https://ui.eda.local:8443/{SERVER_UUID}/api/eda/v1/external_webhook",
+)


Should be these UUID empty by default, specially after the discussion when we agreed on implement it later

Alex-Izquierdo · 2024-02-20T17:12:04Z

tools/docker/docker-compose-mac.yml

+ - EDA_WEBHOOK_URL_PREFIX=${EDA_WEBHOOK_URL_PREFIX:-https://my_nginix_server/edgecafe-beef-feed-fade-decadeedgecafe/api/eda/v1/external_webhook}
+ - EDA_WEBHOOK_HOST=${EDA_WEBHOOK_HOST:-eda-webhook-api:8000}
+ - EDA_WEBHOOK_SERVER=http://${EDA_WEBHOOK_HOST:-eda-webhook-api:8000}


I think the docker-compose should work out the box. (Docker-compose-dev not even is updated) Without override the nginx conf this wouldn't work. Probably is better to not enable the url prefix by default (related with my previous comment)

Alex-Izquierdo · 2024-03-15T12:52:13Z

pyproject.toml

@@ -49,6 +49,8 @@ django-ansible-base = { git = "https://github.com/ansible/django-ansible-base.gi
 jinja2 = ">=3.1.3,<3.2"
 django-split-settings = "^1.2.0"
 pexpect = "^4.9.0"
+psycopg = "*"


@mkanoor What I'm saying is that we should remove psycopg2 and only use psycopg.

Alex-Izquierdo · 2024-03-15T12:53:28Z

pyproject.toml

@@ -49,6 +49,8 @@ django-ansible-base = { git = "https://github.com/ansible/django-ansible-base.gi
 jinja2 = ">=3.1.3,<3.2"
 django-split-settings = "^1.2.0"
 pexpect = "^4.9.0"
+psycopg = "*"


Can you set it as psycopg = "^3.1.17" ? that aligns with downstream constraints.

mkanoor requested a review from a team as a code owner January 10, 2024 18:14

jshimkus-rh reviewed Jan 10, 2024

View reviewed changes

mkanoor force-pushed the webhook_server branch 2 times, most recently from 4994620 to 5d5771e Compare January 11, 2024 03:02

Alex-Izquierdo requested changes Jan 11, 2024

View reviewed changes

mkanoor force-pushed the webhook_server branch from 5d5771e to e673bba Compare January 11, 2024 19:38

mkanoor force-pushed the webhook_server branch 2 times, most recently from 1b90b9d to 4c255fb Compare February 18, 2024 16:33

Alex-Izquierdo self-requested a review February 20, 2024 15:14

Alex-Izquierdo reviewed Feb 20, 2024

View reviewed changes

Alex-Izquierdo self-requested a review February 20, 2024 17:03

Alex-Izquierdo reviewed Feb 20, 2024

View reviewed changes

mkanoor force-pushed the webhook_server branch 6 times, most recently from 11b2c6b to c58466b Compare February 28, 2024 00:49

mkanoor force-pushed the webhook_server branch from c58466b to d35c4dc Compare March 5, 2024 20:49

Alex-Izquierdo reviewed Mar 15, 2024

View reviewed changes

mkanoor force-pushed the webhook_server branch 8 times, most recently from 6c66371 to 1695c02 Compare April 26, 2024 21:51

mkanoor force-pushed the webhook_server branch 2 times, most recently from e09f82c to 2d89172 Compare May 2, 2024 21:16

mkanoor force-pushed the webhook_server branch 6 times, most recently from 9e69bf1 to e199afd Compare May 6, 2024 20:56

mkanoor force-pushed the webhook_server branch 3 times, most recently from 4102c9f to 57906fe Compare May 15, 2024 23:52

feat: activations with webhooks

6841029

mkanoor force-pushed the webhook_server branch from 57906fe to 6841029 Compare May 17, 2024 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: activations with webhooks #595

feat: activations with webhooks #595

mkanoor commented Jan 10, 2024 •

edited

jshimkus-rh Jan 10, 2024

jshimkus-rh Jan 10, 2024

hsong-rh commented Jan 11, 2024

Alex-Izquierdo left a comment

mkanoor commented Jan 11, 2024

Alex-Izquierdo Feb 20, 2024

mkanoor Mar 5, 2024

Alex-Izquierdo Mar 15, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

mkanoor Feb 28, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Feb 20, 2024

Alex-Izquierdo Mar 15, 2024

Alex-Izquierdo Mar 15, 2024

		logger.error(f"Headers {request.headers}")
		logger.error(f"Body {request.body}")

feat: activations with webhooks #595

Are you sure you want to change the base?

feat: activations with webhooks #595

Conversation

mkanoor commented Jan 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsong-rh commented Jan 11, 2024

Alex-Izquierdo left a comment

Choose a reason for hiding this comment

mkanoor commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkanoor commented Jan 10, 2024 •

edited