Skip to content
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

PDO: Move online schema creation behind feature flag #494

Merged
merged 1 commit into from
Feb 11, 2024

Conversation

Krinkle
Copy link
Contributor

@Krinkle Krinkle commented Jul 6, 2023

The XHGui install at Wikimedia Foundation is deployed with database credentials that permit read-write operations (SELECT, INSERT) but as safety precaution do not permit admin actions like CREATE from individual web requests. In addition to not allowing admin actions, our install also disallows DELETE queries, given that the install is exposed to the public Internet (ref #248) and further disables POST requests (so that those features serve HTTP 40x instead of a db query error with HTTP 50x).

Since XHGui version 0.16.0, with #355, the lazy-creation for tables was moved from the Saver code (which is not used when browsing XHGui) to to Repo code, and thus resulted in the application serving HTTP 500 on all requests, unless the CREATE TABLE query is permitted on all web requests.

Fix this by making this method call feature flagged in a way that can be disabled.

Downstream task: https://phabricator.wikimedia.org/T341499

The XHGui install at Wikimedia Foundation is deployed with database
credentials that permit read-write operations (SELECT, INSERT) but
as safety precaution do not permit admin actions like CREATE from
individual web requests. In addition to not allowing admin actions,
our install also disallows DELETE queries, given that the install
is exposed to the public Internet (ref perftools#248)
and further disables POST requests (so that those features serve
HTTP 40x instead of a db query error with HTTP 50x).

Since XHGui version 0.16.0, with perftools#355,
the lazy-creation for tables was moved from the Saver code (which is
not used when browsing XHGui) to to Repo code, and thus resulted in
the application serving HTTP 500 on all requests, unless the
`CREATE TABLE` query is permitted on all web requests.

Fix this by making this method call feature flagged in a way that
can be disabled using XHGUI_PDO_INITSCHEMA=false.

Change-Id: I681d500fd393a47471a475b705c67280b39ab7ce
wmfgerrit pushed a commit to wikimedia/operations-puppet that referenced this pull request Jul 11, 2023
We currently run XHGui version 0.14. I'm attempting to upgrade
to 0.21.3 (latest) in Beta Cluster, but this fails due to a new
`CREATE TABLE IF NOT EXISTS` query that runs on every request to
the app, meant to lazily create/upgrade the schema.

Tracing back the versions, this first fails in 0.16.0, which moved
the schema lazy create feature in XHGui from its "save" class to
its general db "access" class, and thus runs on read requests as
well. This breaks in our deployment where we prefer to separate
admin from general read-write rights in MySQL grants.

I've patched this upstream at
perftools/xhgui#494.

This patch sets the env variable to turn this feature off, thus
hopefully unlocking the XHGui upgrade in Beta and prod.

Change-Id: I652571783b9ef74e023757cfb4e8ab11acec6f98
@glensc glensc changed the base branch from 0.21.x to 0.22.x January 23, 2024 20:26
@glensc glensc added this to the 0.22.0 milestone Jan 23, 2024
@glensc
Copy link
Contributor

glensc commented Jan 23, 2024

updated base branch to 0.22.x, but your pipelines have failed

PHP CS Fixer: src/ServiceProvider/PdoStorageProvider.php#L1Found violation(s) of type: blank_line_before_statement
--


[PHP CS Fixer: src/ServiceProvider/PdoStorageProvider.php#L1](https://github.com/perftools/xhgui/pull/494/files#annotation_12426236988)
Found violation(s) of type: blank_line_before_statement

@Krinkle
Copy link
Contributor Author

Krinkle commented Feb 9, 2024

@glensc That lint issue appears unrelated to this PR. If I understand correctly, between when I started and now, some dev package has implicitly upgraded due to unpinned versions, and this breaks builds of both 0.21.x and 0.22.x branches alike due to existing code not passing the new lint rules.

The warning in question presumably talks about the line break at the start of the file?

I don't mind creating a PR with lint fixes more generally. Most PHP projects I work on, install PHPCS and PHPUnit via Composer, including any config and presets present in the repo. I don't see an obvious way in this repo to run the linter/fixer locally and thus to verify whether it's completed. If you point me to how I do that, I'm happy to submit such a PR.

@glensc glensc merged commit 80a6298 into perftools:0.22.x Feb 11, 2024
@glensc
Copy link
Contributor

glensc commented Feb 11, 2024

The release automation has failed:

In AssertException.php line 32:
                                        
  [Psl\Type\Exception\AssertException]  
  Expected "200", got "int".            

the release automation is probably too complicated for this project. no need for multiple release branches

@glensc
Copy link
Contributor

glensc commented Feb 11, 2024

seems the exact same error I already reported:

@Krinkle Krinkle deleted the fix-pdo-init branch February 11, 2024 18:50
@glensc
Copy link
Contributor

glensc commented Feb 12, 2024

I don't understand your linter rant. the config is in repo and dependencies are managed by composer

@glensc
Copy link
Contributor

glensc commented Feb 12, 2024

Created ORGANIZATION_ADMIN_TOKEN token again:

With "repo" privileges as my PAT:

image

retried the action, job passed now:

@wmfgerrit wmfgerrit restored the fix-pdo-init branch March 11, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants