Skip to content
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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

mbfreder
Copy link
Contributor

Issue #, if available: #639

Description of changes:

  • Added support for VPC Lattice as an event source
  • Include a sample app with a cloudformation template to test using SAM CLI

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Copy link
Collaborator

@deki deki left a 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>
Copy link
Collaborator

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?

Copy link
Contributor Author

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());
Copy link
Collaborator

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?

Copy link
Contributor Author

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" ?

Copy link
Collaborator

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>
Copy link
Collaborator

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

Copy link
Contributor Author

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') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.2.5

Copy link
Contributor Author

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):
Copy link
Collaborator

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?

Copy link
Contributor Author

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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 {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@deki
Copy link
Collaborator

deki commented Apr 23, 2024

@dmahapatro you may want to take a look as well

@mbfreder mbfreder requested a review from deki April 29, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants