LGTM.

From the Task description:

1.*Get revision metadata for a title

• HTTP Verb: GET
• Production Endpoint:
  • <domain>/api/rest_v1/page/title/{title}
  • Example: https://en.wikipedia.org/api/rest_v1/page/title/Earth
• MW REST Endpoint:
  • <domain>/w/rest.php/v1/page/{title}/bare
  • Example: https://en.wikipedia.org/w/rest.php/v1/page/Earth/bare

[…]

All forwarded calls should include a x-restbase-compat header with a value of '1'. This instructs the MW REST endpoints to return RESTbase-compatible data.

I note that the first of the four listed endpoints ("Get revision metadata for a title") does not yet produce equivalent or compatible output.

$ curl 'https://en.wikipedia.org/api/rest_v1/page/title/Earth'
…
$ curl 'https://en.wikipedia.org/w/rest.php/v1/page/Earth/bare' -H 'x-restbase-compat: 1'
…

Looking in Gerrit for message:x-restbase-compat I found https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1070972 which I believe intends to implement the missing compatiblity as part of T374136: Route /api/rest_v1/page/title endpoints to MediaWiki core, which is a sibling task of this one, both under T374135.

Assuming that is finished, LGTM2.

Adding to our serviceops-radar, so that we are aware of discussion and offer input early on.

I note that the first of the four listed endpoints ("Get revision metadata for a title") does not yet produce equivalent or compatible output.

See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1070972

Just want to update that we should be good to go on this by Monday. The final changes to support the compatibility layers are currently riding the train.

@akosiaris -- can we plan to take action on this on Monday from the ServiceOps side? Is there anything else y'all need beforehand?

Compatibility changes are live. Rerouting can be performed whenever ServiceOps is able.

In T374683#10176123, @HCoplin-WMF wrote:

Just want to update that we should be good to go on this by Monday. The final changes to support the compatibility layers are currently riding the train.

@akosiaris -- can we plan to take action on this on Monday from the ServiceOps side? Is there anything else y'all need beforehand?

That's a bit short of a timeline, I don't think we 'll be ready on Monday (we haven't even dug deep into what's needed from our side). But we might be able to target the week of Oct 7th, depending on what exactly we figure out we need to do. I 'll sync internally in the team and let you know.

No worries. Are you comfortable with us posting Oct 17 as an updated target delivery in our hypothesis status?

Hey @akosiaris -- were you able to review this ticket with your team? Are we good targeting next week?

Hey @akosiaris -- just following up again on the timeline here. Do you have any updates?

Hi,

I just had enough time to review this. This can't be implemented in the infrastructure that serviceops maintains but rather the CDN/Edge. Adding Traffic. I 'll also try to post a patch, but I 'll be needing traffic's OK. I 'll also post a patch but I think that implementing such a logic at the CDN/Edge layer is exposing too much information to the it. If done, it must be temporary and reverted at a very clearly agreed date and not left lingering.

@HCoplin-WMF, I had not foreseen this complication, this will take longer than originally anticipated.

Change #1080232 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/puppet@production] ats: Route rest_v1/page/(html|title) to rest-gateway

https://gerrit.wikimedia.org/r/1080232

Change #1080241 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/deployment-charts@master] rest-gateway: Route page and html RESTBase routes to mw-api-int

https://gerrit.wikimedia.org/r/1080241

Change #1080241 merged by jenkins-bot:

[operations/deployment-charts@master] rest-gateway: Route page and html RESTBase routes to mw-api-int

https://gerrit.wikimedia.org/r/1080241

After discussing with @hnowlan, I think I was wrong. We already have some infrastructure (in the form of lua scripts for ATS) that was built with Traffic to allow easier configuration of the routing and mappings of /api/rest_v1/ that I wasn't aware of. I 'll untag Traffic, I don't think they are needed for this one. I 've already pushed and merged a patch to configure the rest-gateway to do the mapping.

Change #1080301 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/deployment-charts@master] rest-gateway: Skip project for /w/rest.php

https://gerrit.wikimedia.org/r/1080301

Change #1080301 merged by jenkins-bot:

