From 4c0a39da0bfbaeda84681588a5386d295c051fa9 Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Wed, 8 May 2019 20:40:33 +0000 Subject: [PATCH 1/3] Add additional filesystem checks for OCI devices This adds checks for the default OCI devices to our conformance test for filesystem validation. This test also refactors where the file paths to check are located to reduce the number of transformations and simplify adding additional paths. Fixes #2973 --- test/conformance/envvars_test.go | 16 ++- test/conformance/filesystem_perm_test.go | 92 ----------------- test/conformance/filesystem_test.go | 96 ++++++++++++++++++ test/conformance/runtime_contract_types.go | 67 ------------ test/test_images/runtime/handlers/file.go | 32 +++--- test/test_images/runtime/handlers/runtime.go | 11 ++ test/types/runtime.go | 101 ++++++++++++++++++- 7 files changed, 231 insertions(+), 184 deletions(-) delete mode 100644 test/conformance/filesystem_perm_test.go create mode 100644 test/conformance/filesystem_test.go delete mode 100644 test/conformance/runtime_contract_types.go diff --git a/test/conformance/envvars_test.go b/test/conformance/envvars_test.go index 02dea99636cf..be1846a51404 100644 --- a/test/conformance/envvars_test.go +++ b/test/conformance/envvars_test.go @@ -20,9 +20,11 @@ package conformance import ( "reflect" + "strconv" "testing" "github.com/knative/serving/test" + "github.com/knative/serving/test/types" corev1 "k8s.io/api/core/v1" ) @@ -35,7 +37,7 @@ func TestShouldEnvVars(t *testing.T) { t.Fatal(err) } r := reflect.ValueOf(names) - for k, v := range ShouldEnvVars { + for k, v := range types.ShouldEnvVars { value, exist := ri.Host.EnvVars[k] if !exist { t.Fatalf("Runtime contract env variable %q is not set", k) @@ -51,15 +53,23 @@ func TestShouldEnvVars(t *testing.T) { func TestMustEnvVars(t *testing.T) { t.Parallel() clients := setup(t) + portStr, ok := types.MustEnvVars["PORT"] + if !ok { + t.Fatalf("Missing PORT from set of MustEnvVars") + } + port, err := strconv.Atoi(portStr) + if err != nil { + t.Fatalf("Invalid PORT value in MustEnvVars") + } _, ri, err := fetchRuntimeInfo(t, clients, &test.Options{ ContainerPorts: []corev1.ContainerPort{ - {ContainerPort: int32(mustEnvCustomPort)}, + {ContainerPort: int32(port)}, }, }) if err != nil { t.Fatal(err) } - for k, v := range MustEnvVars { + for k, v := range types.MustEnvVars { value, exist := ri.Host.EnvVars[k] if !exist { t.Fatalf("Runtime contract env variable %q is not set", k) diff --git a/test/conformance/filesystem_perm_test.go b/test/conformance/filesystem_perm_test.go deleted file mode 100644 index 3775ab36fe72..000000000000 --- a/test/conformance/filesystem_perm_test.go +++ /dev/null @@ -1,92 +0,0 @@ -// +build e2e - -/* -Copyright 2018 The Knative Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -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. -*/ - -package conformance - -import ( - "fmt" - "testing" - - "github.com/knative/serving/test" -) - -func verifyPermString(resp string, expected string) error { - if len(resp) != len(expected) { - return fmt.Errorf("Length of expected and response string don't match. Expected Response: %q Received Response: %q Expected length: %d Received length: %d", - expected, resp, len(expected), len(resp)) - } - - for index := range expected { - if string(expected[index]) != "*" && expected[index] != resp[index] { - return fmt.Errorf("Permission strings don't match at Index: %d. Expected: %q Received Response: %q", - index, expected, resp) - } - } - - return nil -} - -func testFileSystemPermissions(t *testing.T, clients *test.Clients, paths map[string]FilePathInfo) error { - _, ri, err := fetchRuntimeInfo(t, clients, &test.Options{}) - if err != nil { - return err - } - - for path, file := range paths { - found := false - for _, riFile := range ri.Host.Files { - if path != riFile.Name { - continue - } - found = true - if file.IsDirectory != *riFile.IsDir { - return fmt.Errorf("%s isDirectory = %t, want: %t", riFile.Name, *riFile.IsDir, file.IsDirectory) - } - index := len(riFile.Mode) - len(file.PermString) - if index < 0 { - index = 0 - } - if err := verifyPermString(riFile.Mode[index:], file.PermString); err != nil { - return err - } - break - } - if !found { - return fmt.Errorf("Runtime contract file %q doesn't exist", path) - } - } - return nil -} - -// TestMustHaveFileSystemPermissions asserts that the file system has all the MUST have paths and they have appropriate permissions. -func TestMustHaveFileSystemPermissions(t *testing.T) { - t.Parallel() - clients := setup(t) - if err := testFileSystemPermissions(t, clients, MustFilePathSpecs); err != nil { - t.Error(err) - } -} - -// TestShouldHaveFileSystemPermissions asserts that the file system has all the SHOULD have paths and they have appropriate permissions. -func TestShouldHaveFileSystemPermissions(t *testing.T) { - t.Parallel() - clients := setup(t) - if err := testFileSystemPermissions(t, clients, ShouldFilePathSpecs); err != nil { - t.Error(err) - } -} diff --git a/test/conformance/filesystem_test.go b/test/conformance/filesystem_test.go new file mode 100644 index 000000000000..8a40d24053c7 --- /dev/null +++ b/test/conformance/filesystem_test.go @@ -0,0 +1,96 @@ +// +build e2e + +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +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. +*/ + +package conformance + +import ( + "fmt" + "testing" + + "github.com/knative/serving/test" + "github.com/knative/serving/test/types" +) + +func verifyModeString(resp string, expected string) error { + if len(resp) != len(expected) { + return fmt.Errorf("length of expected and response string don't match. Expected Response: %q Received Response: %q Expected length: %d Received length: %d", + expected, resp, len(expected), len(resp)) + } + + for index := range expected { + if string(expected[index]) != "*" && expected[index] != resp[index] { + return fmt.Errorf("mode strings don't match at Index: %d. Expected: %q Received Response: %q", + index, expected, resp) + } + } + + return nil +} + +func testFiles(t *testing.T, clients *test.Clients, paths map[string]types.FileInfo) error { + _, ri, err := fetchRuntimeInfo(t, clients, &test.Options{}) + if err != nil { + return err + } + + for path, file := range paths { + riFile, ok := ri.Host.Files[path] + if !ok { + return fmt.Errorf("runtime contract file info not present: %s", path) + } + + if file.Error != "" && file.Error != riFile.Error { + return fmt.Errorf("%s.Error = %s, want: %s", path, riFile.Error, file.Error) + } + + if file.IsDir != nil && *file.IsDir != *riFile.IsDir { + return fmt.Errorf("%s.IsDir = %t, want: %t", path, *riFile.IsDir, *file.IsDir) + } + + if file.SourceFile != "" && file.SourceFile != riFile.SourceFile { + return fmt.Errorf("%s.SourceFile = %s, want: %s", path, riFile.SourceFile, file.SourceFile) + } + + if file.Mode != "" { + if err := verifyModeString(riFile.Mode, file.Mode); err != nil { + return fmt.Errorf("%s has invalid mode string: %v", path, err) + } + } + } + return nil +} + +// TestMustHaveFiles asserts that the file system has all the MUST have paths and they have appropriate permissions +// and type as applicable. +func TestMustHaveFiles(t *testing.T) { + t.Parallel() + clients := setup(t) + if err := testFiles(t, clients, types.MustFiles); err != nil { + t.Error(err) + } +} + +// TestShouldHaveFiles asserts that the file system has all the SHOULD have paths and that they have the appropriate +// permissions and type as applicable. +func TestShouldHaveFiles(t *testing.T) { + t.Parallel() + clients := setup(t) + if err := testFiles(t, clients, types.ShouldFiles); err != nil { + t.Error(err) + } +} diff --git a/test/conformance/runtime_contract_types.go b/test/conformance/runtime_contract_types.go deleted file mode 100644 index 11b1dd977571..000000000000 --- a/test/conformance/runtime_contract_types.go +++ /dev/null @@ -1,67 +0,0 @@ -/* -Copyright 2018 The Knative Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -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. -*/ - -package conformance - -import ( - "strconv" -) - -const mustEnvCustomPort = 8800 - -//runtime_constract_types.go defines types that encapsulate run-time contract requirements as specified here: https://github.com/knative/serving/blob/master/docs/runtime-contract.md - -// ShouldEnvVars defines environment variables that "SHOULD" be set. -// To match these values with test service parameters, -// map values must represent corresponding test.ResourceNames fields -var ShouldEnvVars = map[string]string{ - "K_SERVICE": "Service", - "K_CONFIGURATION": "Config", - "K_REVISION": "Revision", -} - -// MustEnvVars defines environment variables that "MUST" be set. -var MustEnvVars = map[string]string{ - "PORT": strconv.Itoa(mustEnvCustomPort), -} - -// FilePathInfo data object returned by the environment test-image. -type FilePathInfo struct { - FilePath string - IsDirectory bool - PermString string -} - -// MustFilePathSpecs specifies the file-paths and expected permissions that MUST be set as specified in the runtime contract. -var MustFilePathSpecs = map[string]FilePathInfo{ - "/tmp": { - IsDirectory: true, - PermString: "rw*rw*rw*", // * indicates no specification - }, - "/var/log": { - IsDirectory: true, - PermString: "rw*rw*rw*", // * indicates no specification - }, - // TODO(#822): Add conformance tests for "/dev/log". -} - -// ShouldFilePathSpecs specifies the file-paths and expected permissions that SHOULD be set as specified in the run-time contract. -var ShouldFilePathSpecs = map[string]FilePathInfo{ - "/etc/resolv.conf": { - IsDirectory: false, - PermString: "rw*r**r**", // * indicates no specification - }, -} diff --git a/test/test_images/runtime/handlers/file.go b/test/test_images/runtime/handlers/file.go index e8a249ad1fb4..308e45136097 100644 --- a/test/test_images/runtime/handlers/file.go +++ b/test/test_images/runtime/handlers/file.go @@ -19,31 +19,25 @@ import ( "github.com/knative/serving/test/types" ) -// filePaths is the set of filepaths probed and returned to the -// client as FileInfo. -var filePaths = []string{"/tmp", - "/var/log", - "/dev/log", - "/etc/hosts", - "/etc/hostname", - "/etc/resolv.conf"} - -func fileInfo(paths ...string) []*types.FileInfo { - var infoList []*types.FileInfo - +func fileInfo(paths ...string) map[string]types.FileInfo { + files := map[string]types.FileInfo{} for _, path := range paths { file, err := os.Stat(path) if err != nil { - infoList = append(infoList, &types.FileInfo{Name: path, Error: err.Error()}) + files[path] = types.FileInfo{Error: err.Error()} continue } size := file.Size() dir := file.IsDir() - infoList = append(infoList, &types.FileInfo{Name: path, - Size: &size, - Mode: file.Mode().String(), - ModTime: file.ModTime(), - IsDir: &dir}) + mode := file.Mode() + source, _ := os.Readlink(path) + + files[path] = types.FileInfo{ + Size: &size, + Mode: mode.String(), + ModTime: file.ModTime(), + SourceFile: source, + IsDir: &dir} } - return infoList + return files } diff --git a/test/test_images/runtime/handlers/runtime.go b/test/test_images/runtime/handlers/runtime.go index dcc993403b90..36c5402ecf14 100644 --- a/test/test_images/runtime/handlers/runtime.go +++ b/test/test_images/runtime/handlers/runtime.go @@ -24,6 +24,17 @@ func runtimeHandler(w http.ResponseWriter, r *http.Request) { log.Println("Retrieving Runtime Information") w.Header().Set("Content-Type", "application/json") + filePaths := make([]string, len(types.MustFiles)+len(types.ShouldFiles)) + i := 0 + for key := range types.MustFiles { + filePaths[i] = key + i++ + } + for key := range types.ShouldFiles { + filePaths[i] = key + i++ + } + k := &types.RuntimeInfo{ Request: requestInfo(r), Host: &types.HostInfo{EnvVars: env(), diff --git a/test/types/runtime.go b/test/types/runtime.go index d13816d330f8..bc6ede99981c 100644 --- a/test/types/runtime.go +++ b/test/types/runtime.go @@ -16,8 +16,102 @@ package types import ( "net/http" "time" + + "github.com/knative/pkg/ptr" ) +// MustEnvVars defines environment variables that "MUST" be set. +// The value provided is an example value. +var MustEnvVars = map[string]string{ + "PORT": "8801", +} + +// ShouldEnvVars defines environment variables that "SHOULD" be set. +// To match these values with test service parameters, +// map values must represent corresponding test.ResourceNames fields +var ShouldEnvVars = map[string]string{ + "K_SERVICE": "Service", + "K_CONFIGURATION": "Config", + "K_REVISION": "Revision", +} + +// MustFilePathSpecs specifies the file-paths and expected permissions that MUST be set as specified in the runtime contract. +// See https://golang.org/pkg/os/#FileMode for "Mode" string meaning. '*' indicates no specification. +var MustFiles = map[string]FileInfo{ + "/dev/fd": { + IsDir: ptr.Bool(true), + SourceFile: "/proc/self/fd", + }, + "/dev/full": { + IsDir: ptr.Bool(false), + }, + // TODO(#822): Add conformance tests for "/dev/log" once implemented + //"/dev/log": { + //}, + "/dev/null": { + IsDir: ptr.Bool(false), + }, + "/dev/ptmx": { + IsDir: ptr.Bool(false), + }, + "/dev/random": { + IsDir: ptr.Bool(false), + }, + "/dev/stdin": { + IsDir: ptr.Bool(false), + SourceFile: "/proc/self/fd/0", + }, + "/dev/stdout": { + IsDir: ptr.Bool(false), + SourceFile: "/proc/self/fd/1", + }, + "/dev/stderr": { + IsDir: ptr.Bool(false), + SourceFile: "/proc/self/fd/2", + }, + "/dev/tty": { + IsDir: ptr.Bool(false), + }, + "/dev/urandom": { + IsDir: ptr.Bool(false), + }, + "/dev/zero": { + IsDir: ptr.Bool(false), + }, + "/proc/self/fd": { + IsDir: ptr.Bool(true), + }, + "/proc/self/fd/0": { + IsDir: ptr.Bool(false), + }, + "/proc/self/fd/1": { + IsDir: ptr.Bool(false), + }, + "/proc/self/fd/2": { + IsDir: ptr.Bool(false), + }, + "/tmp": { + IsDir: ptr.Bool(true), + Mode: "dtrwxrwxrwx", + }, + "/var/log": { + IsDir: ptr.Bool(true), + Mode: "drwxrwxrwx", + }, +} + +// ShouldFilePathSpecs specifies the file paths and expected permissions that SHOULD be set as specified in the runtime contract. +// See https://golang.org/pkg/os/#FileMode for "Mode" string meaning. '*' indicates no specification. +var ShouldFiles = map[string]FileInfo{ + "/etc/resolv.conf": { + IsDir: ptr.Bool(false), + Mode: "*rw*r**r**", + }, + "/dev/console": { // This file SHOULD NOT exist. + Error: "stat /dev/console: no such file or directory", + }, +} + // RuntimeInfo encapsulates both the host and request information. type RuntimeInfo struct { // Request is information about the request. @@ -43,7 +137,7 @@ type RequestInfo struct { // HostInfo contains information about the host environment. type HostInfo struct { // Files is a map of file metadata. - Files []*FileInfo `json:"files"` + Files map[string]FileInfo `json:"files"` // EnvVars is a map of all environment variables set. EnvVars map[string]string `json:"envs"` // Cgroups is a list of cgroup information. @@ -72,14 +166,15 @@ type UserInfo struct { // FileInfo contains the metadata for a given file. type FileInfo struct { - // Name is the full filename. - Name string `json:"name"` // Size is the length in bytes for regular files; system-dependent for others. Size *int64 `json:"size,omitempty"` // Mode is the file mode bits. Mode string `json:"mode,omitempty"` // ModTime is the file last modified time ModTime time.Time `json:"modTime,omitempty"` + // SourceFile is populated if this file is a symlink. The SourceFile is the file where + // the symlink resolves. + SourceFile string `json:"sourceFile,omitempty"` // IsDir is true if the file is a directory. IsDir *bool `json:"isDir,omitempty"` // Error is the String representation of the error returned obtaining the information. From b6270020214d1d63f428850e83391d6283b515dc Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Fri, 10 May 2019 21:29:20 +0000 Subject: [PATCH 2/3] Fix comments --- test/types/runtime.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/types/runtime.go b/test/types/runtime.go index bc6ede99981c..5462bdbffe0f 100644 --- a/test/types/runtime.go +++ b/test/types/runtime.go @@ -35,7 +35,7 @@ var ShouldEnvVars = map[string]string{ "K_REVISION": "Revision", } -// MustFilePathSpecs specifies the file-paths and expected permissions that MUST be set as specified in the runtime contract. +// MustFiles specifies the file paths and expected permissions that MUST be set as specified in the runtime contract. // See https://golang.org/pkg/os/#FileMode for "Mode" string meaning. '*' indicates no specification. var MustFiles = map[string]FileInfo{ "/dev/fd": { @@ -100,7 +100,7 @@ var MustFiles = map[string]FileInfo{ }, } -// ShouldFilePathSpecs specifies the file paths and expected permissions that SHOULD be set as specified in the runtime contract. +// ShouldFiles specifies the file paths and expected permissions that SHOULD be set as specified in the runtime contract. // See https://golang.org/pkg/os/#FileMode for "Mode" string meaning. '*' indicates no specification. var ShouldFiles = map[string]FileInfo{ "/etc/resolv.conf": { From 96a1ae2142c4e5458a21aa967836dbebb2b8c4dd Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Fri, 10 May 2019 21:49:24 +0000 Subject: [PATCH 3/3] Code review comments --- test/conformance/envvars_test.go | 4 ++-- test/conformance/filesystem_test.go | 11 ++++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/test/conformance/envvars_test.go b/test/conformance/envvars_test.go index be1846a51404..4eddbbf7e955 100644 --- a/test/conformance/envvars_test.go +++ b/test/conformance/envvars_test.go @@ -55,11 +55,11 @@ func TestMustEnvVars(t *testing.T) { clients := setup(t) portStr, ok := types.MustEnvVars["PORT"] if !ok { - t.Fatalf("Missing PORT from set of MustEnvVars") + t.Fatal("Missing PORT from set of MustEnvVars") } port, err := strconv.Atoi(portStr) if err != nil { - t.Fatalf("Invalid PORT value in MustEnvVars") + t.Fatal("Invalid PORT value in MustEnvVars") } _, ri, err := fetchRuntimeInfo(t, clients, &test.Options{ ContainerPorts: []corev1.ContainerPort{ diff --git a/test/conformance/filesystem_test.go b/test/conformance/filesystem_test.go index 8a40d24053c7..e254a7046128 100644 --- a/test/conformance/filesystem_test.go +++ b/test/conformance/filesystem_test.go @@ -28,17 +28,14 @@ import ( func verifyModeString(resp string, expected string) error { if len(resp) != len(expected) { - return fmt.Errorf("length of expected and response string don't match. Expected Response: %q Received Response: %q Expected length: %d Received length: %d", - expected, resp, len(expected), len(resp)) + return fmt.Errorf("mode = %q (len:%d), want: %q (len:%d)", resp, len(resp), expected, len(expected)) } for index := range expected { - if string(expected[index]) != "*" && expected[index] != resp[index] { - return fmt.Errorf("mode strings don't match at Index: %d. Expected: %q Received Response: %q", - index, expected, resp) + if expected[index] != '*' && expected[index] != resp[index] { + return fmt.Errorf("mode[%d] = %c, want: %c", index, expected[index], resp[index]) } } - return nil } @@ -68,7 +65,7 @@ func testFiles(t *testing.T, clients *test.Clients, paths map[string]types.FileI if file.Mode != "" { if err := verifyModeString(riFile.Mode, file.Mode); err != nil { - return fmt.Errorf("%s has invalid mode string: %v", path, err) + return fmt.Errorf("%s has invalid mode string %s: %v", path, riFile.Mode, err) } } }