Drop using sudo to execute run-tests.sh

The MR shows permission denied failures: https://git.drupalcode.org/issue/drupal-3476189/-/jobs/2831557

-1 as this could hide real issues in file access. It is less than I originally thought (as the webserver is not running root part of functional tests will still be non-root) however that still leaves kernel and unit tests to run excessive permissions.

Sudo supports passing all environment variables using the -E flag. Tagging as needs an IS a update.

I noted in the linked issues that it appeared the problem was that run-tests.sh appeared to accidentally call sudo inside its loops (probably some how confused when looking at argv would be my guess) I would suggest core debug and fixing the underlying fault in run-tests.sh instead of switching to a privileged user for testing.

The MR made just shows the problem. It’s not the solution )or at least all of it). I agree that root should not be needed.

I think that we agree on the solution but maybe the issue description or title does not reflect that.

Few minutes with the debugger. Global $php is indeed being set to the sudo path.

I' have not run a full test run however it appears the solution to the root fault is to move elseif ($sudo = getenv('SUDO_COMMAND')) { block before elseif ($php_env = getenv('_')) { in simpletest_script_init()

(check for sudo before checking if _ is set)

It looks like we do not agree on the solution yet, actually.

I think first of all we need to agree on: what is the user (meaning, the op sys user) that should run PHPUnit tests in the context of GitLabCI? Then, with a decision, adjust accordingly.

At the moment in HEAD it's www-data because of the usage of sudo.

However, I repeat my comment from the related issue #3475974-27: Add www-data user to root group so all env vars as passed to run-tests.sh → :

In fact, I think we should just be running tests as root, like we run all other tasks in these jobs. www-data is, or at least it should be, the user for the Apache webserver to deliver pages: using it to run Unit or Kernel tests is just, IMHO, wrong, and possibly a remnant of simpletest times. Functional(Javascript) tests are different, and IMHO the tests should run under root, and the SUT under www-data because these tests check the results of pages delivered by the webserver.

Elaborating a bit: I think we should run the test without sudo, just with whatever user we are executing the rest of the commands in the context of the test job. In CI, this happens to be root.

Counter view:

Generally you want all tests that execute code to run as close to production as possible, this includes unit and kernel tests. Both of these can access the file system and running as root can mask faults.

In production it is unlikely the process running any of the Drupal code will have root privileges.

All our other tests (cspell, phpstan, lint, etc) are not executing the code they are just reading it for analysis as such the execution user is not relevant to the code.

MR!9585 goes in the alternative direction of using the -E flag with sudo to keep all the env variables.

Also possibly related, 📌 Change run-tests.sh to use Symfony Process instead of proc_open Active .

Fixed up the tests to pass in HOME to clean up the small few errors that remained.

The last two remaining errors are related to Telemetry. It appears that OTEL_COLLECTOR is being set to a literal string $OTEL_COLLECTOR on cursory glance I'm not 100% sure why.

Perhaps Gitlab does this when its an undefined value? Though I don't understand why it would work before the sudo. Unless this was kept in an environment file for user www-data we are no longer loading, although I don't see any code doing so on cursory glance either.

mondrake → changed the visibility of the branch 3476189-drop-using-sudo to hidden.

Really minor feedback in the MR. Two questions and a small character left.

I think this is a great improvement as we know can pass all variables via -E instead of needing to pass one by one.
There is some really minor feedback on the MR, some of it already addressed and some of it might just be a non-issue at all.

This change will also simplify the solution for the gitlab_templates related issue.

I'm setting it to RTBC.

Added a test skip in case OTEL_COLLECTOR is empty. But not sure we want to skip the test overall in that case. Anyway, up for review.

Reverted last commit on a better thinking. Back to RTBC as it was.

IMHO this needs backport to all branches currently tested so the parallel contrib issue can benefit as well.

IMHO this needs backport to all branches currently tested so the parallel contrib issue can benefit as well

I will note gitlab_templates probably should just use the —php flag for run-tests.sh as it allows them to support versions this can’t be backported for (existing tagged releases) for those who may do min/max testing.

They could do the above today without waiting for this issue.

Everything looks in order here, no unanswered questions.

Left one question on the MR

Maybe instead of using such a long sudo command it's better to add simple wrapper-script to source .test.env before running the command.
Moreover it will make logs shorter and less noisy to parse!

Also it should be enough to use su for this purpose so more secure environment

Now we use sudo -u www-data -E HOME=/var/www to set the correct home directory path, but sudo provides also -H, --set-home option that does the same, so maybe let's use it instead?

@murz that seems sensible; let’s try it and see if it works

I pushed the changes, please review.

Tests pass (apart from the usual bunch of random fails in functional ones), so it seems to be working. Setting back to RTBC.

From https://linux.die.net/man/8/sudo, emphasis mine:

-H' The -H (HOME) option requests that the security policy set the HOME environment variable to the home directory of the target user (root by default) as specified by the password database. Depending on the policy, this may be the default behavior.

Re OTEL_COLLECTOR this is injected into the scheduled core performance test pipelines directly in GitLab.

There is also code in PerformanceTestTrait::openTelemetryTracing() that skips sending to the collector if the variable is not set:

    $collector = getenv('OTEL_COLLECTOR');
    if (!$collector) {
      return;
    }

I did a light review of the comment and I don't see any response to the suggestions by andypost in #24.

#24 is above me, but to me it sounds like it can be a follow up anyway.

Added a comment about OTEL_COLLECTOR, setting it to the empty string is a bit confusing when we shouldn't need it at all - I think we can remove it entirely.

Applied suggestion.

I think the change is minor enough to let me RTBC back. Tests are green.

I think we only need this on 11.x, it doesn't backport cleanly to 10.x either. Please open a backport MR if you disagree!

Committed eba0136 and pushed to 11.x. Thanks!

longwave → closed merge request !9585

#39 agree backport is not necessary for core, but I am not sure about contrib. Will drop a line in Slack/gitlab.

Slack discussion on backport: https://drupal.slack.com/archives/CGKLP028K/p1735851033816899

Automatically closed - issue fixed for 2 weeks with no activity.