linux_masked_paths integration test #2950

nayuta-ai · 2024-10-09T16:47:13Z

Summary

Create linux_masked_paths integration test.

Related Issue

#361

Refer

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

nayuta-ai · 2024-10-13T15:10:09Z

@utam0k Thank you for the review! Please review my code again.

YJDoc2

I still have to check the main test implementation, have a couple of comments, and clippy CI is failing, so please fix that as well.

tests/contest/runtimetest/src/tests.rs

nayuta-ai · 2024-10-19T17:13:57Z

@utam0k @YJDoc2
I have completed the revisions. Please check!

YJDoc2 · 2024-10-22T04:26:35Z

I have completed the revisions. Please check!

Hey, thanks a lot, I'll try to get to this today or so.

YJDoc2

sorry it took me a while to get to this. Have some questions and comments, please take a look.

YJDoc2 · 2024-10-28T18:58:51Z

tests/contest/runtimetest/src/utils.rs

+        Err(_) => {
+            Ok(false) // If there's an error, then it's not readable
+        }


nit: can there be any kind of error which are unrelated, and do not actually indicate readability?

Yes, it is possible. Basically, it is the case that there is no file in the path, but there are also temporary errors, such as network issues.

ok. How frequently have you run into these other errors if at all? If the frequency of them is small, then this is ok. If that is the case, please reply and mark this comment as resolved, as no change is needed.

Apologies for the additional comment. I revised this to return an error and expect only the ENOENT case, which I believe addresses the concern.
8ec531f

YJDoc2 · 2024-10-28T19:00:47Z

tests/contest/runtimetest/src/utils.rs

+            if contents.is_empty() {
+                // empty file
+                return Ok(false);
+            }


Even though file is empty, getting Ok(_) would mean the operation succeeded and we have read access, right? And can you comment this code, or (if possible) restructure. it was a little confusing when reading.

In a masked path, references to file names are possible, but the contents of the file cannot be accessed. Therefore, in the read access test, we check whether the file is empty. According to https://github.com/opencontainers/runtime-tools/blob/master/cmd/runtimetest/main.go#L416, the test for masked paths can determine if the file is empty by checking if the number of bytes read with read is 0, which indicates EOF.

I think it would be good to refactor it as shown below, so I would like to modify this implementation approach and add comments. Is this method sufficient?

Suggested change

if contents.is_empty() {

// empty file

return Ok(false);

}

fn test_file_read_access(path: &str) -> Result<bool, std::io::Error> {

let mut file = OpenOptions::new().create(false).read(true).open(path)?;

// Create a buffer with a capacity of 1 byte

let mut buffer = [0u8; 1];

match file.read(&mut buffer) {

Ok(0) => Ok(false),

Ok(_) => Ok(true),

Err(e) => Err(e),

}

}

ok, so going in original test, this comment explains that if we get eof (or in our case a 0 bytes read) that indicates blocked read, because the tests use files with some content. Please add a similar comment.

Also in your refactored approach, you need to account for EOF error like the current code has. Other than that the refactored code has much better readability.

Also see this

tests/contest/runtimetest/src/tests.rs

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

YJDoc2 · 2024-10-28T19:05:46Z

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

+        match fs::metadata(&test_file) {
+            io::Result::Ok(md) => {
+                bail!(
+                    "reading path {:?} should have given error, found {:?} instead",
+                    test_file,
+                    md
+                )
+            }
+            io::Result::Err(e) => {
+                let err = e.kind();
+                if let io::ErrorKind::NotFound = err {
+                    Ok(())
+                } else {
+                    bail!("expected not found error, got {:?}", err);
+                }
+            }
+        }
+    })


Why is this check here? we should be checking this in the runtime test and here we should be creating the path, right? Or did I misunderstood anything?

When setting a masked path, it is impossible to determine whether it is a relative path, so I added this to verify whether it meets the test requirements at the time of creation. This implementation is also adopted in runtime-tools, so verification is necessary.