[operations/deployment-charts@master] rest-gateway: Skip project for /w/rest.php

https://gerrit.wikimedia.org/r/1080301

@HCoplin-WMF The internal mappings on rest-gateway work, all that's left is merging https://gerrit.wikimedia.org/r/c/1080232 and external traffic will move from being routed to restbase to being routed, via rest-gateway, to /w/script.php. It looks like, something major happening aside, we will be able after all to meet the Oct 17th date. Is there some communication or other action that you 'd like to take before the change is merged and deployed? Or should we just deploy it on that date?

In T374683#10150726, @daniel wrote:

I note that the first of the four listed endpoints ("Get revision metadata for a title") does not yet produce equivalent or compatible output.

See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1070972

Looking at it today, it looks much better indeed. The header is live and working.

It is okay for the rest.php version to omit the { items: [ … ] } wrapper? I don't know what the impliction of this is, maybe RESTBase clients already have to support it without the wrapper for other reasons. I'm just comparing the two and noticed the difference.

$ curl 'https://en.wikipedia.org/api/rest_v1/page/title/Earth' | jq
{
  "items": [
    {
      "title": "Earth",
      "page_id": 9228,
      "rev": 1251058746,
      "tid": "fd358b40-8bde-11ef-b827-b199ec4151ce",
      "namespace": 0,
      "user_id": 765510,
      "user_text": "T g7",
      "timestamp": "2024-10-14T04:42:57Z",
      "comment": "/* External links */",
      "tags": [
        "wikieditor"
      ],
      "restrictions": [],
      "page_language": "en",
      "redirect": false
    }
  ]
}

$ curl 'https://en.wikipedia.org/w/rest.php/v1/page/Earth/bare' -H 'x-restbase-compat: true' | jq
{
  "title": "Earth",
  "page_id": 9228,
  "rev": 1251058746,
  "tid": "DUMMY",
  "namespace": 0,
  "user_id": 765510,
  "user_text": "T g7",
  "timestamp": "2024-10-14T04:42:57Z",
  "comment": "/* External links */",
  "tags": [
    "wikieditor"
  ],
  "restrictions": [],
  "page_language": "en",
  "redirect": false
}

It is okay for the rest.php version to omit the { items: [ … ] } wrapper?

No it's not. I... simpley missed that. Good catch, thank you!

Thank you @Krinkle ! That's an embarrassing oversight.

I've posted a fix, which we could backport. But given that there's no huge urgency, I suggest we let the change ride next week's train and activate the rerouting on or after Oct. 28th.

Sorry for the last minute timeline changes, @akosiaris

Edit: to be clear, my "embarrassing oversight" comment was only directed at myself, as the author of this task. I even posted links to the endpoints in question without noticing the discrepancy, sigh.

I suggest we let the change ride next week's train and activate the rerouting on or after Oct. 28th

Fine by me. I 'll have very limited availability that week, but a Service Ops team member should be able to pick this up. Otherwise, I 'll be merging the week after that.

The fix for the items wrapper has ridden the train and is live on production wikis. MediaWiki Interfaces is good for the rerouting to proceed at Service Ops' convenience.

Hi @akosiaris -- any updates on this? Were y'all able to pull it in this week? If not, do you have a new ETA?

Change #1080232 merged by Alexandros Kosiaris:

[operations/puppet@production] ats: Route rest_v1/page/(html|title) to rest-gateway

https://gerrit.wikimedia.org/r/1080232

In T374683#10263499, @BPirkle wrote:

The fix for the items wrapper has ridden the train and is live on production wikis. MediaWiki Interfaces is good for the rerouting to proceed at Service Ops' convenience.

I am not sure this is true, I fear. Compare the 2 results from before merging and deploying the above patch and after for curl -X 'GET' 'https://en.wikipedia.org/api/rest_v1/page/title/Paris' -H 'accept: application/json' | jq .

before

{
  "items": [
    {
      "title": "Paris",
      "page_id": 22989,
      "rev": 1254224070,
      "tid": "30054ce0-9ad5-11ef-ba0c-d74d75f006bd",
      "namespace": 0,
      "user_id": 35498457,
      "user_text": "Maxeto0910",
      "timestamp": "2024-10-30T00:17:10Z",
      "comment": "",
      "tags": [
        "mobile edit",
        "mobile web edit",
        "visualeditor",
        "advanced mobile edit"
      ],
      "restrictions": [],
      "page_language": "en",
      "redirect": false,
      "page_deleted": null
    }
  ]
}

after

{
  "id": 22989,
  "key": "Paris",
  "title": "Paris",
  "latest": {
    "id": 1254224070,
    "timestamp": "2024-10-30T00:17:10Z"
  },
  "content_model": "wikitext",
  "license": {
    "url": "https://creativecommons.org/licenses/by-sa/4.0/deed.en",
    "title": "Creative Commons Attribution-Share Alike 4.0"
  },
  "html_url": "https://en.wikipedia.org/w/rest.php/v1/page/Paris/html"
}

I 'll revert to avoid any weird effects on consumers of those APIs while we get this sorted out.

Change #1087222 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/puppet@production] Revert "ats: Route rest_v1/page/(html|title) to rest-gateway"

https://gerrit.wikimedia.org/r/1087222

Change #1087222 merged by Alexandros Kosiaris:

[operations/puppet@production] Revert "ats: Route rest_v1/page/(html|title) to rest-gateway"

https://gerrit.wikimedia.org/r/1087222

Change reverted

@akosiaris The "after" output you post above would be the output without RESTbase compat mode (not the incomplete compat mode output). This generates the correct result:

curl 'https://en.wikipedia.org/w/rest.php/v1/page/Earth/bare' -H 'x-restbase-compat: true' | jq

gives:

{
  "items": [
    {
      "title": "Earth",
      "page_id": 9228,
      "rev": 1254189690,
      "tid": "DUMMY",
      "namespace": 0,
      "user_id": 137678,
      "user_text": "SidP",
      "timestamp": "2024-10-29T20:41:30Z",
      "comment": "/* Size and shape */Capitalization, wording, reference formatting, biblio data",
      "tags": [
        "mobile edit",
        "mobile web edit",
        "advanced mobile edit"
      ],
      "restrictions": [],
      "page_language": "en",
      "redirect": false
    }
  ]
}

Note that the check for the header value got stricter at some point - x-restbase-compat: 1 used to work, but it no longer does. It has to be x-restbase-compat: true.

In T374683#10290345, @daniel wrote:

@akosiaris The "after" output you post above would be the output without RESTbase compat mode (not the incomplete compat mode output). This generates the correct result:

curl 'https://en.wikipedia.org/w/rest.php/v1/page/Earth/bare' -H 'x-restbase-compat: true' | jq

gives:

{
  "items": [
    {
      "title": "Earth",
      "page_id": 9228,
      "rev": 1254189690,
      "tid": "DUMMY",
      "namespace": 0,
      "user_id": 137678,
      "user_text": "SidP",
      "timestamp": "2024-10-29T20:41:30Z",
      "comment": "/* Size and shape */Capitalization, wording, reference formatting, biblio data",
      "tags": [
        "mobile edit",
        "mobile web edit",
        "advanced mobile edit"
      ],
      "restrictions": [],
      "page_language": "en",
      "redirect": false
    }
  ]
}

Note that the check for the header value got stricter at some point - x-restbase-compat: 1 used to work, but it no longer does. It has to be x-restbase-compat: true.

My mistake. I missed that part. I assume that we are OK with clients that don't pass x-restbase-compat: true to receive the output I pasted above in the "after" section then. Which means I should re-revert this and proceed as planned. If not, please do tell me to hold off.

Change #1087230 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/puppet@production] Revert^2 "ats: Route rest_v1/page/(html|title) to rest-gateway"

https://gerrit.wikimedia.org/r/1087230

My mistake. I missed that part. I assume that we are OK with clients that don't pass x-restbase-compat: true to receive the output I pasted above in the "after" section then. Which means I should re-revert this and proceed as planned. If not, please do tell me to hold off.

Clients shouldn't have to send the header, the gateway should add it when forwarding the request...

