Page MenuHomePhabricator

Please review MediaWiki Apache config changes adding new docroot for the auth domain
Closed, ResolvedPublic

Description

Please review these MediaWiki Apache config changes adding new docroot for the auth domain: https://gerrit.wikimedia.org/r/c/operations/puppet/+/1115104. rzl suggested that I file a task to request your help (see https://gerrit.wikimedia.org/r/c/operations/puppet/+/1115104/comments/e5bba9f1_237a857c).

Event Timeline

RLazarus changed the task status from Open to In Progress.Feb 2 2025, 8:14 PM
RLazarus claimed this task.
jijiki triaged this task as Medium priority.Feb 3 2025, 1:23 PM

We rolled this back because, on deploying to Kubernetes, we didn't see a diff associated with flipping public_rewrites to false (line 541 of the modified version).

The patch is correct; this is a template bug. It comes from this in _site_helpers.tpl:

{{- range $v := .vhosts -}}
    {{- $params := merge $v $defaults $base_params $tpl_defaults -}}
    {{- include "mw-vhost" $params | indent 4 -}}
{{- end }}

The intent is to take each vhost (from the values, in this case from /etc/helmfile-defaults/mediawiki/httpd.yaml) and fill in any unset values from the defaults, falling back to successively more generic defaults when keys are missing.

The problem is, false is the zero value for booleans. merge treats a false value the same as a missing dict entry, so it's impossible to override the default public_rewrites: true in $tpl_defaults with public_rewrites: false in helmfile-defaults.

That is,

{{- merge (dict)           (dict "a" true) -}}   # returns true
{{- merge (dict "a" true)  (dict "a" false) -}}  # returns true
{{- merge (dict "a" false) (dict "a" true) -}}   # returns true

The first two are consistent with how we'd want default-overriding to work, but the third means it's a bug to use merge this way.

The solution is indeed to use mergeOverwrite instead:

{{ mergeOverwrite (dict "a" true) (dict "a" false) }} yields map[a:false]

I would like to help resolve this faster and unblock my team's work. I'll submit that patch, if it's such an easy fix (I hope it really is).

Change #1117922 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[operations/deployment-charts@master] mediawiki: Allow overwriting true config defaults with false

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

Change #1118003 had a related patch set uploaded (by RLazarus; author: RLazarus):

[operations/deployment-charts@master] mediawiki: Fix default-merging logic in _site_helpers.tpl

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

Change #1118003 merged by jenkins-bot:

[operations/deployment-charts@master] mediawiki: Fix default-merging logic in _site_helpers.tpl

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

The template bug is fixed and we should be able to deploy the original patch next week. The SRE team will be at a summit so I'm not sure exactly what the timing will be, but I'll try to find a quiet moment to ship this out.

Change #1117922 abandoned by Bartosz Dziewoński:

[operations/deployment-charts@master] mediawiki: Allow overwriting true config defaults with false

Reason:

Superseded by https://gerrit.wikimedia.org/r/c/1118003, thank you rzl!

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

I saw that it was deployed today. Thank you.

I saw that it was deployed today. Thank you.

Thanks for your patience, and for exposing that bug!