ok, so I see that you have carried over the implementation from go tests, but I'm still having a bit of difficulty understanding this -

The go code seems to checks that if error is not null and error is not found, then return error, else return null. I.e. if the error is not found, then the prepare function will return error, other wise not. Here the behavior seems to be opposite.

Also, this function/closure we pass, both here and in go code, will be executed before runtime has done anything. This is supposed to prepare the rootfs for tests running inside the container. Given that, what exactly is this checking? This will be running on plain bundle fs that is created for each test, It is ofcourse not going to find any of the files.

What I'm thinking here is that even the go tests have slightly incorrect implementation (which has happened before). Can you confirm going through comments in go code and seeing what the test is supposed to check, and what it is actually validating?

There are cases where the Go implementation may be incorrect. I'll check it out.

I posted a question in the Issue because I couldn’t fully grasp the purpose of this test.（opencontainers/runtime-tools#780 ）

This is my own opinion, but I believe the test might be trying to illustrate a discrepancy where the path added to masked_path is specified from the host system’s perspective, resulting in an unintended relative path from / inside the container that doesn’t match the expected path.

As for addressing this test, we could:

Add the above comment

Wait for a response on the Issue before deciding

Delete the test

Refactor the test

If we choose to improve the test, I think the following approach could work:

Create a file inside the container we want to mask (masked-relpath).

Specify the relative path from the host system (./bundle/rootfs/masked-relpath) as the path to mask.

After creating the container, verify that the file is masked by checking if it exists at the expected path without the ./bundle/rootfs prefix.

Do you think this would be a good approach?

YJDoc2 · 2024-10-28T19:06:31Z

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

+            }
+        };
+
+        match fs::metadata(r_path) {


same as one before.

The validation for fs::metadata will be similar to the following comment.
#2950 (comment)

YJDoc2 · 2024-10-28T19:07:05Z

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

+    }
+}
+
+fn test_node(mode: u32) -> TestResult {


Suggested change

fn test_node(mode: u32) -> TestResult {

fn test_mode(mode: u32) -> TestResult {

typo?

YJDoc2 · 2024-10-28T19:07:40Z

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs

+        if let TestResult::Failed(_) = res {
+            return res;
+        }
+        std::thread::sleep(std::time::Duration::from_millis(1000));


Are we sleeping to make sure the container exists? or some other reason? Please add a comment in code.

It is unnecessary for this test, so I have deleted it.
8ec531f

YJDoc2 · 2024-11-01T16:35:21Z

@nayuta-ai , I have replied to comments, also the PR has conflicts with main, Please make the suggested changes and resolve conflicts. Thanks.

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

…s.rs Signed-off-by: nayuta-ai <nayuta723@gmail.com>

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

nayuta-ai force-pushed the integration_test/linux_masked_paths branch from 063d558 to 6d0604c Compare October 9, 2024 16:51

YJDoc2 self-requested a review October 10, 2024 12:15

YJDoc2 added the kind/test label Oct 10, 2024

utam0k reviewed Oct 13, 2024

View reviewed changes

tests/contest/contest/src/tests/linux_masked_paths/masked_paths.rs Outdated Show resolved Hide resolved

nayuta-ai force-pushed the integration_test/linux_masked_paths branch from 52b4683 to 448504e Compare October 13, 2024 15:06

nayuta-ai requested a review from utam0k October 13, 2024 15:08

YJDoc2 requested changes Oct 14, 2024

View reviewed changes

tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved

tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved

nayuta-ai force-pushed the integration_test/linux_masked_paths branch from d3600be to 62e5e1a Compare October 15, 2024 15:19

YJDoc2 reviewed Oct 16, 2024

View reviewed changes

tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved

YJDoc2 reviewed Oct 16, 2024

View reviewed changes

tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved

nayuta-ai force-pushed the integration_test/linux_masked_paths branch 4 times, most recently from 7fa807b to 2baa120 Compare October 19, 2024 17:10

nayuta-ai requested a review from YJDoc2 October 19, 2024 17:14

YJDoc2 reviewed Oct 28, 2024

View reviewed changes

nayuta-ai force-pushed the integration_test/linux_masked_paths branch from 27f3230 to 48739f7 Compare November 1, 2024 16:38

nayuta-ai and others added 5 commits November 2, 2024 01:42

linux_masked_paths integration test

de3d429

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

Update tests/contest/contest/src/tests/linux_masked_paths/masked_path…

e06cdd6

…s.rs Signed-off-by: nayuta-ai <nayuta723@gmail.com>

fix the test coverage issues and resolve the lint errors

6371c03

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

fix: add logic to check if a file's contents or directory are empty

eedf25c

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

add some docstring and fix redundant code

8ec531f

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

nayuta-ai force-pushed the integration_test/linux_masked_paths branch from 48739f7 to 8ec531f Compare November 1, 2024 17:05

fix some errors

e95796f

Signed-off-by: nayuta-ai <nayuta723@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

linux_masked_paths integration test #2950

linux_masked_paths integration test #2950

nayuta-ai commented Oct 9, 2024

nayuta-ai commented Oct 13, 2024

YJDoc2 left a comment

nayuta-ai commented Oct 19, 2024

YJDoc2 commented Oct 22, 2024

YJDoc2 left a comment

YJDoc2 Oct 28, 2024

nayuta-ai Oct 31, 2024

YJDoc2 Nov 1, 2024

nayuta-ai Nov 1, 2024 •

edited

Loading

YJDoc2 Oct 28, 2024

nayuta-ai Oct 31, 2024 •

edited

Loading

nayuta-ai Oct 31, 2024

YJDoc2 Nov 1, 2024

YJDoc2 Nov 1, 2024

nayuta-ai Nov 1, 2024

YJDoc2 Oct 28, 2024

nayuta-ai Oct 31, 2024 •

edited

Loading

YJDoc2 Nov 1, 2024

nayuta-ai Nov 1, 2024

nayuta-ai Nov 3, 2024

YJDoc2 Oct 28, 2024

nayuta-ai Nov 3, 2024 •

edited

Loading

YJDoc2 Oct 28, 2024

YJDoc2 Oct 28, 2024

nayuta-ai Nov 1, 2024

YJDoc2 commented Nov 1, 2024

-            if contents.is_empty() {
-                // empty file
-                return Ok(false);
-            }
+fn test_file_read_access(path: &str) -> Result<bool, std::io::Error> {
+    let mut file = OpenOptions::new().create(false).read(true).open(path)?;
+    // Create a buffer with a capacity of 1 byte
+    let mut buffer = [0u8; 1];
+    match file.read(&mut buffer) {
+        Ok(0) => Ok(false),
+        Ok(_) => Ok(true),
+        Err(e) => Err(e),
+    }
+}

	fn test_node(mode: u32) -> TestResult {
	fn test_mode(mode: u32) -> TestResult {

linux_masked_paths integration test #2950

Are you sure you want to change the base?

linux_masked_paths integration test #2950

Conversation

nayuta-ai commented Oct 9, 2024

Summary

Related Issue

Refer

nayuta-ai commented Oct 13, 2024

YJDoc2 left a comment

Choose a reason for hiding this comment

nayuta-ai commented Oct 19, 2024

YJDoc2 commented Oct 22, 2024

YJDoc2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayuta-ai Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayuta-ai Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayuta-ai Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayuta-ai Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YJDoc2 commented Nov 1, 2024

nayuta-ai Nov 1, 2024 •

edited

Loading

nayuta-ai Oct 31, 2024 •

edited

Loading

nayuta-ai Oct 31, 2024 •

edited

Loading

nayuta-ai Nov 3, 2024 •

edited

Loading