Skip to content

Issue 14989 - Redirect any "v.XX" not including docs to "latest" using a netlify edge function #16378

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

sbegin0
Copy link
Contributor

@sbegin0 sbegin0 commented Apr 3, 2025

Description

Utilizing netlify edge functions, this is an initial attempt at redirecting old versions to "/latest" (excluding docs).

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@sbegin0 sbegin0 requested a review from a team as a code owner April 3, 2025 21:13
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test labels Apr 3, 2025
@istio-testing
Copy link
Contributor

Hi @sbegin0. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@craigbox
Copy link
Contributor

craigbox commented Apr 3, 2025

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Apr 3, 2025
@craigbox
Copy link
Contributor

craigbox commented Apr 3, 2025

spot testing:

loads

redirects

fails:

HTTP 508 is Loop Detected.

$ curl -D- https://deploy-preview-16378--preliminary-istio.netlify.app/v1.24/docs
HTTP/2 301 
date: Thu, 03 Apr 2025 22:47:58 GMT
location: https://deploy-preview-16378--preliminary-istio.netlify.app/docs
server: Netlify
strict-transport-security: max-age=31536000; includeSubDomains; preload
x-nf-request-id: 01JQYXDX9FAVA8M6TYKED5PQ5D
x-robots-tag: noindex
content-length: 0
curl -D- https://deploy-preview-16378--preliminary-istio.netlify.app/v1.24/docs/
HTTP/2 508 
date: Thu, 03 Apr 2025 22:47:54 GMT
server: Netlify
strict-transport-security: max-age=31536000; includeSubDomains; preload
vary: Accept-Encoding
x-nf-request-id: 01JQYXDSJWSD0T5K3BD737YKAH
x-robots-tag: noindex
content-length: 0

@craigbox
Copy link
Contributor

craigbox commented Apr 3, 2025

Have a look at all the things you load when you go to https://istio.io/v1.24/docs/ for example:

/css
/js
/image
/manifest.json
/favicons

While most of these should be the same between releases, we can't currently guarantee that.

https://istio.io/latest/en/sitemap.xml lets you infer the paths that are relevant on the site. They are basically:

/latest/
/latest/about/
/latest/blog/
/latest/docs/
/latest/get-involved/
/latest/news/
/latest/search/

so instead of saying "only allow /docs/ through" I think it would be best to say "if the URL starts with /vX.XX and then one of /, /about, /blog etc then redirect".

@craigbox
Copy link
Contributor

craigbox commented Apr 7, 2025

(We also have to account for language codes, currently /zh and /uk.)

@sbegin0
Copy link
Contributor Author

sbegin0 commented Apr 8, 2025

New fails:
any redirects with just version number and nothing after
https://deploy-preview-16378--preliminary-istio.netlify.app/v1.10

But does redirect anything with '/' after the version number
https://deploy-preview-16378--preliminary-istio.netlify.app/v1.10/

Still fails:
https://deploy-preview-16378--preliminary-istio.netlify.app/v1.24/docs/

@craigbox
Copy link
Contributor

Are you blocked on this, or are you debugging it?

@sbegin0
Copy link
Contributor Author

sbegin0 commented Apr 10, 2025

Still debugging, should have another commit later today.

@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 10, 2025
@craigbox
Copy link
Contributor

Created #16404 to address how people will get to these archive pages when we remove the general site archive.

@craigbox
Copy link
Contributor

any redirects with just version number and nothing after
https://deploy-preview-16378--preliminary-istio.netlify.app/v1.10

But does redirect anything with '/' after the version number
https://deploy-preview-16378--preliminary-istio.netlify.app/v1.10/

Both now redirect to https://deploy-preview-16378--preliminary-istio.netlify.app/latest/.

Some of my other tests:

Can you explain why the results are different? This suggests a subtle bug?

@craigbox
Copy link
Contributor

Language spot testing:

When on https://deploy-preview-16378--preliminary-istio.netlify.app/v1.24/zh/docs/overview/ if I hit "English" in the footer I end up at /latest//docs/overview/ (note two slashes, incorrect version.

(I'm not sure how those footer links work)

It feels like we need to build a test suite!

@sbegin0
Copy link
Contributor Author

sbegin0 commented Apr 16, 2025

^ fixed trailing slash issue but redirects between languages still add that second slash. Working on a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/docs ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants