-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add support for mTLS EventStreams #1358
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #1358 +/- ##
==========================================
- Coverage 93.92% 93.54% -0.38%
==========================================
Files 320 323 +3
Lines 18837 18980 +143
==========================================
+ Hits 17692 17755 +63
- Misses 1145 1225 +80
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
- Can you add missing return type hints?
- It is not crazy to expect that the interaction with gateway will continue growing. I would consider to implement a GatewayClient and separate concerns in the service. A client would also help with tests
fb4e8d8
to
b10e232
Compare
verify=self.gateway_ssl_verify, | ||
timeout=DEFAULT_TIMEOUT, | ||
) | ||
if response.status_code == status.HTTP_200_OK: |
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 guess that you have already checked that gateway returns 200 but it is still a weird response for a delete operation which is usually 204. I suggest to add 204 as well for more resilience.
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.
Actually, looking at https://github.com/ansible-automation-platform/aap-gateway/pull/833 the ca-certificates view inherits from DRF viewset, so it should return 204, not 200.
src/aap_eda/services/signals.py
Outdated
from aap_eda.services.sync_certs import SyncCertificates | ||
|
||
|
||
@receiver(post_save, sender=models.EdaCredential) |
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.
Signals should be covered by tests
This depends on - Gateway PR which adds mTLS Routes - Platform collection which can be used by the installer - Installer changes to create a mTLS route
b10e232
to
1420594
Compare
|
This depends on
Adding a new event stream for mTLS support. A new credential has been added which can be used in conjuction with a new mTLS EventStream. When the mTLS EventStream is created if the user has added a Certificate we add it to the Gateway which can then be pulled by Envoy.
Customers in the financial sector have a requirement to use Certificate based auth when sending events to EventStreams.
This change uses the permissive mTLS feature in Envoy to dynamically update the certificates without having to restart the server. As long as Gateway has the latest Certifcate Envoy can pull it in via Secret Discovery Service (SDS) so the data flow is EDA --> Gateway <-- Envoy
This is not a breaking change but its an optional feature that requires the Gateway changes
Create a Certificate using OpenSSL
Add a new mTLS Credential (my_mtls) in EDA and load the above certificate
Add a new mTLS EventStream using the mTLS Credential (my_mtls)
Send an Event Payload to the above EventStream and it should be successful.