-
Notifications
You must be signed in to change notification settings - Fork 561
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
Support VPC Lattice as an event source #845
base: main
Are you sure you want to change the base?
Conversation
classes
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.
Successfully deployed the sample but it's hard to follow and needs more documentation along with it. We should also enable auth and leverage https://github.com/okigan/awscurl to call it.
Looking at https://docs.aws.amazon.com/lambda/latest/dg/services-vpc-lattice.html#vpc-lattice-register V1 payload is the default, it makes sense for me to only support V2 but we should avoid that customers end up misconfigured.
@@ -63,6 +70,12 @@ | |||
<version>6.2.3</version> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.projectlombok</groupId> | |||
<artifactId>lombok</artifactId> |
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 know where it's coming from, but can we please not introduce Lombok to this project?
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.
Sure. Thanks
@@ -125,7 +125,7 @@ public String format(ContainerRequestType servletRequest, ContainerResponseType | |||
logLineBuilder.append(" "); | |||
logLineBuilder.append(servletRequest.getRequestURI()); | |||
logLineBuilder.append(" "); | |||
logLineBuilder.append(servletRequest.getProtocol()); | |||
//logLineBuilder.append(servletRequest.getProtocol()); |
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.
what is the reason for this change?
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.
There's no way to get the protocol from the lattice payload (confirmed with the VPC Lattice team), so I returned an UnsupportedOperationException which throws a nullpointerException on that line. Is it the right thing to return? or should we default to "HTTP/1.1" ?
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'd default to HTTP as this is the only protocol supported anyway.
<parent> | ||
<groupId>org.springframework.boot</groupId> | ||
<artifactId>spring-boot-starter-parent</artifactId> | ||
<version>3.2.3</version> |
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.
should be 3.2.5
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.
Thanks
|
||
dependencies { | ||
implementation ( | ||
implementation('org.springframework.boot:spring-boot-starter-web:3.2.3') { |
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.
3.2.5
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.
Thanks
import json | ||
import http.client | ||
|
||
def handler(event, context): |
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.
what is this being used for?
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.
This is the code for the Outbound lambda function, which is used to call the SpringBoot application. I updated the README with a diagram to illustrate the flow.
Properties: | ||
Handler: index.handler | ||
Role: !GetAtt OutboundLambdaFunctionRole.Arn | ||
Runtime: python3.9 |
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.
why are we using Python here?
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.
This is the client lambda function. I used python because it was much easier to write.
* This object is initialized with an <code>VPCLatticeV2RequestEvent</code> event and a <code>SecurityContext</code> generated | ||
* by an implementation of the <code>SecurityContextWriter</code>. | ||
*/ | ||
public class AwsVpcLatticeV2HttpServletRequest extends AwsHttpServletRequest { |
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.
Duplicates a lot of code from AwsHttpApiV2ProxyHttpServletRequest
. Either pull methods up to AwsHttpServletRequest
or extend the class hierarchy accordingly.
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 already moved most of the duplicated code in AwsHttpServletRequest
.
@dmahapatro you may want to take a look as well |
# Conflicts: # pom.xml
# Conflicts: # pom.xml
Issue #, if available: #639
Description of changes:
By submitting this pull request