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

[🐛 BUG]: Fail to upload files #1314

Closed
1 task done
egonbraun opened this issue Oct 6, 2022 · 18 comments · Fixed by roadrunner-server/http#58
Closed
1 task done

[🐛 BUG]: Fail to upload files #1314

egonbraun opened this issue Oct 6, 2022 · 18 comments · Fixed by roadrunner-server/http#58
Assignees
Labels
B-bug Bug: bug, exception F-need-verification
Milestone

Comments

@egonbraun
Copy link

egonbraun commented Oct 6, 2022

No duplicates 🥲.

  • I have searched for a similar issue in our bug tracker and didn't find any solutions.

What happened?

When we tried to upload an image to our backend service the worker failed and the client received an HTTP 500 error message, the logs just stated that the file did not exist.

Our stack is running:

  • PHP 8.1
  • Laravel 9.28.0
  • Roadrunner 2.11.3
  • spiral/roadrunner-laravel 5.9.0

We noticed that the error happens before our code is even triggered, indicating that the issue happens while Laravel is building the request to send to out controller class. The error itself is triggered in the following file as part of the nyholm/psr7 library (I believe is a core dependency of Laravel or Synfony).

vendor/nyholm/psr7/src/Factory/Psr17Factory.php

It's worth noting our applicating runs inside a Docker container.

Our roadrunner configuration:

version: "2.7"

server:
  command: "php /app/vendor/bin/rr-worker start"
  relay: pipes
  user: www-data
  group: www-data

rpc:
  listen: tcp://:6001

http:
  address: 0.0.0.0:8000
  pool:
    num_workers: 0
    max_jobs: 500
    supervisor:
      exec_ttl: 300s

logs:
  mode: production
  level: error
  encoding: json

status:
  address: localhost:2114

reload:
  interval: 1s
  patterns:
    - ".php"
  services:
    http:
      recursive: true
      ignore:
        - "bin"
        - "database"
        - "docs"
        - "logs"
        - "ops"
        - "storage"
        - "submodules"
        - "tests"
        - "tmp"
        - "vendor"
      patterns:
        - ".json"
        - ".md"
        - ".php"
        - ".py"
        - ".sh"
        - ".yml"
      dirs:
        - "."

I have checked the information provided in the caveats wiki section (which mentions a similar issue regarding the upload of files) and also the information in this issue.

I have also tried changing the isValid() function of the UploadedFile.php Synfony class and it also did not work

Thanks!

Version (rr --version)

2.11.3

Relevant log output

2022-10-06T15:35:18.412Z	DEBUG	server      	worker is allocated	{"pid": 13716, "internal_event_name": "EventWorkerConstruct"}

2022-10-06T15:35:18.416Z	INFO	server      	In Psr17Factory.php line 49:
   The file "/tmp/upload3578791433" cannot be opened:
 start [--laravel-path [LARAVEL-PATH]] [--relay-dsn RELAY-DSN] [--refresh-app] [--worker-mode WORKER-MODE]

2022-10-06T15:35:18.419Z	ERROR	http        	execute	{"start": "2022-10-06T15:35:18.158Z", "elapsed": "260.795709ms", "error": "sync_worker_exec_worker_with_timeout: Network:\n\tsync_worker_exec_payload: EOF"}

2022-10-06T15:35:18.419Z	INFO	http        	http log	{"status": 500, "method": "POST", "URI": "/2.0/user/update", "remote_address": "127.0.0.1:43024", "read_bytes": 360038, "write_bytes": 0, "start": "2022-10-06T15:35:18.158Z", "elapsed": "261.082333ms"}

nginx access_log 172.18.0.1 "POST /2.0/user/update HTTP/1.1" 500 "curl/7.79.1" 0 0.263
@rustatian
Copy link
Member

rustatian commented Oct 6, 2022

Hey @egonbraun 👋🏻
Could you please try to remove:

server:
  command: "php /app/vendor/bin/rr-worker start"
  relay: pipes
  user: www-data <--- THIS
  group: www-data <--- THIS

I suspect, that there are not enough permissions to open the file.

@rustatian rustatian added this to General Oct 6, 2022
@rustatian rustatian moved this to Backlog in General Oct 6, 2022
@rustatian rustatian moved this from Backlog to Discuss in General Oct 6, 2022
@egonbraun
Copy link
Author

