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

Add API Gateway #1572

Merged
merged 8 commits into from
May 22, 2023
Merged

Add API Gateway #1572

merged 8 commits into from
May 22, 2023

Conversation

yoomlam
Copy link
Contributor

@yoomlam yoomlam commented May 17, 2023

What was the problem?

VRO currently serves up its API using Spring MVC. This constrains implementations of new APIs to be written using Spring MVC and Java.

How does this fix it?

To support serving up APIs implemented in other languages (i.e., Python used by the CC Team), implement a VRO Gateway that acts as a proxy to forward requests to the specified tenant API, as determined by the URI prefix.

Referring to https://piotrminkowski.com/2020/02/20/microservices-api-documentation-with-springdoc-openapi/, implement using Spring Boot 3 and Spring Cloud Gateway.

Creating a docker image and enabling Helm deployment will be done in a separate PR

How to test this PR

  • Run the API Gateway: ./gradlew :api-gateway:bootRun
  • Run the domain APIs:
# Start VRO's App API
./gradlew :app:dockerComposeUp
# This API's Swagger UI is available at http://localhost:8080/swagger

# Start Team CC's API -- see domain-cc/README.md
cd domain-cc/svc-cc-app/src
uvicorn api:app --port 18000 --reload
# This API's Swagger UI is available at http://localhost:18000/docs
  • Browse to http://localhost:8060/, read instructions, and click Swagger UI
    • Select the definition "App API" -- note that it is the same as App API. To use it, select the "/app" server on the left dropdown
    • Select the definition "Contention Classification API" -- note that it is the same as Team CC's API. This API needs to be updated to allow the "/contention-classification" to be selected as the server.

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2023

Test Results

91 tests  ±0   91 ✔️ ±0   49s ⏱️ +5s
34 suites ±0     0 💤 ±0 
34 files   ±0     0 ±0 

Results for commit 3b7e08e. ± Comparison against base commit 6ddd1d5.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2023

JaCoCo Test Coverage

File Coverage [44.25%]
OpenApiProperties.java 100% 🍏
OpenApiConfiguration.java 97.79% 🍏
OpenApiProperties.java 0%
License.java 0%
Server.java 0%
Contact.java 0%
Info.java 0%
OpenApiConfiguration.java 0%
GatewayApplication.java 0%
GatewayConfiguration.java 0%
HomePageController.java 0%
Total Project Coverage 74.67%

@yoomlam yoomlam marked this pull request as ready for review May 17, 2023 15:25
@@ -11,9 +11,6 @@ plugins {
dependencies {
implementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310'

// Added
implementation 'javax.validation:validation-api'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to import this for all java projects, plus Spring Boot 3 uses jakarta instead.

@@ -17,7 +17,6 @@ plugins {
id 'starter.java.deps-springdoc-conventions'

// Use our `local` container variants
id 'local.java.container-conventions'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

local.java.container-spring-conventions already includes local.java.container-conventions

url: /app/v3/api-docs
- name: Contention Classification API
# Use the route defined under spring.cloud.gateway below
url: /contention-classification/openapi.json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukey-luke If you the location of the openapi.json changes, please update this line.

filters:
- RewritePath=/app/(?<path>.*), /$\{path}
- id: contention-classification-tenant
# TODO: add Swagger servers such that /contention-classification is used as the prefix for requests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yoomlam yoomlam force-pushed the yoom/api-gateway branch from 5a10910 to a6e0c86 Compare May 17, 2023 17:36
@@ -0,0 +1 @@
spring_boot_version=3.0.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use Spring Boot 3. The rest of VRO is using Spring Boot 2.

@yoomlam yoomlam force-pushed the yoom/api-gateway branch 2 times, most recently from c8afd39 to 5feb2d9 Compare May 17, 2023 18:26
@yoomlam yoomlam force-pushed the yoom/api-gateway branch from 5feb2d9 to 7858c88 Compare May 17, 2023 18:57
Copy link
Contributor

@engineer-plus-plus engineer-plus-plus left a comment

Choose a reason for hiding this comment

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

I would love to see included a README file and automated tests. In a README, it would be useful to be able to find out what exactly this api gateway does. Is it just url-mapping? Authentication? Throttling? I'm pretty sure api gateways have been known to do a variety of things like this.

@Configuration
public class OpenApiConfiguration {
@Autowired
private final OpenApiProperties openApiProperties = new OpenApiProperties();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a little unusual to initialize @autowired member of a class? Seems to mildly defeat the purpose of dependency injection.

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 was copied from

@Autowired private final OpenApiProperties openApiProperties = new OpenApiProperties();

I removed @Autowired and added @RequiredArgsConstructor.

.title(info.getTitle())
.description(info.getDescription())
.version(info.getVersion())
.license(new License().name(license.getName()).url(license.getUrl()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to not just pass existing contact and license objects? It would make the code simpler.

Copy link
Contributor Author

@yoomlam yoomlam May 18, 2023

Choose a reason for hiding this comment

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

They're different object types:

  • io.swagger.v3.oas.models.info.Contact - what is expected
  • gov.va.vro.propmodel.Contact - what we create as a result of parsing the application.yaml file

Copy link
Contributor

@msnwatson msnwatson left a comment

Choose a reason for hiding this comment

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

since this ain't being used right now, i'm good with testing being included in another PR once it is potentially better exposed through docker containers and helm charts like mentioned in the PR description

@yoomlam
Copy link
Contributor Author

yoomlam commented May 18, 2023

since this ain't being used right now, i'm good with testing being included in another PR once it is potentially better exposed through docker containers and helm charts like mentioned in the PR description

Yeah, there's barely any code for unit testing. The API Gateway can be tested in an integration or end-to-end test.

@yoomlam yoomlam merged commit 23d9857 into develop May 22, 2023
@yoomlam yoomlam deleted the yoom/api-gateway branch May 22, 2023 15:01
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.

3 participants