Check file access information in conformance tests #5102

shashwathi · 2019-08-08T19:30:04Z

Fixes #4083

Proposed Changes

Update file conformance test by attempting to read & write to files or dir

/lint
/area test-and-release

/assign @dgerd

knative-prow-robot

@shashwathi: 0 warnings.

In response to this:

Fixes #4083

Proposed Changes

Update file conformance test by attempting to read & write to files or dir

/lint
/area test-and-release

/assign @dgerd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

taragu · 2019-08-26T00:53:56Z

test/test_images/runtime/handlers/file_access_attempt.go

+type permissionBits uint32
+
+const (
+	userRead permissionBits = 1 << (9 - 1 - iota)


Should we assign blank identifiers for the ones that are not being used? Looks like only otherRead and otherWrite are used.

We've updated to only list the two we use.

savitaashture · 2019-09-24T04:42:11Z

test/test_images/runtime/handlers/file_access_attempt.go

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/


I think the copied license is not the correct format.
Empty line has to be there after line number 5 and 6

Nice catch, we've fixed this now

savitaashture · 2019-09-24T04:47:53Z

test/conformance/runtime/filesystem_test.go

+
+		riFileAccess, ok := ri.Host.FileAccess[path]
+		if ok && riFileAccess.ReadErr != "" {
+			return fmt.Errorf("Want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)


Suggested change

return fmt.Errorf("Want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)

return fmt.Errorf("want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)

savitaashture · 2019-09-24T04:48:13Z

test/conformance/runtime/filesystem_test.go

+			return fmt.Errorf("Want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)
+		}
+		if ok && riFileAccess.WriteErr != "" {
+			return fmt.Errorf("Want no write errors for file %s, got error: %s", path, riFileAccess.WriteErr)


Suggested change

return fmt.Errorf("Want no write errors for file %s, got error: %s", path, riFileAccess.WriteErr)

return fmt.Errorf("want no write errors for file %s, got error: %s", path, riFileAccess.WriteErr)

googlebot · 2019-09-30T16:08:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

andrew-su · 2019-09-30T16:11:29Z

@googlebot I consent.

googlebot · 2019-09-30T16:11:33Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

markusthoemmes

Is there a reason why we actually need to test to write and cannot rely on the permission bits to tell us?

markusthoemmes · 2019-10-28T09:52:43Z

test/test_images/runtime/handlers/runtime.go

+	var paths []string
+	for _, path := range filePaths {
+		excluded := false
+		for i := len(excludedPaths) - 1; i >= 0 && !excluded; i-- {


Why not do a way simpler:

for _, excluded := range excludedPaths { if path == excluded { excluded = true break } }

Thanks for suggestion. Updated 👍

shashwathi · 2019-10-28T18:04:43Z

@markusthoemmes : Great question. I believe the context lies in this conversation #4074 (review)

vagababov · 2019-10-28T18:28:10Z

test/test_images/runtime/handlers/file_access_attempt.go

+	"knative.dev/serving/test/types"
+)
+
+type permissionBits uint32


https://golang.org/pkg/os/#FileMode should work just as well?

Yeah both FileMode and permissionBits are both unint32. Reason we went with custom type in this test case was to add the ability to compare permission bits via hasPermission function. This makes the code more readable.

vagababov · 2019-10-28T18:29:53Z

test/test_images/runtime/handlers/file_access_attempt.go

+		return err
+	}
+	readFile, err := os.OpenFile(path, os.O_RDONLY, 0)
+	defer readFile.Close()


I am close to be sure that this will panic, if the error is not nil (since if file isn't there it won't open it).

if err != nil {return err} readFile.Close() return nil

test/test_images/runtime/handlers/file_access_attempt.go

knative-test-reporter-robot · 2019-10-28T19:25:33Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	pull-knative-serving-integration-tests	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

vagababov

/lgtm
/approve
/hold
for comment

vagababov · 2019-10-29T00:08:46Z

test/test_images/runtime/handlers/file_access_attempt.go

+	if err != nil {
+		return err
+	}
+	readFile.Close()


For fun of it we can do return readfile.Close(), but if you think that can't happen that's fine as well

knative-prow-robot · 2019-10-29T00:09:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashwathi, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [vagababov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

- Use permissions to determine whether we can read or write to file/directory. - Validate response body for file access information Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

- Thanks to Markus for suggestion about simplify the logic to get excluded file paths

shashwathi · 2019-10-29T16:46:51Z

@vagababov : Addressed your comment. Please review again.

shashwathi · 2019-10-29T16:47:19Z

/hold cancel

vagababov · 2019-10-29T20:26:07Z

/lgtm

knative-prow-robot assigned dgerd Aug 8, 2019

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Aug 8, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 8, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 8, 2019

knative-prow-robot requested review from dushyanthsc and tcnghia August 8, 2019 19:30

knative-prow-robot reviewed Aug 8, 2019

View reviewed changes

taragu reviewed Aug 26, 2019

View reviewed changes

savitaashture reviewed Sep 24, 2019

View reviewed changes

andrew-su force-pushed the file_conformance branch from e0acbfd to ddfe245 Compare September 30, 2019 16:08

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Sep 30, 2019

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Sep 30, 2019

markusthoemmes reviewed Oct 28, 2019

View reviewed changes

vagababov reviewed Oct 28, 2019

View reviewed changes

shashwathi force-pushed the file_conformance branch from 84d8bd4 to 177228a Compare October 28, 2019 18:36

vagababov reviewed Oct 28, 2019

View reviewed changes

shashwathi force-pushed the file_conformance branch from 177228a to 50a7667 Compare October 28, 2019 20:57

vagababov reviewed Oct 29, 2019

View reviewed changes

knative-prow-robot assigned vagababov Oct 29, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 29, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2019

shashwathi and others added 4 commits October 29, 2019 09:45

Add handler with file access info

86976a5

- Use permissions to determine whether we can read or write to file/directory. - Validate response body for file access information Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

Address comments

76f8c98

Address comments

f3c96a4

- Thanks to Markus for suggestion about simplify the logic to get excluded file paths

Address comment

49f5e57

shashwathi force-pushed the file_conformance branch from 50a7667 to 49f5e57 Compare October 29, 2019 16:45

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

knative-prow-robot merged commit 8fb75cc into knative:master Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check file access information in conformance tests #5102

Check file access information in conformance tests #5102

shashwathi commented Aug 8, 2019

knative-prow-robot left a comment

taragu Aug 26, 2019

andrew-su Sep 30, 2019

savitaashture Sep 24, 2019

andrew-su Sep 30, 2019

savitaashture Sep 24, 2019

savitaashture Sep 24, 2019

googlebot commented Sep 30, 2019

andrew-su commented Sep 30, 2019

googlebot commented Sep 30, 2019

markusthoemmes left a comment

markusthoemmes Oct 28, 2019

shashwathi Oct 28, 2019

shashwathi commented Oct 28, 2019

vagababov Oct 28, 2019

shashwathi Oct 28, 2019

vagababov Oct 28, 2019

knative-test-reporter-robot commented Oct 28, 2019

vagababov left a comment

vagababov Oct 29, 2019

knative-prow-robot commented Oct 29, 2019

shashwathi commented Oct 29, 2019

shashwathi commented Oct 29, 2019

vagababov commented Oct 29, 2019

	return fmt.Errorf("Want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)
	return fmt.Errorf("want no read errors for file %s, got error: %s", path, riFileAccess.ReadErr)

	return fmt.Errorf("Want no write errors for file %s, got error: %s", path, riFileAccess.WriteErr)
	return fmt.Errorf("want no write errors for file %s, got error: %s", path, riFileAccess.WriteErr)

Check file access information in conformance tests #5102

Check file access information in conformance tests #5102

Conversation

shashwathi commented Aug 8, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

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

googlebot commented Sep 30, 2019

andrew-su commented Sep 30, 2019

googlebot commented Sep 30, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashwathi commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-test-reporter-robot commented Oct 28, 2019

vagababov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Oct 29, 2019

shashwathi commented Oct 29, 2019

shashwathi commented Oct 29, 2019

vagababov commented Oct 29, 2019