Hey @rustatian !

Surprisingly that fixed it. But now I have even more questions! hahahaha

I checked /tmp and it is properly set in my system:

$ ls -ld /tmp
drwxrwxrwt 1 root root 4096 Oct  7 14:57 /tmp

And this is how the www-data user is set:

$ id www-data
uid=33(www-data) gid=33(www-data) groups=33(www-data)

So, are you suggesting this is a permissions issue or is Roadrunner doing something wrong?

Thanks for the fix!

@rustatian
Copy link
Member

Hey @egonbraun 👋🏻
So, I guess you started RR from the root user, right?

@egonbraun
Copy link
Author

Correct.

@rustatian
Copy link
Member

rustatian commented Oct 7, 2022

So, when the RR process creates a file in the tmp folder, it creates it with the root permissions. But child processes are created with a different user (www-data).
Then, when a PHP process touches this file, it just has no permission to do that. In this case, I highly recommend using a regular user to start a RR with proper permissions.

@rustatian
Copy link
Member

Hey @egonbraun 👋🏻
If the issue was resolved, feel free to close it (if you don't have any questions) 😃

@egonbraun
Copy link
Author

Done! Thanks!

Repository owner moved this from Discuss to Unreleased in General Oct 10, 2022
@egonbraun
Copy link
Author

But one thing I still don't get.

Roadrunner was indeed set to start as www-data user as you can see in my config, and Roadrunner is the one responsible for starting the PHP process.

The main Roadrunner process was root but the workers were www-data which I believe is a common daemon behaviour in Linux systems.

So, how can I do what you are proposing?

Thanks again!

@rustatian
Copy link
Member

It would be best if you did not generally start RR from the root permissions. RR is not a webserver, it's the application server. It won't require root permissions. Just create a user, then put www-data and the created user in the same group.

common daemon behaviour in Linux

Not all daemons require a root. For example systemctl --user systemd unit services.

@egonbraun
Copy link
Author

egonbraun commented Oct 10, 2022

Maybe I am just being dumb, so apologies for that. :)

Do you mean that the binary of roadrunner should be started as a user different than root? I mean, we user supervisor, so I can ask supervisor to start the roadrunner bootstrap script as a different user.

Yeah, if that's the case, then this is something I was definitely not doing. But it's totally doable.

@rustatian
Copy link
Member

No no, you're asking good questions. I'm thinking about the permissions... I mean, should we create tmp files from the same user specified in the configuration? I guess yes. In this case, this is the RR bug... But, I don't think that the RR should be executed from the root user.

@rustatian
Copy link
Member

But, PHP should not touch these files. The original problem I guess is the roadrunner-laravel package, it should not touch the external files created by the RR.

@rustatian
Copy link
Member

Since we're not actively supporting this package (you know, we're busy with the SpiralFramework, which doesn't have this problem), The better way to solve this problem would be a PR 😃

@rustatian
Copy link
Member

@egonbraun Hey 👋🏻
I looked at this problem a bit deeper; RR sends a file link to the PHP worker. So, this is a RR bug.

@rustatian rustatian reopened this Oct 11, 2022
Repository owner moved this from Unreleased to Discuss in General Oct 11, 2022
@egonbraun
Copy link
Author

Got it! For now I will keep running rr as root until this issue is fixed then.

Thanks @rustatian :)

@rustatian
Copy link
Member

My pleasure 👍🏻
It'll be fixed this week, and since we're already started v2.12.0 preparation, I'll release a v2.12.0-beta.1 with this fix, so you may safely use it.

@rustatian rustatian added this to the v2.12.0 milestone Oct 11, 2022
@rustatian rustatian moved this from Discuss to Todo in General Oct 11, 2022
Repository owner moved this from Todo to Unreleased in General Oct 14, 2022
@rustatian
Copy link
Member

@rustatian
Copy link
Member

Hey @egonbraun 👋🏻
Have you had a chance to test the fix?

@rustatian rustatian mentioned this issue Nov 24, 2022
6 tasks
@rustatian rustatian moved this from Unreleased to Done in General Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-bug Bug: bug, exception F-need-verification
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants