use exact match for illegal path check

[zhangbo1882]

In our environment, the file system directory is as follows:

Tue Jun 04 22:28:35][#48# ]$df -h
Filesystem                 Size  Used Avail Use% Mounted on
tmpfs                       77G  104K   77G   1% /dev/shm
tmpfs                       31G  9.8M   31G   1% /run
tmpfs                      5.0M     0  5.0M   0% /run/lock
tmpfs                      4.0M     0  4.0M   0% /sys/fs/cgroup
/dev/mapper/atomicos-root  150G  144G  5.8G  97% /sysroot
/dev/vda2                  483M   84M  400M  18% /boot
/dev/vdc                   1.2T   87G  1.1T   8% /sysroot/home/centos/external

We have a directory named /sysroot. If the envoy config file is the that directory, envoy can not start up.

[2024-06-04 22:28:35.581][3382724][critical][main] [source/server/server.cc:131] error initializing configuration 'configs/envoy.yaml': Invalid path: configs/envoy.yaml
[2024-06-04 22:28:35.581][3382724][info][main] [source/server/server.cc:972] exiting
Invalid path: configs/envoy.yaml

In my mind, envoy should only check the default system directory such as /dev /sys /proc as illegal path.
So it is better to use exact match instead of startwith match.

[adisuissa]

Thanks, left a high-level comment.

[adisuissa]

/wait

[adisuissa]

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only]

@envoyproxy/senior-maintainers assignee is @phlax

🐱

Caused by: a #34539 (review) was submitted by @adisuissa.

see: more, trace.

[adisuissa]

Assigning Harvey for a senior-maintainer pass.
/assign @htuch

Waiting for comment update.
/wait

[htuch]

LGTM

[zhangbo1882]

@htuch , I removed the test case I added in last commit. Because in the CI test environment, there is no /sysroot directory, so the function canonicalPath can not return a valid path so that /sysroot will always be considered as illegal, the test case will fail.

EXPECT_FALSE(file_system_.illegalPath("/sysroot/"));

In my local system, I have the /sysroot directory, the test case will pass.
I do not have any good ideas to add the test cases in CI environment except the following way

  const Api::SysCallStringResult canonical_path = canonicalPath("/sysroot");
  if (canonical_path.return_value_.empty()) {
    EXPECT_TRUE(file_system_.illegalPath("/sysroot")); 
  } else {
    EXPECT_FALSE(file_system_.illegalPath("/sysroot")); 
  }

But I do not think it is necessary to have above test code. What's your opinion?

[htuch]

I think the whole test should move to a mock environment for filesystem impl, to avoid the CI machine dependency. This probably will save some grief in the future. We could leave one or two tests using the real /dev maybe, which should be standard, and mock the rest.

Concretely what this means here is that in the canonicalPath implementation, we shouldn't use ::realpath directly, but some variant in os_syscalls_impl.cc that can be mocked out by replacing the OS syscall singleton

[zhangbo1882]

If we need to refactor the whole testing for filesystem impl, can we have another PR to do that?

[htuch]

@zhangbo1882 that's fine if you're willing to do that.

[zhangbo1882]

I will try my best to do that. Do we need to create a issue for that ?

[htuch]

OK, LGTM providing there is some test followup.