-
Notifications
You must be signed in to change notification settings - Fork 9
feat(proxy): abstract lambda runtime api proxy #669
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?
feat(proxy): abstract lambda runtime api proxy #669
Conversation
bottlecap/src/proxy/interceptor.rs
Outdated
impl Interceptor { | ||
pub fn new( | ||
config: Arc<Config>, | ||
aws_config: &AwsConfig, |
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.
the aws config can also be removed
what is needed is only a proxy uri and a boolean for lwa
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 we need the following:
AWS_LWA_LAMBDA_RUNTIME_API
valueAWS_LAMBDA_RUNTIME_API
value
I can just send them instead of the whole config, but no matter what, this would increase the amount of fields in the state, and copying the AwsConfig
struct isn't that expensive. I could also just make the config an Arc
a reference if that's cleaner, reducing allocations.
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 might have spread out the comments in the code:
AWS_LWA_LAMBDA_RUNTIME_API
is used just once to create the socket. This can be done in start and then it's not needed anymoreAWS_LAMBDA_RUNTIME_API
can be also extracted once in the start method once with "http:/""
https://github.com/DataDog/datadog-lambda-extension/blob/jordan.gonzalez/proxy/abstract-lambda-runtime-api-proxy/bottlecap/src/proxy/interceptor.rs#L304
it is a matter of abstraction and decoupling.
The code of the proxy/intercept should accept a socket and a uri. It should not care about which socket is lwa and which uri is in the config. And especially it could be not tightly coupled with the datadog configs.
The start method could simply get the 2 strings that are needed once, and never changed, and disregard the rest of the configs.
So any future change and refactor in the config won't affect most of this code
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 understand where you're coming from to remove the coupling.
Yet I think for configuration it should be OK, this proxy's sole purpose is to work with the AWS Lambda Runtime API and/or LWA.
I think I've removed so far the config
field. I don't think the start should accept a socket/uri, this is a datadog proxy, it should remain coupled with the datadog configuration, we are eventually gonna need to use the configuration for appsec, so I don't think its bad that we have it as is.
Any change/refactor in the config should actually affect the code, we want to ensure everything works here, and not just where it's being called, that's how I see it, so far, only 2 products are (will) using it: LWA - AppSec
such as `AWS_LAMBDA_EXEC_WRAPPER`, `AWS_LWA_PROXY_LAMBDA_RUNTIME_API`, and `AWS_LAMBDA_RUNTIME_API`
main proxy code
allows us to have clear control of when we enable the proxy
…m the aws config
its not used
a6d432b
to
c26ee6d
Compare
What?
Abstracts the concept of the
proxy
from the Lambda Web Adapter implementation.This will unlock the usage of ASM.
How?
Using
axum
crate, we create a new server proxy with specific routes from the Lambda Runtime API which we are interested in proxying.Motivation
ASM and SVLS-6760