Summarizing from a discussion on IRC with @daniel I missed the part about

All forwarded calls should include a x-restbase-compat header with a value of 'true'. This instructs the MW REST endpoints to return RESTbase-compatible data.

I 'll have to look into how to implement that in the rest-gateway, I 'll take a step back and figure that one out before proceeding with the re-reverted change.

Change #1087388 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/deployment-charts@master] api|rest-gateway: Support sending request headers to upstream

https://gerrit.wikimedia.org/r/1087388

Change #1087388 merged by jenkins-bot:

[operations/deployment-charts@master] api|rest-gateway: Support sending request headers to upstream

https://gerrit.wikimedia.org/r/1087388

Change #1087462 had a related patch set uploaded (by Alexandros Kosiaris; author: Alexandros Kosiaris):

[operations/deployment-charts@master] Fixup for rest-gateway's request_headers_to_add

https://gerrit.wikimedia.org/r/1087462

Minor hiccup aside with having to quote values in the template, this appears to work

deploy2002$ curl --connect-to en.wikipedia.org:443:staging.svc.eqiad.wmnet:4113 https://en.wikipedia.org/en.wikipedia.org/v1/page/title/Earth |jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   371    0   371    0     0    758      0 --:--:-- --:--:-- --:--:--   757
{
  "items": [
    {
      "title": "Earth",
      "page_id": 9228,
      "rev": 1254189690,
      "tid": "DUMMY",
      "namespace": 0,
      "user_id": 137678,
      "user_text": "SidP",
      "timestamp": "2024-10-29T20:41:30Z",
      "comment": "/* Size and shape */Capitalization, wording, reference formatting, biblio data",
      "tags": [
        "mobile edit",
        "mobile web edit",
        "advanced mobile edit"
      ],
      "restrictions": [],
      "page_language": "en",
      "redirect": false
    }
  ]
}

So I 'll merge fixup and CDN changes.

Change #1087462 merged by jenkins-bot:

[operations/deployment-charts@master] Fixup for rest-gateway's request_headers_to_add

https://gerrit.wikimedia.org/r/1087462

Change #1087230 merged by Alexandros Kosiaris:

[operations/puppet@production] Revert^2 "ats: Route rest_v1/page/(html|title) to rest-gateway"

https://gerrit.wikimedia.org/r/1087230

rest-gateway and CDN changes merged, I 've forced esams so I could test more easily and now the diff is pretty simple

--- before	2024-11-05 16:11:47.739942452 +0200
+++ after	2024-11-05 16:15:51.356279856 +0200
@@ -4,7 +4,7 @@
       "title": "Athens",
       "page_id": 1216,
       "rev": 1255439155,
-      "tid": "e4fafaa0-9b7f-11ef-a3f3-99b4d84e1811",
+      "tid": "DUMMY",
       "namespace": 0,
       "user_id": 48111654,
       "user_text": "GonzalezRio",

That tid change aside, everything else is equivalent. @daniel, @BPirkle, @HCoplin-WMF, please advise if having the value DUMMY as a tid is by design or not. Feel free to close the task if yes, otherwise let me know if you wish to revert.

In T374683#10292741, @akosiaris wrote:

That tid change aside, everything else is equivalent. @daniel, @BPirkle, @HCoplin-WMF, please advise if having the value DUMMY as a tid is by design or not. Feel free to close the task if yes, otherwise let me know if you wish to revert.

Yes, this is fine. The TID value designed as a transparent token. It can be used as a path parameter in future request, but that parameter has already been ignored for several months now.

Change #1087568 had a related patch set uploaded (by BPirkle; author: BPirkle):

[mediawiki/core@master] REST: Allow page endpoint "redirect" parameter to have value "false".

https://gerrit.wikimedia.org/r/1087568

@Legoktm -- this was 100% an unintended breakage that we are working to resolve as we speak. This change was supposed to be a clean reroute, with 0 impact to all API users. We will be rolling back the change imminently, while we create an updated PR to resolve the issue at its root. Sincere apologies for the accidental break, and we will get it resolved as soon as possible. Wholeheartedly agree that this should not be an emergency drill for you, as there should be no changes required on your side.

