Skip to content

examples: Added a sandbox for testing HTTP caching based on the Front Proxy sandbox#12549

Merged
mattklein123 merged 23 commits intoenvoyproxy:masterfrom
yosrym93:sandbox
Sep 17, 2020
Merged

examples: Added a sandbox for testing HTTP caching based on the Front Proxy sandbox#12549
mattklein123 merged 23 commits intoenvoyproxy:masterfrom
yosrym93:sandbox

Conversation

@yosrym93
Copy link
Contributor

@yosrym93 yosrym93 commented Aug 7, 2020

Commit Message:
Added a sandbox for testing HTTP caching based on the Front Proxy sandbox.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Risk Level: N/A
Testing: CI example test.
Docs Changes: Update the examples docs.
Release Notes: N/A

…y sandbox

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

yosrym93 commented Aug 7, 2020

This is a draft, it is still missing documentation updates.
This sandbox runs envoy with the CacheFilter configured to SimpleHttpCache, with two backend services that requests can be routed to.

To run the sandbox:

  • cd examples/cache
  • docker-compose build --pull
  • docker-compose up -d
  • docker-compose ps, make sure you can see three containers running.

To send a request:

  • curl -v localhost:8000/service/<service_no>/<response>
    • <service_no>: Which one of the two service is this request sent to (1 or 2).
    • <response>: Which response are you requesting. Response can be found in examples/cache/responses.yaml. This file is used by both services and is mounted to the containers. Therefore, any updates made to the responses can be instantly seen by making a request (no need to rebuild or rerun the containers).

Example: curl -v localhost:8000/service/1/valid-for-minute.

Cached responses can be identified by having an age header. Furthermore, when validation takes place, a custom header is added to the response, which can either be validation: not-modified or validation: new-response, to show if the backend actually performed revalidation and the revalidation results.

Each response has an etag computed based on the response body in responses.yaml. After the etag is calculated, the date at which this response is created is appended to the body. This date is useful when a stale cached response is revalidated and served, as it shows that the date header is the date at which the response was validated, not the date of its creation.

@dio
Copy link
Member

dio commented Aug 7, 2020

cc. @phlax

@phlax
Copy link
Member

phlax commented Aug 8, 2020

hi @yosrym93 assuming #12491 lands first you will need to add a test for the sandbox in ci/verify_examples.sh

the format for the test needs to be:

run_example_cache () {
    local name paths
    name=cache
    paths=cache

    bring_up_example "$name" "$paths"
   
    ...curl tests or similar...

    cleanup "$name" "$paths"
}

the name of the function must be run_example_${dirname_of_example}

paths are comma separated paths to docker-compose envs relative to the examples dir

the (curl) tests should reflect all commands/instructions documented in the example.

i should probs write this up in dev docs somewhere i guess

EDIT: latest test format is in #12874

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

yosrym93 commented Aug 13, 2020

hi @phlax,
I am following the discussion at #12491 and will add a test for this example once it is approved.

@moderation
Copy link
Contributor

Changing "v2" to "v3" won't work. As per earlier comment the complete string is different

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

Changing "v2" to "v3" won't work. As per earlier comment the complete string is different

Sorry I did not notice the different string, my bad. I corrected it and made sure it runs correctly.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@moderation
Copy link
Contributor

I won't install Docker on my Linux machine so I modified the sandbox configs and scripts to run natively on different ports and the demo works well. Thanks

@stale
Copy link

stale bot commented Aug 22, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 22, 2020
@yosrym93
Copy link
Contributor Author

Waiting for #12491 to be merged.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 23, 2020
@stale
Copy link

stale bot commented Aug 31, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 31, 2020
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 31, 2020
@yosrym93 yosrym93 marked this pull request as ready for review August 31, 2020 19:56
@yosrym93
Copy link
Contributor Author

This is ready to be reviewed. I'll add an example test once #12491 is merged.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

@phlax @moderation I cleaned the docs and added an example test according to #12874. This should be ready for a final review.

@yosrym93
Copy link
Contributor Author

@phlax I don't understand why the tests are failing. I looked at the log for the cache example test and it seems like everything ran smoothly with no errors (like it did locally on my machine). Why is it failing?

@phlax
Copy link
Member

phlax commented Sep 11, 2020

@yosrym93 verify_build_configs - see here:

https://dev.azure.com/cncf/envoy/_build/results?buildId=50952&view=logs&j=5799eb7e-eba2-5aab-b97e-d4dd72e7ced8&t=1d3119f4-58fb-569d-0a6a-9fe5a7e7686d&l=170

@phlax
Copy link
Member

phlax commented Sep 11, 2020

...we probs need some docs about this if there arent any already

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

@yosrym93 verify_build_configs

It should be fixed now.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

@phlax I think I need to remove "cache/responses.yaml" from the BUILD file and added to the EXCLUDED_BUILD_CONFIGS in "verify_examples.sh". What's the right way to do this?

@phlax
Copy link
Member

phlax commented Sep 11, 2020

update:

EXCLUDED_BUILD_CONFIGS=${EXCLUDED_BUILD_CONFIGS:-"^./jaeger-native-tracing|docker-compose"}

and include ^./cache/responses.yaml

ie:

EXCLUDED_BUILD_CONFIGS=${EXCLUDED_BUILD_CONFIGS:-"^./cache/responses.yaml|^./jaeger-native-tracing|docker-compose"}

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
@yosrym93
Copy link
Contributor Author

@phlax This works now! Any further comments?

@yosrym93
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12549 (comment) was created by @yosrym93.

see: more, trace.

phlax
phlax previously approved these changes Sep 12, 2020
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

ive run the example and built the docs - looks good - ive left some nits and general comments

Signed-off-by: Yosry Ahmed <yosrym93@gmail.com>
Signed-off-by: Yosry Ahmed <yosrym93@gmail.com>
@capoferro
Copy link
Contributor

@jmarantz I see you're assigned to this review. Are you the right person to get this merged?

@moderation
Copy link
Contributor

This is a doc and sandbox only PR and has been thoroughly reviewed. Would be good to merge /cc @htuch @jmarantz

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thank you for more examples!!!

@mattklein123 mattklein123 merged commit d80f12f into envoyproxy:master Sep 17, 2020
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.

7 participants