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

Add support for bind mount options and clean up connector tests #52

Merged
merged 22 commits into from
Mar 29, 2024

Conversation

webbnh
Copy link
Contributor

@webbnh webbnh commented Mar 27, 2024

Changes introduced with this PR

This PR is an extension of #47 and supersedes it: this change rebases the two commits from @rh-tguittet on the end of main, adds a series of commits containing a sequence of fixes/enhancements to the tests, and an enhancement to the bind test which checks multiple scenarios.

I suggest reviewing each of the commits individually (they should all be squashed when this PR is merged), highlights:

  • Reposition the t.Cleanup() invocations so that they might have a chance of being useful if the test actually fails or crashes.
  • Remove the various timing Sleep() calls and replace them with loops which wait for the required datum to appear; this addresses at least one of the flakey tests which was failing occasionally in the CI.
  • Fix a bug in TestCgroupNsByNamespacePath() where we were fetching the same datum twice and checking that it was the same, instead of fetching the corresponding values from the two different containers.
  • Rework readOutputUntil() to avoid a bug where it might omit from the returned value the last buffer read when an error such as EOF occurs.
  • Inline checkIfconfig(), since it's used in only one place and it could be reduced to a single line.
  • Replace a number of uses of assert.NoError() with assert.NoErrorR().
  • Change the existing TestSimpleVolume() function into a helper function which is called repeatedly by each of three new tests which use a table to generate the various scenarios. The three tests correspond to three host platform types -- non-Linux, SELinux, and Linux with SELinux disabled -- and the tests mark themselves as "skipped" if they don't match the host platform. Modify the test to create the bind-mounted file on the fly, so that sharing the same file between the containers doesn't lead to conflict; tighten up the pass/fail assertion.

Fixes #46.


By contributing to this repository, I agree to the contribution guidelines.

Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have doubts about the modified test case, but I see improvement here even if I'm not convinced it's really testing the new capability... so ... a reluctant "approve"?

connector_test.go Outdated Show resolved Hide resolved
connector_test.go Outdated Show resolved Hide resolved
connector_test.go Show resolved Hide resolved
jaredoconnell

This comment was marked as resolved.

@mfleader

This comment was marked as resolved.

@webbnh

This comment was marked as resolved.

@webbnh webbnh force-pushed the rh-tguittet-patch-46 branch from 999bcf0 to 82bf625 Compare March 28, 2024 01:29
rh-tguittet and others added 15 commits March 27, 2024 21:41
Signed-off-by: Thibault Guittet <88336850+rh-tguittet@users.noreply.github.com>
Adding `:Z` to the volume definition seems to work for both SELinux and
non-SELinux systems. It turns out this allows to simplify the test.

On Fedora (SELinux: Enforcing):

	$ go test -run TestSimpleVolume
	PASS
	ok      go.flow.arcalot.io/podmandeployer       0.404s

On Ubuntu (Apparmor disabled):

	$ go test -run TestSimpleVolume
	PASS
	ok  	go.flow.arcalot.io/podmandeployer	1.346s
@webbnh webbnh force-pushed the rh-tguittet-patch-46 branch 2 times, most recently from c7471b8 to 022353e Compare March 28, 2024 01:48
@webbnh webbnh force-pushed the rh-tguittet-patch-46 branch from 022353e to 53cfa45 Compare March 28, 2024 01:54
@webbnh webbnh marked this pull request as draft March 28, 2024 02:01
@webbnh
Copy link
Contributor Author

webbnh commented Mar 28, 2024

Sorry for the churn, folks, but this problem doesn't seem to reproduce for me on CentOS-9, only in the CI on GitHub.

I'm doubting that the original test ever worked (in the CI). Looking at an old test run from #46 before Thibault changed the test, it's logging a warning:

failed to set SELinux permissions on folder, chcon error: exit status 1, this may cause test failure if SELinux is enabled.

With the old, looser pass/fail assertion, the test would have noted that the container ran successfully but would not have noted that the file was inaccessible inside the container, and it would have declared it passing. However, now that I've tightened up the pass/fail assertion, the test fails under these circumstances.

I've gradually started adding the SELinux stuff from the original code back in. And, I've enhanced the logging to report the SELinux error:

chcon error: exit status 1: chcon: can't apply partial context to unlabeled file 'test_file.txt'
    chcon: can't apply partial context to unlabeled file './tests/volume'

A small amount of Googling turned up a number of hits. It's possible that the chcon command arguments are just incomplete. But, until I/we figure out how to address the situation, I'm putting this PR into draft.

@jaredoconnell
Copy link
Contributor

Are the files it's trying to access being set to the correct SELinux type? There's a specific type for use in a container, container_t.

Copy link
Contributor

@jaredoconnell jaredoconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fixed for Mac OS. Looks good except the CI issue.

connector_test.go Outdated Show resolved Hide resolved
connector_test.go Outdated Show resolved Hide resolved
@webbnh
Copy link
Contributor Author

webbnh commented Mar 28, 2024

I'm now reasonably satisfied with the test behavior. There are now three separate test scenarios -- non-Linux, Linux with SELinux disabled, and SELinux enabled -- and they are enshrined in three separate tests which all use the same helper function, so the code is fairly DRY and not too hard to understand; and, each of the three tests marks itself "skipped" if it doesn't match the current platform, so it's clear what is going on and what is not.

However, the three tests are non-trivially different from each other...meaning, we potentially have different behaviors on each platform and therefore a certain lack of portability between them. Hopefully this is acceptable: the message is, if you want to be portable, either don't use bind mounts in your plugin, or arrange for your target platforms to be more homogeneous.

I've tested on the SELinux platform. The CI represents the non-SELinux Linux platform (and that works, too). If @jaredoconnell can verify the non-Linux (Mac OS X) platform, then I think we're good to go.

(Thanks for your patience!)

@webbnh webbnh marked this pull request as ready for review March 28, 2024 21:52
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! ... aside from a couple of comment comments. 😀

connector_test.go Outdated Show resolved Hide resolved
connector_test.go Outdated Show resolved Hide resolved
@webbnh webbnh requested a review from dbutenhof March 29, 2024 16:46
@webbnh webbnh merged commit a0b30ab into arcalot:main Mar 29, 2024
2 checks passed
@webbnh webbnh deleted the rh-tguittet-patch-46 branch March 29, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for volume options
5 participants