I also want to recognize and apologize for the slight delay in response and action since you flagged. It is election day in the US, so our engineers who are normally responding this time of day are out voting!

Change #1087591 had a related patch set uploaded (by Arlolra; author: Arlolra):

[operations/puppet@production] Revert "Revert^2 "ats: Route rest_v1/page/(html|title) to rest-gateway""

https://gerrit.wikimedia.org/r/1087591

Change #1087591 merged by Ladsgroup:

[operations/puppet@production] Revert "Revert^2 "ats: Route rest_v1/page/(html|title) to rest-gateway""

https://gerrit.wikimedia.org/r/1087591

In T374683#10294694, @HCoplin-WMF wrote:

@Legoktm -- this was 100% an unintended breakage that we are working to resolve as we speak.

Sounds good. I'll give the revert ~20 more minutes to roll out fully and report back to see if things are fixed.

This change was supposed to be a clean reroute, with 0 impact to all API users.

This is a reasonable goal, but I don't think it justfies the lack of communication. Shit happens, just let us know what to expect ahead of time. For example, if this was deployed to test.wikipedia.org first, our automated test suite would've run against it and caught it. Redirecting a big chunk of RESTBase over to rest.php is genuinely a big deal (as evidenced by how long it took to get here!!) and should be something people are shouting from the rooftops in celebration, not complaining about.

I also want to recognize and apologize for the slight delay in response and action since you flagged. It is election day in the US, so our engineers who are normally responding this time of day are out voting!

Makes sense, hopefully we can dig into this properly in the incident report (e.g. should U.S. election day be a no deploy day given low availability).

In T374683#10294780, @Legoktm wrote:

I'll give the revert ~20 more minutes to roll out fully and report back to see if things are fixed.

Things look good from what I can see.

Bill correctly identified the root cause: the core endpoitn expects redirect=no (for consistency with index.php), while RESTbase expected redirect=false. We can just accept both, see https://gerrit.wikimedia.org/r/1087568.

In T374683#10295353, @daniel wrote:

Bill correctly identified the root cause: the core endpoitn expects redirect=no (for consistency with index.php), while RESTbase expected redirect=false. We can just accept both, see https://gerrit.wikimedia.org/r/1087568.

Before rolling out the switch again, it might be a good idea to survey all query parameters that are accepted for compatibility, if that hasn't been done already.

Moreover: do we have a set of urls that we've tested to work? It might be useful for us so that we can prepare an httpbb test suite to run before and after switching one host for validating the change.

In T374683#10295502, @Joe wrote:

Before rolling out the switch again, it might be a good idea to survey all query parameters that are accepted for compatibility, if that hasn't been done already.

Yes, I just double-checked - the only other supported query parameter is stash, which can be true or false.

Moreover: do we have a set of urls that we've tested to work? It might be useful for us so that we can prepare an httpbb test suite to run before and after switching one host for validating the change.

That would be nice, but it wouldn't have caught the problem we just encoutnered either - I wouldn't have thought to check for redirect=false. Known good cases are easy to cover, unknown bad cases are easy to miss...

Also, we may encounter problems that are more subtle - etag handling or cache control headers or CORS or whatnot. I checked all the stuff that I could think of a while ago, see https://docs.google.com/spreadsheets/d/10FaxUcD6y4Xjss21HfXUwVsH98RCWO7Bs9hhZuDTfFg/edit?pli=1&gid=0#gid=0. But as we just found out, I didn't think of all the relevgant things to check...

In T374683#10295634, @daniel wrote:

That would be nice, but it wouldn't have caught the problem we just encoutnered either - I wouldn't have thought to check for redirect=false. Known good cases are easy to cover, unknown bad cases are easy to miss...

Also, we may encounter problems that are more subtle - etag handling or cache control headers or CORS or whatnot. I checked all the stuff that I could think of a while ago, see https://docs.google.com/spreadsheets/d/10FaxUcD6y4Xjss21HfXUwVsH98RCWO7Bs9hhZuDTfFg/edit?pli=1&gid=0#gid=0. But as we just found out, I didn't think of all the relevgant things to check...

