google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 292 forks source link

Implement `fpm-server-requirement-status` API Endpoint for FPFE Health Check and Script Access Verification #9632

Open hussain-t opened 2 weeks ago

hussain-t commented 2 weeks ago

Feature Description

Develop a new fpm-server-requirement-status API endpoint within the First-party mode infrastructure to verify connectivity to the FPFE /healthy endpoint and check direct PHP script access to the wp-content/plugins. This endpoint should assess whether the WordPress server can support First-party mode by performing the necessary checks.

See fpm-server-requirement-status from the REST API Controller and Health Check for First-Party Frontend sections for more details.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Happy path

Unhappy paths

QA:Eng

Note that when testing on a local development environment, the measurement.php health check will work for a Local WP install, but it won't work for a WP Local Docker installation, this is because the hostname won't resolve to the correct IP address within the Docker container.

The easiest way to test the unhappy path scenarios is to manually modify the requested URLs in REST_First_Party_Mode_Controller. However it's also possible to test them by proxying the calls made by file_get_contents(). To do this, create a file in mu-plugins with the following content:

<?php

stream_context_set_default(
  array(
    'http' => array( 'proxy' => 'localhost:8080' ), // Set the addresses according to your local environment.
    'https' => array( 'proxy' => 'localhost:8080' ),
    'ssl' => array( 'verify_peer' => false ), // This may be necessary depending on your local environment.
  )
);

Follow the happy path test, but modify the requested URLs or use a proxy to intercept the calls and verify the isFPMHealthy and isScriptAccessEnabled properties are set to false under various erroneous conditions.

Changelog entry

techanvil commented 2 weeks ago

Hey @hussain-t, reviewing these AC has made me reconsider the endpoint name. I'd suggest a preferable name would be fpm-server-requirement-status so we can GET fpm-server-requirement-status which would be more aligned with the general RESTful approach with the HTTP verb and resource name making semantic sense.

I'd also suggest that it should directly return the is_fpm_health_check_failed and is_script_access_disabled properties, that way we can update the client-side state with them directly rather than needing to make another request to the settings endpoint when we call this one.

There's also the outstanding question of whether we actually rename is_fpm_health_check_failed and is_script_access_disabled as per https://github.com/google/site-kit-wp/issues/9625#issuecomment-2462850044.

hussain-t commented 2 weeks ago

Thanks, @techanvil! Overall, I agree with your suggestions, renaming the properties and the endpoint to fpm-server-requirement-status to make it clearer. I’d suggest keeping it as a POST since the endpoint performs checks and updates the server-side properties, even though it’s not directly taking data from the client. Let me know if that works!

hussain-t commented 2 weeks ago

Thanks for the suggestions, @techanvil 👍

techanvil commented 2 weeks ago

Thanks @hussain-t, a fair point about the use of POST. I think it could go either way but I'm happy enough keeping it a POST.

hussain-t commented 2 weeks ago

Thanks, @techanvil. After giving it some more thought, I think a GET request might suit this endpoint better, given that it retrieves the current server status without taking input from the client.

techanvil commented 2 weeks ago

Thanks @hussain-t. SGTM too, let's go with the GET, then.

techanvil commented 2 weeks ago

Thanks for updating the AC, @hussain-t. I've noticed there's one additional aspect that could use clarifying and potentially updating.

  • Direct PHP Script Access Check: A test should confirm whether the server can directly access PHP files within the wp-content/plugins directory.

As per my comment on the design doc, I'd like to clarify the intention here, essentially I would advise making an HTTP request to check the script accessibility rather than a local filesystem level check, if that's not already the plan.

hussain-t commented 2 weeks ago

Thanks, @techanvil. I've updated the AC accordingly.

techanvil commented 2 weeks ago

Thanks @hussain-t!

AC ✅

hussain-t commented 2 weeks ago

Hi @techanvil, I’ve updated the AC to include the full URL for the /healthy endpoint (e.g., https://G-1234.fps.goog/mpath/healthy).

hussain-t commented 1 week ago

Thanks, @techanvil. The IB LGTM! I've slightly bumped the estimate up to 15.

IB ✅

mohitwp commented 3 days ago

QA Update ✅

![Image](https://github.com/user-attachments/assets/f062679e-f66f-4b77-bf86-4205e17063ef) ![Image](https://github.com/user-attachments/assets/7a653355-8806-40bc-bdf2-c83b4841d56f)