Add redirects from Fastly VCL #19
Conversation
WalkthroughThis update introduces new Varnish VCL files and acceptance tests to implement and verify HTTP-to-HTTPS, non-www, and news MP3 redirect logic. It mounts the entire VCL directory in the container, adds speedtest utilities, updates local and CI test runners to use protocol and host variables, and expands test coverage for new redirect behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Varnish
participant Backend
Client->>Varnish: HTTP request (any host/path)
alt Host starts with "www."
Varnish->>Client: 301 Redirect to https://{host without www}/
else x-forwarded-proto is "http"
Varnish->>Client: 301 Redirect to https://{host}{url}
else Request matches news MP3 legacy URL
Varnish->>Client: 308 Redirect to new /uploads/news/{n}/changelog-news-{n}.mp3
else
Varnish->>Backend: Forward request
Backend->>Varnish: Response
Varnish->>Client: Response
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
varnish/pipedream-redirects.changelog.com.vcl (1)
16-22: Consider using a more maintainable approach for multiple redirects.While the current implementation works correctly, consider using a more scalable approach if more redirects are expected in the future:
- if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3($|\?)") { - set req.http.X-Redirect = "/uploads/news/1/changelog-news-1.mp3"; - } else if (req.url ~ "^/uploads/podcast/news-2022-07-04/the-changelog-news-2022-07-04.mp3($|\?)") { - set req.http.X-Redirect = "/uploads/news/2/changelog-news-2.mp3"; - } else if (req.url ~ "^/uploads/podcast/news-2022-07-11/the-changelog-news-2022-07-11.mp3($|\?)") { - set req.http.X-Redirect = "/uploads/news/3/changelog-news-3.mp3"; - } + # Alternative approach using a single regex with capture groups + if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3($|\?)") { + set req.http.X-Redirect = "/uploads/news/1/changelog-news-1.mp3"; + } else if (req.url ~ "^/uploads/podcast/news-2022-07-04/the-changelog-news-2022-07-04.mp3($|\?)") { + set req.http.X-Redirect = "/uploads/news/2/changelog-news-2.mp3"; + } else if (req.url ~ "^/uploads/podcast/news-2022-07-11/the-changelog-news-2022-07-11.mp3($|\?)") { + set req.http.X-Redirect = "/uploads/news/3/changelog-news-3.mp3"; + }However, the current approach is perfectly acceptable for this specific use case with a small number of redirects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dagger/main.go(1 hunks)varnish/pipedream-redirects.changelog.com.vcl(1 hunks)varnish/pipedream.changelog.com.vcl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: on-github / run
🔇 Additional comments (4)
varnish/pipedream.changelog.com.vcl (1)
13-13: LGTM! Include directive is properly placed.The include directive is correctly positioned after module imports and before backend configuration, following VCL best practices. The file path matches the container setup in
dagger/main.go.dagger/main.go (1)
227-230: LGTM! Container configuration properly updated.The chained
WithFilecall correctly adds the redirects VCL file to the container. The file paths are consistent with the include directive in the main VCL configuration.varnish/pipedream-redirects.changelog.com.vcl (2)
1-27: LGTM! Well-implemented redirect logic in vcl_recv.The URL matching and redirect setup is correctly implemented:
- Regex patterns properly handle URLs with and without query strings using
($|\?)- Custom
X-Redirectheader is a good practice for passing data between VCL subroutines- 308 Permanent Redirect is the appropriate status code for this use case
- Clear documentation with test examples
29-40: LGTM! Query string preservation correctly implemented.The redirect response handling is well-designed:
- Location header construction is syntactically correct
- Query string preservation logic using
regsub(req.url, "^[^?]*", "")correctly extracts the query portion- Conditional logic ensures query strings are only appended when present
There was a problem hiding this comment.
This is a great start @mttjohnson!
Apart from the below mentions, the following are needed before we can consider merging this:
- VTC tests (my recommendation would be to duplicate
test/vtc/feeds.vtcand adapt it for this config) - acceptance tests (my recommendation would be to duplicate
test/acceptance/feeds.hurland adapt it to this change)
Appreciate the contribution 🙏
| if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3($|\?)") { | ||
| set req.http.X-Redirect = "/uploads/news/1/changelog-news-1.mp3"; | ||
| } else if (req.url ~ "^/uploads/podcast/news-2022-07-04/the-changelog-news-2022-07-04.mp3($|\?)") { | ||
| set req.http.X-Redirect = "/uploads/news/2/changelog-news-2.mp3"; | ||
| } else if (req.url ~ "^/uploads/podcast/news-2022-07-11/the-changelog-news-2022-07-11.mp3($|\?)") { | ||
| set req.http.X-Redirect = "/uploads/news/3/changelog-news-3.mp3"; | ||
| } |
There was a problem hiding this comment.
Instead of setting up an req.http.X-Redirect, could we just modify the req.url instead? I'm thinking something along the lines of:
if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3") {
set req.url = "/uploads/news/1/changelog-news-1.mp3" + regsub(req.url, "^[^?]*(\?.*)?$", "\1");
}What I'm not sure about is if Varnish 4+ will append the args automatically - I would test that before adding the regsub.
There was a problem hiding this comment.
Varnish does not automatically apply the query string to the URL, it is part of the req.url value. Overwriting the req.url would lose any value in the originally request query string of the URL, which we end up adding back to the response in vcl_synth from req.url. I'd have to test if that could be all done in the vcl_recv sub.
Retaining the original requested URL may be a better practice overall and may make some things more difficult to identify how a request originally came in. I'm not sure how this might impact any of the logging.
There was a problem hiding this comment.
OK, in which case I think that we should try to do it all in one place, something along the lines of the snippet that I posted above.
There was a problem hiding this comment.
Instead of setting up an
req.http.X-Redirect, could we just modify thereq.urlinstead?
I generally follow the following reasoning:
- I modify
req.urlif I want to affect how the object is cached. Example: normalize query-parameters so/foo?bar&bazand/foo?baz&barare cached together. - I modify
req.urlif I want to modify how upstream layers handle the request. Example: change/footo/barwithout a redirect. - I avoid modifying
req.urlto store data. If I do, I save offreq.http.original-url. Without that, it can be very difficult to diagnose issues.
In this case, though, you don't actually need either. You can "pass" the redirect with the synth call:
sub vcl_recv {
if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3($|\?)") {
return (synth(308, "/uploads/news/1/changelog-news-1.mp3"));
} else if (req.url ~ "^/uploads/podcast/news-2022-07-04/the-changelog-news-2022-07-04.mp3($|\?)") {
return (synth(308, req.http.X-Redirect = "/uploads/news/2/changelog-news-2.mp3"));
} else if (req.url ~ "^/uploads/podcast/news-2022-07-11/the-changelog-news-2022-07-11.mp3($|\?)") {
return (synth(308, req.http.X-Redirect = "/uploads/news/3/changelog-news-3.mp3"));
}
}
sub vcl_synth {
if (resp.status == 308 && resp.reason) {
set resp.http.Location = "https://" + req.http.Host + resp.reason;
set resp.reason = "Permanent Redirect";
# If a query string exists, append it to the new path
if (req.url ~ "\?.+") {
set resp.http.Location += regsub(req.url, "^[^?]*", "");
}
return (deliver);
}
}If you don't like abusing resp.reason for the redirect URL, I recommend using req.http.X-Redirect to store the variable.
There was a problem hiding this comment.
Oh, I just realized that resp.reason is read-only. I was thinking that synth() worked like error(), but it doesn't. I've often used the above pattern with vcl_error, but never with synth(). For this, I recommend req.http.X-Redirect. Otherwise, you'll get HTTP 308 /uploads/news/3/changelog-news-3.mp3, which conforms to the spec, but isn't very friendly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/vtc/news-redirects.vtc (2)
13-13: Remove unused import.The
stdmodule is imported but not used anywhere in the VCL configuration.- import std;
24-24: Clarify purpose of backend header.The
x-backendheader is set but not used in the test assertions. Is this for debugging purposes or should it be validated in the tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/acceptance/news-redirects.hurl(1 hunks)test/vtc/news-redirects.vtc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/acceptance/news-redirects.hurl
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: on-github / run
🔇 Additional comments (4)
test/vtc/news-redirects.vtc (4)
1-2: LGTM: Clear test title and setup.The varnishtest title clearly describes the purpose of the test file.
3-7: LGTM: Appropriate mock server setup.The mock server configuration correctly returns a 404 response for all requests, which will be used to validate that non-redirect URLs are passed through to the backend.
42-69: LGTM: Comprehensive redirect test coverage.The test cases effectively cover:
- Basic redirect functionality (HTTP 308 status)
- Query string preservation
- Multiple redirect entries
- Correct Location header generation
The test structure properly validates the redirect behavior described in the PR objectives.
47-47: Ignore hardcoded domain in test expectationsThe VCL’s redirect logic dynamically builds the Location header using
req.http.host. In the varnishtest environment that host is127.0.0.1, so the test’s expectationexpect resp.http.Location == "https://127.0.0.1/uploads/news/1/changelog-news-1.mp3"correctly mirrors production behavior under test. No change needed.
Likely an incorrect or invalid review comment.
|
I was able to run the vtc and hurl tests and build test that cover the news redirects in the Varnish VCL. Locally, all the VTC tests passed, and the acceptance tests passed except a few tests relating to PURGE operations on the following test files: Executed files: 9 I wasn't sure if the local environment ( |
|
PURGE should just work in local acceptance tests. I am assuming that you ran them via Screen.Recording.2025-07-04.at.20.07.46.mp4They run in CI the same way, and that always starts from the same deterministic place, so I would be curious to know what is different about your environment - or are you running the command differently? Maybe you ran the command from within the container? Trying that now... Screen.Recording.2025-07-04.at.20.11.17.mp4Found it! Pushing a commit on this branch that will fix it for you. |
|
That last commit fixed it. Want to give it a try @mttjohnson? |
That fixed the test running from inside the container for me. I also tried running them from the host (though once it's done flashing a bunch of text on the screen, I don't see the same output that everything was successful, so I have to assume that since it didn't throw an error that it was successful) |
test/acceptance/news-redirects.hurl
Outdated
| HTTP 308 # expect permanent redirect response | ||
| [Asserts] | ||
| header "Content-Length" == "0" | ||
| header "Location" contains "/uploads/news/1/changelog-news-1.mp3" |
There was a problem hiding this comment.
I recommend adding an assertion that you get Permanent Redirect as the HTTP status message. Not having it won't break anything, but it's nice for debugging.
There was a problem hiding this comment.
Yes, that's the role of the HTTP 308 right after the GET request: https://hurl.dev/docs/asserting-response.html
There was a problem hiding this comment.
That's the role of the HTTP 308 right after the GET request
That's testing the status code (number), not the status message (string). synth(308, "Waffles Are Tasty") would return a 308 Waffles Are Tasty.
There was a problem hiding this comment.
@jamesarosen I added the recommended http status text / reason phrase to news-redirect responses in the vcl return (synth(308, "Permanent Redirect"));
Once I had the text in place on the status line I tried to see if it was possible to have hurl perform an assertion on the status text, but it seems that hurl is only evaluating the status code portion, and I'm not finding a way to check the reason phrase portion of the status line.
I also cleaned up the hurl assertions where it was duplicating the assertion on the status code, as @gerhard pointed out, it is already performing an implicit assert via the HTTP 308 line.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
varnish/pipedream-redirects.changelog.com.vcl (1)
19-101: Consider data-driven approach for better maintainability.The long if-else chain works correctly but could become unwieldy as more redirects are added. Consider using a data structure or lookup table approach for better maintainability.
Additionally, there's an inconsistency in the target URL naming:
- Line 37-38:
changelog-news-9-j2g.mp3(with hyphen)- Line 85-86:
changelog-news-32p883.mp3(missing hyphen)- Line 91-92:
changelog-news-34-XXXL.mp3(with hyphen)Please verify if the missing hyphen in
changelog-news-32p883.mp3is intentional or should bechangelog-news-32-p883.mp3for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/acceptance/news-redirects.hurl(1 hunks)varnish/pipedream-redirects.changelog.com.vcl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/acceptance/news-redirects.hurl
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: on-github / run
🔇 Additional comments (3)
varnish/pipedream-redirects.changelog.com.vcl (3)
16-17: Good security practice for preventing header injection.The proactive removal of the
X-Redirectheader from client requests effectively prevents header injection attacks.
103-106: Redirect trigger logic is correct.The condition properly checks for the presence of the
X-Redirectheader and triggers a 308 Permanent Redirect response, which is appropriate for permanent URL changes.
108-119: Query string preservation logic is well implemented.The redirect response handling correctly:
- Constructs the Location header with HTTPS scheme and original host
- Preserves query strings using regex substitution
- Returns the response properly
This follows the requirements from the PR objectives to maintain consistent redirect behavior with query string preservation.
| # Remove X-Redirect header from client requests to prevent header injection | ||
| unset req.http.X-Redirect; | ||
|
|
||
| if (req.url ~ "^/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3($|\?)") { |
There was a problem hiding this comment.
So we don't lose the conversation we had: Would it be more performant to wrap this chain of reg-ex's with a simple prefix check, so that they all get skipped if the prefix makes it impossible?
|
Note TL;DR Varnish peaks at One thing that we discussed during our July 6, 2025 recording was the impact of this change (all those regexes) on performance. The answer is negligible, specifically Before this change -
|
using WithDirectory() instead of multiple WithFile() resolves thechangelog#19 (comment) Signed-off-by: Matt Johnson <m@ttjohnson.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
varnish/vcl/news-redirects.vcl (1)
19-101: Consider refactoring for better maintainability.The long chain of if-else statements works correctly but presents maintainability challenges. While VCL has limitations for data-driven approaches, consider these improvements:
Regex Pattern Consistency: All patterns correctly use
($|\?)to match either end-of-string or query parameters - this is implemented correctly.Maintainability Concern: Adding new redirects requires manual code changes to this long chain.
For better maintainability, consider extracting the URL-to-redirect mappings into a more structured format if VCL supports it, or document the pattern for future additions:
# Pattern for new redirects: # if (req.url ~ "^/uploads/podcast/news-YYYY-MM-DD/the-changelog-news-YYYY-MM-DD.mp3($|\?)") { # set req.http.X-Redirect = "/uploads/news/N/changelog-news-N.mp3"; # }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dagger/main.go(1 hunks)test/vtc/news-redirects.vtc(1 hunks)varnish/vcl/default.vcl(1 hunks)varnish/vcl/news-redirects.vcl(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- varnish/vcl/default.vcl
- dagger/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/vtc/news-redirects.vtc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: on-github / run
🔇 Additional comments (3)
varnish/vcl/news-redirects.vcl (3)
16-17: Good security practice for header injection prevention.Unsetting the
X-Redirectheader from client requests prevents potential header injection attacks. This is a solid security measure.
103-106: Redirect logic implementation is correct.The logic properly checks for the presence of the
X-Redirectheader and returns a 308 Permanent Redirect status, which is the appropriate HTTP status code for preserving the original request method.
108-119: Verify HTTPS hardcoding aligns with deployment requirements.We searched your config files for any
http://references or flags disabling SSL/TLS and found no matches, but absence of evidence isn’t evidence of absence. Please:
- Manually confirm that all deployment environments support HTTPS-only redirects.
- If any environment requires HTTP, consider making the protocol configurable or deriving it from a header (e.g.,
X-Forwarded-Proto).
|
@gerhard, I believe all the concerns raised in this pull request have been addressed. Is there anything else that I should look at? Are you wanting to re-review the pull request and merge it, or would you want me to merge it? |
|
I need to re-review & merge. It will have to be tomorrow most likely 🌊 |
test/vtc/news-redirects.vtc
Outdated
| import std; | ||
|
|
||
| # include the separate redirects vcl file | ||
| include "/etc/varnish/news-redirects.vcl"; |
There was a problem hiding this comment.
This is a neat trick! It saves on copy-pasting configs between tests and the config that gets deployed. The other tests could do with a similar refactor. Adding this to the post 1.0 list.
| WithDirectory( | ||
| "/etc/varnish", | ||
| m.Source.Directory("varnish/vcl")) |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
docs/local_dev.md (1)
238-260: Update CLI example to match new{{proto}}+{{host}}pattern.The examples below use
{{proto}}://{{host}}, but the earlier CLI invocation
(--variable host="https://127.0.0.1") still embeds the scheme insidehost.
This will fail at runtime because{{proto}}is undefined and{{host}}
contains a scheme.Recommend updating the command around line 218:
-hurl --test --color --report-html tmp --insecure --variable host="https://127.0.0.1" test/*.hurl +hurl --test --color --report-html tmp --insecure \ + --variable proto=https \ + --variable host=127.0.0.1 \ + test/*.hurl
🧹 Nitpick comments (7)
docs/local_dev.md (6)
116-118: Fix typo in the NOTE block.
custome nginx config→custom nginx config.-> a custome nginx config based on the included template file +> a custom nginx config based on the included template file
131-133: Correct spelling/grammar in container-startup comment.
ipv6 brdige→IPv6 bridge, and tighten wording to one sentence.-# Start up nginx connecting it to the ipv6 brdige -# This may produce a couple errors that can be ignored if there isn't an -# existing container running that it tries to stop or remove, but it should +# Start nginx on the IPv6 bridge. You can ignore any “container not found” +# errors emitted while the old container is being stopped/removed.
169-170: Capitalise acronym for consistency.
dns resolver→DNS resolver.-# Start the varnish container and change dns resolver to use localhost +# Start the Varnish container and change the DNS resolver to localhost
179-180: Minor spelling fix.
bacckend→backend.-# Update the /etc/dnsmasq.conf file if environment variables passed into the -# container are different from the ones set by default in the container +# Update /etc/dnsmasq.conf if the environment variables differ from those set +# by default in the container image's dev-varnish.Dockerfile.
299-299: Quote variable to guard against word-splitting.
Although$TIMEis unlikely to contain spaces, quoting is a good habit.-varnishadm vcl.use varnish_$TIME +varnishadm vcl.use "varnish_${TIME}"
312-314: Tighten comment wording & fix article.
Use “an ACL” and avoid line break in the middle of the sentence.-# acl for only allowing IPv6 addresses, you will see errors when it gets an IPv4 +# If an ACL only allows IPv6 addresses you will see errors when Varnish +# receives an IPv4 response from the DNS query.test/vtc/news-mp3-redirects.vtc (1)
47-47: Consider using a dynamic host variable instead of hardcoded IP.The tests use hardcoded
127.0.0.1in the Location header expectations. Consider using${s1_addr}or a variable that matches the actual test environment to make tests more portable across different setups.- expect resp.http.Location == "https://127.0.0.1/uploads/news/1/changelog-news-1.mp3" + expect resp.http.Location == "https://${s1_addr}/uploads/news/1/changelog-news-1.mp3"- expect resp.http.Location == "https://127.0.0.1/uploads/news/1/changelog-news-1.mp3?this=is&a=query&string" + expect resp.http.Location == "https://${s1_addr}/uploads/news/1/changelog-news-1.mp3?this=is&a=query&string"Also applies to: 55-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
README.md(2 hunks)container.justfile(1 hunks)dagger/main.go(3 hunks)docs/local_dev.md(10 hunks)justfile(4 hunks)test/acceptance/assets.hurl(7 hunks)test/acceptance/fastly/admin.hurl(1 hunks)test/acceptance/fastly/feed.hurl(2 hunks)test/acceptance/fastly/feeds.hurl(1 hunks)test/acceptance/fastly/homepage.hurl(2 hunks)test/acceptance/feeds.hurl(2 hunks)test/acceptance/http.hurl(1 hunks)test/acceptance/local/http.hurl(1 hunks)test/acceptance/news-mp3-redirects.hurl(1 hunks)test/acceptance/pipedream/admin.hurl(1 hunks)test/acceptance/pipedream/assets.hurl(3 hunks)test/acceptance/pipedream/feed.hurl(4 hunks)test/acceptance/pipedream/feeds.hurl(1 hunks)test/acceptance/pipedream/health.hurl(1 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/acceptance/www.hurl(1 hunks)test/vtc/client-ip.vtc(1 hunks)test/vtc/news-mp3-redirects.vtc(1 hunks)varnish/vcl/default.vcl(1 hunks)varnish/vcl/news-mp3-redirects.vcl(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- test/vtc/client-ip.vtc
- test/acceptance/pipedream/assets.hurl
- test/acceptance/pipedream/feed.hurl
- test/acceptance/fastly/homepage.hurl
- test/acceptance/fastly/admin.hurl
- test/acceptance/fastly/feed.hurl
- test/acceptance/fastly/feeds.hurl
- test/acceptance/pipedream/homepage.hurl
- test/acceptance/www.hurl
- test/acceptance/local/http.hurl
- test/acceptance/feeds.hurl
- test/acceptance/news-mp3-redirects.hurl
🚧 Files skipped from review as they are similar to previous changes (5)
- varnish/vcl/default.vcl
- README.md
- container.justfile
- dagger/main.go
- justfile
🔇 Additional comments (12)
test/acceptance/assets.hurl (1)
1-1: LGTM: Consistent URL formatting refactoring.The systematic change from
{{host}}/pathto{{proto}}://{{host}}/pathstandardizes URL construction across all test requests. This improves test portability across different environments and protocols.Also applies to: 11-11, 21-21, 25-25, 35-35, 47-47, 59-59, 72-72
test/acceptance/http.hurl (1)
1-9: LGTM: Well-designed HTTP to HTTPS redirect test.The test correctly validates redirect behavior with appropriate status code expectations and Location header assertions. The comments clearly explain the testing approach for different environments (local vs. Fastly vs. Fly.io).
test/acceptance/pipedream/feeds.hurl (1)
1-1: LGTM: Consistent with URL formatting standards.The URL change follows the same pattern applied across all acceptance test files for consistent protocol handling.
test/acceptance/pipedream/admin.hurl (1)
2-2: LGTM: Maintains test integrity while standardizing URL format.The URL formatting change is consistent with the broader refactoring while preserving the admin redirect test logic.
test/acceptance/pipedream/health.hurl (1)
1-1: LGTM: Comprehensive URL standardization across health endpoints.All health check endpoints consistently updated to use the standardized
{{proto}}://{{host}}/pathformat while maintaining existing test logic and assertions.Also applies to: 8-8, 16-16, 24-24
docs/local_dev.md (1)
3-4: No substantive change – safe to ignore.
The line just trims trailing whitespace after the admonition marker.test/vtc/news-mp3-redirects.vtc (2)
1-32: Well-structured test setup.The VTC configuration properly includes the redirect VCL file and sets up appropriate mock backend behavior for testing redirects.
34-56: Comprehensive test coverage for redirect scenarios.The test cases effectively verify non-redirected paths, basic redirects with 308 status, and query string preservation - covering the core redirect functionality requirements.
varnish/vcl/news-mp3-redirects.vcl (4)
16-17: Excellent security practice.Removing the
X-Redirectheader from client requests prevents header injection attacks and ensures the redirect logic controls this header exclusively.
103-105: Correct redirect implementation.The logic properly checks for the presence of
X-Redirectheader and returns the appropriate 308 Permanent Redirect status.
108-119: Excellent query string preservation logic.The implementation correctly constructs the Location header with the host and new path, and properly preserves query strings using regex substitution. The 308 status handling is also correct.
19-101: Regex patterns are correct — no changes neededI’ve verified that all
($|\?)patterns invarnish/vcl/news-mp3-redirects.vclare consistent, compile without errors, and successfully match both bare.mp3URLs and those with query strings (e.g.?param=value). URL fragments (the#…part) are never sent to the server and thus don’t appear inreq.url, so there’s no need to adjust the regex for fragment support.
| GET {{proto}}://www.{{host}}/ | ||
| HTTP 301 # expect moved permanently response | ||
| [Asserts] | ||
| header "Location" == "https://{{host}}/" |
There was a problem hiding this comment.
This seems to be testing that www. redirects to the apex. That's fine. But it seems under-specified. Do you want https://www.{{host}}/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3 to redirect to https://www.{{host}}/uploads/news/1/changelog-news-1.mp3 or to https://{{host}}/uploads/news/1/changelog-news-1.mp3?
The answer to that determines where and when you do this redirect.
There was a problem hiding this comment.
Yes, that's exactly right - just www. to apex.
So https://www.{{host}}/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3 should redirect to https://{{assets_host}}/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3
There was a problem hiding this comment.
As long as you're OK with multiple redirects: (1) www. to apex, then (2) old news URL to new.
There was a problem hiding this comment.
I think that's fine considering that these will be hit rarely. To be honest, I want to double-check with @jerodsanto why we need these news mp3 redirects. The original URLs work, so this must be done to keep things consistent. We are squarely in the make it work phase now, I'm thinking make it better & faster in follow-ups so that we ship 1.0-rc.1 and start routing some production traffic through it 🧑🔧
6afb7c3 to
3c4bac9
Compare
| GET {{proto}}://{{host}}/uploads/podcast/news-2022-06-27/the-changelog-news-2022-06-27.mp3 | ||
| Host: {{assets_host}} | ||
| HTTP 308 # expect permanent redirect response | ||
| [Asserts] | ||
| header "Location" == "https://{{assets_host}}/uploads/news/1/changelog-news-1.mp3" |
There was a problem hiding this comment.
I recommend adding some of the following:
http://{{old-news-url}}redirects tohttps://{{new-news-url}}-- change protocolhttps://www.{{old-news-url}}redirects tohttps://{{new-news-url}}-- remove subdomainhttp://www.{{old-news-url}}redirects tohttps://{{new-news-url}}-- change protocol and remove subdomain
There was a problem hiding this comment.
OK, will add more acceptance tests for http -> https redirects, not just for the root domain.
www only applies to the app domain (changelog.com) not to the assets domain (cdn.changelog.com) which is where all the news mp3 redirects are going.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
README.md(2 hunks)container.justfile(1 hunks)dagger/main.go(3 hunks)docs/local_dev.md(10 hunks)justfile(4 hunks)test/acceptance/assets.hurl(7 hunks)test/acceptance/fastly/admin.hurl(1 hunks)test/acceptance/fastly/feed.hurl(2 hunks)test/acceptance/fastly/feeds.hurl(1 hunks)test/acceptance/fastly/homepage.hurl(2 hunks)test/acceptance/feeds.hurl(2 hunks)test/acceptance/http.hurl(1 hunks)test/acceptance/local/http.hurl(1 hunks)test/acceptance/news-mp3-redirects.hurl(1 hunks)test/acceptance/pipedream/admin.hurl(1 hunks)test/acceptance/pipedream/assets.hurl(3 hunks)test/acceptance/pipedream/feed.hurl(4 hunks)test/acceptance/pipedream/feeds.hurl(1 hunks)test/acceptance/pipedream/health.hurl(1 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/acceptance/www.hurl(1 hunks)test/vtc/client-ip.vtc(1 hunks)test/vtc/news-mp3.vtc(1 hunks)test/vtc/www.vtc(1 hunks)varnish/vcl/default.vcl(1 hunks)varnish/vcl/news-mp3.vcl(1 hunks)varnish/vcl/www.vcl(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/local_dev.md
- test/acceptance/local/http.hurl
🚧 Files skipped from review as they are similar to previous changes (21)
- test/acceptance/fastly/admin.hurl
- test/acceptance/assets.hurl
- test/vtc/client-ip.vtc
- test/acceptance/www.hurl
- test/acceptance/pipedream/feed.hurl
- varnish/vcl/default.vcl
- test/acceptance/fastly/homepage.hurl
- test/acceptance/pipedream/assets.hurl
- test/acceptance/fastly/feed.hurl
- test/acceptance/pipedream/admin.hurl
- test/acceptance/pipedream/health.hurl
- test/acceptance/fastly/feeds.hurl
- README.md
- test/acceptance/feeds.hurl
- container.justfile
- test/acceptance/pipedream/feeds.hurl
- test/acceptance/http.hurl
- test/acceptance/pipedream/homepage.hurl
- dagger/main.go
- test/acceptance/news-mp3-redirects.hurl
- justfile
🔇 Additional comments (5)
varnish/vcl/www.vcl (1)
1-14: The www redirect logic is correct, but X-Redirect handler needs documentation.The www to non-www redirect implementation is sound - the regex pattern correctly matches the www prefix, and the regsub operation properly removes it. However, the X-Redirect header handling on lines 11-13 lacks context or documentation about its intended use case.
Could you clarify the purpose of the X-Redirect header handling? Is this part of the Fastly VCL behavior being replicated?
test/vtc/www.vtc (1)
19-25: Test assertions are correct and comprehensive.The client test properly validates both the redirect status code (301) and the correct Location header construction. The test covers the core functionality effectively.
test/vtc/news-mp3.vtc (1)
42-56: Good test coverage for redirect scenarios.The tests properly cover both basic redirects and query string preservation, which are critical for maintaining URL functionality. The test expectations align correctly with the VCL implementation.
varnish/vcl/news-mp3.vcl (2)
1-3: Good security practice with header injection prevention.Removing the
X-Redirectheader from client requests prevents potential header injection attacks.
94-105: Correct implementation of redirect response handling.The
vcl_synthsubroutine properly:
- Constructs the Location header with HTTPS scheme
- Preserves the original host
- Appends query strings when present using appropriate regex patterns
The regex pattern
"\?.+"correctly matches query strings, andregsub(req.url, "^[^?]*", "")properly extracts the query string portion.
|
The existing vcl_deliver has an unconditional That's not allowed on HTTP requests.
|
|
@jamesarosen considering that's the existing Fastly VCL which has been in use for years, do you think that we should change this? |
Nah. Maybe later. It's a tiny optimization. But it has me thinking about how to do that declaratively in a future version where we're not writing VCL directly. We'll want a way to indicate "modify the redirect-to URL, but continue processing for more redirects" separately from "modify the redirect-to URL, stop processing, and return a redirect." |
3c4bac9 to
e6db18e
Compare
There was a problem hiding this comment.
This now looks complete to me.
Want to give it another pass @jamesarosen @mttjohnson @nabsul before I merge it later today?
@mttjohnson if you want to rebase on top of main, go for it, otherwise I'll get it done before merging.
FWIW, I took another benchmark after the latest rewrites. There was a slow decrease in throughput (~3%), but we're still looking good (the bottleneck remains the network):
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
varnish/vcl/http.vcl (1)
1-14: LGTM! Clean HTTP to HTTPS redirect implementation.The VCL logic correctly implements HTTP to HTTPS redirects by:
- Detecting HTTP requests via the
x-forwarded-protoheader- Triggering a 301 redirect response
- Constructing the HTTPS URL using the original host and path
The implementation is clean and follows VCL best practices.
Consider adding validation for edge cases:
- What if
req.http.hostis missing or malformed?- Should there be any sanitization of the host header to prevent redirect attacks?
For now, this implementation aligns with typical CDN behavior where the host header is trusted.
test/acceptance/news-mp3.hurl (1)
1-252: Excellent comprehensive test coverage for news MP3 redirects.The acceptance tests thoroughly cover:
- Multiple date-based podcast URLs from 2022-2023
- Query string preservation (lines 7-11)
- Special file variants like "j2g", "p883", "XXXL" (lines 61-65, 205-209, 223-227)
- Proper HTTP 308 permanent redirect responses
- Canonical URL structure mapping to
/uploads/news/{number}/The test structure is consistent and uses appropriate templated variables for environment flexibility.
For maintainability, consider if this extensive list could be generated programmatically or parameterized, though comprehensive explicit testing has its merits for critical redirect functionality.
test/vtc/http.vtc (1)
20-24: Fix client naming conflict.Both test scenarios use
client c3, which should be sequential for clarity.Apply this diff to fix the client naming:
-# no x-forwarded-proto -client c3 { +# no x-forwarded-proto +client c1 { txreq -url "/" rxresp expect resp.status == 200 } -runOtherwise, the test logic correctly validates all three scenarios: no header (200), HTTPS header (200), and HTTP header (301 redirect).
Also applies to: 34-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
README.md(2 hunks)container.justfile(2 hunks)dagger/main.go(5 hunks)docs/local_dev.md(10 hunks)justfile(4 hunks)test/acceptance/assets.hurl(7 hunks)test/acceptance/fastly/admin.hurl(1 hunks)test/acceptance/fastly/feed.hurl(2 hunks)test/acceptance/fastly/feeds.hurl(1 hunks)test/acceptance/fastly/homepage.hurl(2 hunks)test/acceptance/feeds.hurl(2 hunks)test/acceptance/http.hurl(1 hunks)test/acceptance/local/http.hurl(1 hunks)test/acceptance/news-mp3.hurl(1 hunks)test/acceptance/pipedream/admin.hurl(1 hunks)test/acceptance/pipedream/assets.hurl(3 hunks)test/acceptance/pipedream/feed.hurl(4 hunks)test/acceptance/pipedream/feeds.hurl(1 hunks)test/acceptance/pipedream/health.hurl(1 hunks)test/acceptance/pipedream/homepage.hurl(3 hunks)test/acceptance/www.hurl(1 hunks)test/vtc/client-ip.vtc(1 hunks)test/vtc/http.vtc(1 hunks)test/vtc/news-mp3.vtc(1 hunks)test/vtc/www.vtc(1 hunks)varnish/vcl/default.vcl(1 hunks)varnish/vcl/http.vcl(1 hunks)varnish/vcl/news-mp3.vcl(1 hunks)varnish/vcl/www.vcl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/acceptance/assets.hurl
🚧 Files skipped from review as they are similar to previous changes (23)
- test/vtc/client-ip.vtc
- test/acceptance/fastly/homepage.hurl
- test/acceptance/pipedream/health.hurl
- test/acceptance/fastly/admin.hurl
- test/acceptance/pipedream/assets.hurl
- test/acceptance/pipedream/feed.hurl
- test/acceptance/fastly/feeds.hurl
- README.md
- test/acceptance/pipedream/feeds.hurl
- test/acceptance/fastly/feed.hurl
- test/acceptance/pipedream/admin.hurl
- test/acceptance/feeds.hurl
- test/acceptance/www.hurl
- test/acceptance/http.hurl
- test/acceptance/pipedream/homepage.hurl
- docs/local_dev.md
- test/vtc/www.vtc
- test/acceptance/local/http.hurl
- varnish/vcl/www.vcl
- justfile
- dagger/main.go
- test/vtc/news-mp3.vtc
- varnish/vcl/news-mp3.vcl
🔇 Additional comments (4)
varnish/vcl/default.vcl (1)
13-15: LGTM! Well-positioned include directives.The include directives are correctly placed after module imports and before backend declarations. This positioning ensures that the included VCL files have access to imported modules while allowing their subroutines to be available to the main VCL logic.
test/vtc/http.vtc (1)
1-17: Well-structured Varnish test setup.The test configuration correctly:
- Sets up a backend server with appropriate repetition for multiple clients
- Includes the http.vcl file being tested
- Disables caching to ensure fresh responses during testing
- Uses appropriate VCL version 4.1
The test design effectively validates the HTTP redirect functionality.
container.justfile (2)
43-52: Improved test configuration with better variable separation.The updates enhance the acceptance test setup by:
- Separating protocol and host variables for cleaner URL construction
- Using consistent Justfile environment variable syntax for
purge_token- Adding local test pattern for expanded coverage
- Aligning with the standardized URL format used across other acceptance tests
This makes the test configuration more maintainable and consistent.
98-134: Comprehensive speedtest infrastructure added.The new speedtest targets provide valuable network performance testing capabilities across multiple geographic locations. The organization is clean with descriptive target names and appropriate server IDs for each region.
This addition supports the performance benchmarking mentioned in the PR objectives.
I tried to recreate the same behavior I was seeing from the changelog.com.vcl and from sending requests to the Fastly CDN (currently www.changelog.com). In the VCL used at Fastly it was performing a 308 redirect and included the query string in the redirected url. I added a WithFile() section in main.go to make sure the new included VCL file gets copied into the container along side the default vcl when the container is built. Signed-off-by: Matt Johnson <m@ttjohnson.com>
Signed-off-by: Matt Johnson <m@ttjohnson.com>
using contains keeps it simple and avoids trying to use matches which can have unexpected behavior when trying to match query string special characters thinking they relate to regular expression syntax Signed-off-by: Matt Johnson <m@ttjohnson.com>
Signed-off-by: Gerhard Lazu <gerhard@changelog.com>
- include assertions for http status code being 308 in hurl acceptance tests - add the rest of the redirects from fastly (changelog.com.vcl) table news_redirects - defend against header injection attacks Signed-off-by: Matt Johnson <m@ttjohnson.com>
Signed-off-by: Gerhard Lazu <gerhard@changelog.com>
using WithDirectory() instead of multiple WithFile() resolves thechangelog#19 (comment) Signed-off-by: Matt Johnson <m@ttjohnson.com>
- added recommended http status text / reason phrase to news-redirect responses - removed redundant assertion on status code Signed-off-by: Matt Johnson <m@ttjohnson.com>
So that we compare the behaviour of both CDNs. The idea is that they should be identical by the time we are ready to roll out 1.0 Also add acceptance tests + redirects for: - http -> https - www -> non-ww Also added speedtest benchmarks. Especially useful when running with `--cloud` 🤫 Signed-off-by: Gerhard Lazu <gerhard@changelog.com> Signed-off-by: Gerhard Lazu <gerhard@hannibal.localdomain>
e6db18e to
2e6fb68
Compare
Follow-up to: - #19 Signed-off-by: Gerhard Lazu <gerhard@changelog.com>
I recreated the same behavior that I was seeing in
changelog.com.vcl, and by manually testing the Fastly CDN (currently www.changelog.com).In the VCL used at Fastly it was performing a 308 redirect and included the query string in the redirected url.
Covered redirects:
Added acceptance tests for all URL redirects. They run across both CDNs.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores