Routing under `-` causes issues
!20455 (merged) and !21126 (merged) caused some issues when we deployed to GitLab.com.
- We have rolling deploys on GitLab.com. The old code only knows about the non-
-
version. That isn't forwards-compatible, which is hard to do, so we have a period of time during the deploy where links might not work, because a page might be served by the new code and then you click on a link and are served the old code. The new code is backwards-compatible. - The repository links (particularly for
raw
) will break any non-redirect-following clients, likecurl
or gitlab-org/quality/triage-ops!361 (merged). - We may need to update Workhorse's upload acceleration to match the new routes.
Designs
- Show closed items
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Sean McGivern mentioned in merge request gitlab-org/quality/triage-ops!361 (merged)
mentioned in merge request gitlab-org/quality/triage-ops!361 (merged)
- Author Contributor
Suggestions from the call:
- Revert both of these changes. We will have broken links in some comments from canary, but that's not too bad if it's only from canary users from today.
- Keep
raw
without-
for some period (maybe forever), without redirecting. - Check the upload acceleration case (cc @nolith).
- Contributor
I think we need separate issue for all these problems:
- Strange things happen with markdown and merge request links during a rolling deploy
- Some clients don't follow redirects and we should think carefully about breaking them (
/raw/
) - If these route changes interact with workhorse routing, we should make sure that we merge/deploy workhorse route changes ahead of rails route changes
Collapse replies - Author Contributor
Some clients don't follow redirects and we should think carefully about breaking them (
/raw/
)Can we think of other cases? I think
raw
might be the only one here because it's the one that doesn't respond with HTML or JSON. - Contributor
we have .diff and .patch routes that are meant for machine consumption. Don't know how much they are used.
We could considering doing a log analysis where we look at the user agent.
- Author Contributor
I created #118850 for this point - again, @dzaporozhets, I'll let you manage it from there.
- Author Contributor
For now:
- We've drained canary: gitlab-com/gl-infra/production#1514 (closed)
- @nolith is going to create revert MRs and assign to @dzaporozhets.
- Once those are merged, we'll pick into auto-deploy / 12.6 and move forward again.
-
@smcgivern will check how many notes were created with the
-
links.
Collapse replies - Author Contributor
how many notes were created with the
-
links.I got a statement timeout doing this in SQL, so I did it in Ruby:
[ gprd ] production> notes = Note.where("created_at > '2019-12-18'").to_a; notes.size => 360275 [ gprd ] production> notes.select { |note| note.note.include?('/-/merge_requests') }.count => 46
Some of these are on non-GitLab projects, sadly. @jarv do you think we should silently fix them, or something else?
- Author Contributor
[ gprd ] production> notes.select { |note| note.note.include?('/-/merge_requests') }.map(&:id) => [262301113, 262328927, 262330565, 262332702, 262341193, 262345335, 262348417, 262351958, 262353867, 262354249, 262377821, 262379769, 262380780, 262385681, 262386394, 262387581, 262400365, 262400431, 262403775, 262436242, 262441577, 262443865, 262451716, 262453690, 262459065, 262464996, 262467766, 262472003, 262473127, 262478995, 262479384, 262482144, 262482251, 262488824, 262494135, 262496614, 262498048, 262501864, 262507658, 262509221, 262512044, 262535657, 262536025, 262547070, 262553262, 262553557]
- Author Contributor
We could also fix these in HAProxy, potentially, and then remove that rule once the first part of #118849 (closed) is done.
- John Jarvis mentioned in issue gitlab-com/gl-infra/production#1514 (closed)
mentioned in issue gitlab-com/gl-infra/production#1514 (closed)
- Alessio Caiazza changed the description
Compare with previous version changed the description
- Maintainer
Possible solution for rolling deploys to make it forward-compatible:
- Create a redirect for
/-/xxx
to redirect to/xxx
- Deploy this in 12.7 to make web nodes ready
- Then deploy the move to
/-/
in 12.8
- Create a redirect for
Collapse replies - Author Contributor
Thanks @engwan, I like that idea! Note that if we're only worried about rolling deploys for GitLab.com, we don't need to wait a month.
@dzaporozhets what do you think?
- Contributor
I am wondering if there can be a redirect loop when one canary redirects from
/-/xxx
to/xxx
and another from/xxx
to/-/xxx
. - Author Contributor
Could we make
/-/xxx
not redirect, but just be handled by/xxx
? Or vice versa, then add redirects after that's deployed?Edited by Sean McGivern - Contributor
I am wondering if we better have both
/xxx
and/-/xxx
routes without redirects for one release. 1 - Contributor
and then replace
/xxx
with redirect to/-/xxx
one release later - Maintainer
Yeah, you're right. It could redirect you back and forth. Just routing both to the same controller actions seems like a good idea
To summarize:
- First release: Handle both
/xxx
and/-/xxx
. URL helpers should still use/xxx
so that this doesn't break with a rolling deploy. - Next release: Switch URL helpers to use
/-/xxx
and then switch/xxx
to a redirect.
Edited by Heinrich Lee Yu 3 - First release: Handle both
- Contributor
@engwan sounds like a plan!
- Author Contributor
Cool, I moved this to #118849 (closed) - @dzaporozhets please feel free to assign / label / milestone / etc. as you see fit.
- John Jarvis mentioned in issue gitlab-development-kit#741 (closed)
mentioned in issue gitlab-development-kit#741 (closed)
- Alessio Caiazza mentioned in merge request !22034 (merged)
mentioned in merge request !22034 (merged)
- Maintainer
I made a single revert MR on !22034 (merged) to reduce the number of conflicts
- Sean McGivern mentioned in issue #118849 (closed)
mentioned in issue #118849 (closed)
- Dmytro Zaporozhets (DZ) assigned to @dzaporozhets
assigned to @dzaporozhets
- Contributor
If these route changes interact with workhorse routing, we should make sure that we merge/deploy workhorse route changes ahead of rails route changes
@jacobvosmaer-gitlab can you point me to workhorse routing that needs to be updated?
Collapse replies - Author Contributor
@dzaporozhets I think the main concern here was upload acceleration. https://gitlab.com/gitlab-org/gitlab-workhorse/blob/master/internal/upstream/routes.go handles this; in particular, you can see https://gitlab.com/gitlab-org/gitlab-workhorse/blob/ddb591f5580917f1a7eb1f37b3a27d2e95612214/internal/upstream/routes.go#L235 which assumes a project's uploads will be at
/uploads
for the project.I don't think any of the current MRs would have affected this, it's just a thing to consider in future changes.
- Maintainer
I might not be the case, but we have patterns on workhorse to match routes, and those patterns may affect the chain of handler we use
- Author Contributor
I haven't created an issue for this one as it's more of an FYI right now.
- Contributor
@dzaporozhets if you follow Sean's link above you can see the workhorse router. You know better than me what routes are going to change. :)
What probably has to happen is that for each affected route that workhorse knows about, we update the tests with a case where it uses the new route (so we have tests with both new and old), and we update the route regexes.
- Contributor
@smcgivern thanks for the link. THats exactly what I was looking for
- Sean McGivern mentioned in issue #118850
mentioned in issue #118850
- Author Contributor
@dzaporozhets @nolith just to clarify, I think we should close this issue once the reverts are deployed and we're all good. Then we can use #118850 and #118849 (closed) going forward. Does that sound good?
Collapse replies - Maintainer
it makes sense to me @smcgivern
- Contributor
Yes, agree.
- Maintainer
Just a note that we also moved wiki routes: !21185 (merged)
Not sure if we want to revert this too.
UPDATE: Looks like this wasn't included in the last auto deploy.
Edited by Heinrich Lee Yu Collapse replies - Maintainer
UPDATE: Looks like this wasn't included in the last auto deploy.
merged one December 5th, it should be already on production @engwan
1 - Contributor
- Maintainer
I do agree @dzaporozhets
- Maintainer
Yeah, I think so too. I think wiki pages aren't very commonly linked to in descriptions / comments, so it should be fine during rolling deploys.
And if it's already in production, then we don't have anything to worry about.
- Heinrich Lee Yu changed the description
Compare with previous version changed the description
- Maintainer
Hi @smcgivern,
Please add labels to your issue, this aids categorization and locating issues in the future.
Thanks for your help!
You are welcome to help improve this comment.
- 🤖 GitLab Bot 🤖 added auto updated label
added auto updated label
- Contributor
@smcgivern I close this issue as revert happened in !22034 (merged)
1 - Dmytro Zaporozhets (DZ) closed
closed
- Heinrich Lee Yu mentioned in issue #28803 (closed)
mentioned in issue #28803 (closed)
- Heinrich Lee Yu mentioned in issue #202049 (closed)
mentioned in issue #202049 (closed)
- Heinrich Lee Yu mentioned in issue gitlab-com/www-gitlab-com#6556 (moved)
mentioned in issue gitlab-com/www-gitlab-com#6556 (moved)
- Heinrich Lee Yu mentioned in issue #208669 (closed)
mentioned in issue #208669 (closed)
- Dmytro Zaporozhets (DZ) mentioned in merge request !44644 (closed)
mentioned in merge request !44644 (closed)