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

Update landing request API for use in GatewayAPI demonstrations where multiple services are involved #5

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jspaleta
Copy link

@jspaleta jspaleta commented Jun 4, 2024

This implements two features exposed as optional environment variables that helped me extend the existing starwars tutorial into more complicated demo service environments using Ingress and GatewayAPI without needing additional images.

Here's what is implemented:

  1. LANDING_RESPONSE_MSG
    This environment variable allows the default landing request response to be replaced on a service backend basis. This is very useful to communicate back to the curl client which cluster service backend you are actually talking with while making a series of changes to ingress or gatewayAPI rules as part of a guided walkthrough.

  2. LANDING_REQUEST_URL
    This environment variable allows starwars image based service to call a separate landing request service URL when a landing request is made. I used this to create thin multi-team microservice configuration involving multiple services using the same starwars image.

I used these environment variables in my gateway API/ingress demos at SCALE conference, to show some 'interesting' starwars themed examples that extend the death star service example. I'm hoping to build a documented example(s) for Cilium docs along the same lines, but first I'd like to get the official starwars service docker image updated with these optional capability so I can use the official images in the deployment manifests I write-up.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Could you sign off the commits? The DCO bot wasn't set up here yet to ensure this.

restapi/configure_deathstar.go Show resolved Hide resolved
@jspaleta jspaleta changed the title Update landing request API with customizable response text and option… Update landing request API for use in GatewayAPI demonstrations where multiple services are involved Jun 5, 2024
…al landing request microservice mode, controlled by environment variables

Signed-off-by: Jef Spaleta <[email protected]>
@jspaleta jspaleta force-pushed the customized-landing-response branch from b80bff3 to a4a93e6 Compare June 5, 2024 15:43
@jspaleta
Copy link
Author

jspaleta commented Jun 5, 2024

Could you sign off the commits? The DCO bot wasn't set up here yet to ensure this.

🥌

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

If this were targeted for use in production environments, I would probably scrutinize this a bit more just given the nature of relaying requests to a third party, however given this is primarily used as a demo tool this is probably fine. I'd prefer an approval from someone actually familiar with this code.

Copy link
Member

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

Some v minor comments but otherwise LGTM

Build and push to a registry with `make publish`. You can specify the image tag with the env var `IMAGE` (defaults to `cilium/starwars`).
Build and push to a registry with `make publish`. You can specify the image tag with the env var `IMAGE` (defaults to `cilium/starwars`)

## Set custom landing response msg
Copy link
Member

Choose a reason for hiding this comment

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

For clarity in this readme it might be nice to have an H2 something like "Runtime env vars", to distinguish from the "Build and publish" section that mentions setting a build-time env var

if err != nil {
fmt.Printf("client: error making http request: %s\n", err)
return operations.NewPostRequestLandingServiceUnavailable().WithPayload(
"Request Forward to " + landingRequestURL + " Failed\nUnknon Request Error")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Request Forward to " + landingRequestURL + " Failed\nUnknon Request Error")
"Request Forward to " + landingRequestURL + " Failed\nUnknown Request Error")

return operations.NewPostRequestLandingOK().WithPayload("Ship landed\n")
var landingRequestURL, ok = os.LookupEnv("LANDING_REQUEST_URL")
if !ok {
return operations.NewPostRequestLandingOK().WithPayload(landingResponse() + "\n")
Copy link
Member

Choose a reason for hiding this comment

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

I would have landingResponse() include the terminating \n. It already returns one of them, might as well add all of them in one place

Copy link
Member

Choose a reason for hiding this comment

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

Also - and this is really not a big deal especially since it's demo code - for readability I'd have split this out into a named function, rather than have quite so many if / else indents. But really not a big deal

@joestringer joestringer marked this pull request as draft October 3, 2024 17: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.

3 participants