That all makes sense, but also now seems like the perfect time to encode a bunch of this lost-then-rediscovered knowledge into httpbb rules.

This is a haunted graveyard but it doesn't have to remain so.

In T374683#10295353, @daniel wrote:

Bill correctly identified the root cause: the core endpoitn expects redirect=no (for consistency with index.php), while RESTbase expected redirect=false. We can just accept both, see https://gerrit.wikimedia.org/r/1087568.

The responses were also not returning an etag header, as previously expected.

In T374683#10295634, @daniel wrote:

In T374683#10295502, @Joe wrote:

Before rolling out the switch again, it might be a good idea to survey all query parameters that are accepted for compatibility, if that hasn't been done already.

That would be nice, but it wouldn't have caught the problem we just encoutnered either - I wouldn't have thought to check for redirect=false. Known good cases are easy to cover, unknown bad cases are easy to miss...

AFAIK redirect=false was always the canonical way to pass it (c.f. T130757#2184827). And like I said, this is all in the mwbot-rs test suite. Can you deploy to test.wikipedia.org first this time and give us like a week?

Also, we may encounter problems that are more subtle - etag handling or cache control headers or CORS or whatnot. I checked all the stuff that I could think of a while ago, see https://docs.google.com/spreadsheets/d/10FaxUcD6y4Xjss21HfXUwVsH98RCWO7Bs9hhZuDTfFg/edit?pli=1&gid=0#gid=0. But as we just found out, I didn't think of all the relevgant things to check...

This is a private spreadsheet.

It seems like this should be included in the next (or a near-future) Tech News edition.
Please could someone propose some wording/links for a Tech News entry? Please also confirm whether it should be in the upcoming edition (Frozen on Friday, sent on Monday), or the edition after that. Thanks.

Also, we may encounter problems that are more subtle - etag handling or cache control headers or CORS or whatnot. I checked all the stuff that I could think of a while ago, see https://docs.google.com/spreadsheets/d/10FaxUcD6y4Xjss21HfXUwVsH98RCWO7Bs9hhZuDTfFg/edit?pli=1&gid=0#gid=0. But as we just found out, I didn't think of all the relevgant things to check...

This is a private spreadsheet.

Should be accessible to you now.

The responses were also not returning an etag header, as previously expected.

That shouldn't have been the case... you are seeing the etags now, right?

Can you deploy to test.wikipedia.org first this time and give us like a week?

Not sure how easy it is to have gateway rules depend on the wiki domain... @akosiaris ?

In T374683#10297761, @daniel wrote:

The responses were also not returning an etag header, as previously expected.

That shouldn't have been the case... you are seeing the etags now, right?

Yes.

In T374683#10297761, @daniel wrote:

The responses were also not returning an etag header, as previously expected.

That shouldn't have been the case... you are seeing the etags now, right?

Can you deploy to test.wikipedia.org first this time and give us like a week?

Not sure how easy it is to have gateway rules depend on the wiki domain... @akosiaris ?

Yes, it is doable, although not an exception we 'd like to carry for a prolonged period of time. A week sounds fine though.

In T374683#10297633, @Quiddity wrote:

It seems like this should be included in the next (or a near-future) Tech News edition.
Please could someone propose some wording/links for a Tech News entry? Please also confirm whether it should be in the upcoming edition (Frozen on Friday, sent on Monday), or the edition after that. Thanks.

@Quiddity has a point here, lack of sufficient communication has backfired once already, it's prudent to over communicate this time around to avoid similar issues.

I think the exact wording, as well as timing, is up to MW-Interfaces-Team, so I 'll refrain from suggesting one, but @HCoplin-WMF, this is probably a good opportunity!

Confirming that we are including a callout for this work in this week's Tech News. Thanks for the nudge @akosiaris and @Quiddity !

Change #1087568 merged by jenkins-bot:

[mediawiki/core@master] REST: Allow page endpoint "redirect" parameter to have value "false".

https://gerrit.wikimedia.org/r/1087568