From ac54fa3693fcfc68886242f2648a222c50d2ca7a Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 20 Mar 2024 15:26:17 +0100 Subject: [PATCH 01/25] fixed integration tests Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 47ade0755d..1d2dcd3508 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "os" + "path" "path/filepath" "reflect" "testing" @@ -292,6 +293,53 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } } +// TestBuildWithoutHome ensures that running func without HOME fails for pack builder +func TestBuildWithoutHome(t *testing.T) { + root, cleanup := Mktemp(t) + defer cleanup() + t.Setenv("HOME", "") + verbose := true + + f := fn.Function{Runtime: "go", Name: "test-deploy-without-home", Root: root} + client := newClient(verbose) + if _, err := client.Build(context.Background(), f); err == nil { + t.Fatalf("expected an error for HOME not defined with pack builder, got none") + } + + // dont call del() because function wasnt deployed +} + +// TestDeployWithoutWritableDotConfig ensures that running client.New works without +// .config being accessable (write/read) +// TODO: change this test to for-loop of Runs with different dir permissions? +func TestDeployWithoutWritableDotConfig(t *testing.T) { + // defer Within(t, "tempdata/example.com/baddotconfig") + root, cleanup := Mktemp(t) + defer cleanup() + + t.Setenv("HOME", root) + verbose := true + + // write new .config with no perms + err := os.Mkdir(path.Join(root, ".config"), 0000) + if err != nil { + t.Error(err) + } + + f := fn.Function{Runtime: "go", Name: "test-deploy-no-perms-config", Root: root} + + // client with pack builder + client := newClient(verbose) + + // expect to fail on build for HOME not being defined with pack builder + _, _, err = client.New(context.Background(), f) + // this error should be 'permission denied' for trying to write to open .config + if err == nil { + t.Fatalf("expected an error but got nil") + } + // dont call del() because function wasnt deployed +} + // TestRemove ensures removal of a function instance. func TestRemove(t *testing.T) { defer Within(t, "testdata/example.com/remove")() From 72aadba93b4af03c9a27a0df73671c0bc6226d45 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Mon, 25 Mar 2024 08:09:17 +0100 Subject: [PATCH 02/25] docker config.json credentials test when HOME not defined Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 35 ++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 7522bde3fd..5581388772 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -465,6 +465,41 @@ func TestNewCredentialsProviderEmptyCreds(t *testing.T) { } } +// TestNewCredentialsSkipDockerConfigWhenNoHome ensures that docker .config +// credentials are NOT loaded when HOME is empty. +// (it would be at $HOME/.docker/config.json). Removing +// should result in returned loaded credentials instead. +func TestNewCredentialsSkipDockerConfigWhenNoHome(t *testing.T) { + resetHomeDir(t) + + // setup PATH, .docker/config.json (docker creds) and creds loader + withPopulatedDockerAuthConfig(t) + + helper := newInMemoryHelper() + setUpMockHelper("docker-credential-mock", helper)(t) + + helper.Add(&credentials.Credentials{ + ServerURL: "docker.io", + Username: dockerIoUser, + Secret: dockerIoUserPwd, + }) + + // have docker config credential loader but HOME is not defined -- should return nil + t.Setenv("HOME", "") + credentialsProvider := creds.NewCredentialsProvider(testConfigPath(t), + creds.WithVerifyCredentials(correctVerifyCbk), + ) + returnedCreds, err := credentialsProvider(context.Background(), "docker.io/someorg/someimage:sometag") + + // expect no credentials and error to be found here + if returnedCreds != (docker.Credentials{}) { + t.Errorf("expected no docker credentials to be found but got '%v'\n", returnedCreds) + } + if err != creds.ErrCredentialsNotFound { + t.Errorf("expected error to be '%v', but got '%v'", creds.ErrCredentialsNotFound, err) + } +} + func TestCredentialsProviderSavingFromUserInput(t *testing.T) { resetHomeDir(t) From c99d964ba8f2868fa1dbe1e889efb2a99d6fdd1d Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 28 Mar 2024 14:53:00 +0100 Subject: [PATCH 03/25] pack test Signed-off-by: gauron99 --- pkg/builders/buildpacks/builder_test.go | 73 +++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/pkg/builders/buildpacks/builder_test.go b/pkg/builders/buildpacks/builder_test.go index 7ed7acf9c7..9e703c157a 100644 --- a/pkg/builders/buildpacks/builder_test.go +++ b/pkg/builders/buildpacks/builder_test.go @@ -2,12 +2,15 @@ package buildpacks import ( "context" + "io/fs" "os" "path/filepath" "reflect" + "strings" "testing" pack "github.com/buildpacks/pack/pkg/client" + "github.com/pkg/errors" "knative.dev/func/pkg/builders" fn "knative.dev/func/pkg/functions" ) @@ -216,6 +219,76 @@ func TestBuild_Errors(t *testing.T) { } } +// TestBuild_HomeAndPermissions ensures that function fails during build while HOME is +// not defined and different .config permission scenarios +func TestBuild_HomeAndPermissions(t *testing.T) { + testCases := []struct { + name string + homePath bool + homePerms fs.FileMode + expectedErr bool + errContains string + }{ + {name: "xpct-fail - create pack client when HOME not defined", homePath: false, homePerms: 0, expectedErr: true, errContains: "$HOME is not defined"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc := tc + var ( + i = &mockImpl{} + b = NewBuilder( // Func Builder logic + WithName(builders.Pack), + WithImpl(i)) + f = fn.Function{ + Runtime: "go", + Build: fn.BuildSpec{Image: "example.com/parent/name"}, + } + ) + // set temporary dir and assign it as func directory + tempdir := t.TempDir() + f.Root = tempdir + + // setup HOME env and its perms + if tc.homePath == false { + t.Setenv("HOME", "") + } else { + // setup new home with perms + err := os.MkdirAll(filepath.Join(tempdir, "home"), tc.homePerms) + if err != nil { + t.Fatal(err) + } + t.Setenv("HOME", filepath.Join(tempdir, "home")) + } + // TODO: gauron99 -- add test for pack_home? + i.BuildFn = func(ctx context.Context, opts pack.BuildOptions) error { + packHome := os.Getenv("PACK_HOME") + if packHome == "" { + _, err := os.UserHomeDir() + if err != nil { + return errors.Wrap(err, "getting user home") + } + } + return nil + } + + err := b.Build(context.Background(), f, nil) + // error scenarios + if tc.expectedErr { + if err == nil { + t.Fatalf("expected error but got nil") + } else if !strings.Contains(err.Error(), tc.errContains) { + t.Fatalf("expected error message '%v' was not found in the actual error: '%v'", tc.errContains, err) + } + } else { + if err != nil { + t.Fatalf("didnt expect an error but got '%v'", err) + } + } + }) + } +} + type mockImpl struct { BuildFn func(context.Context, pack.BuildOptions) error } From 2dca860c18f7ab8ef13060b8cb7470df424168f1 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 28 Mar 2024 16:08:39 +0100 Subject: [PATCH 04/25] simplify Signed-off-by: gauron99 --- pkg/builders/buildpacks/builder_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/builders/buildpacks/builder_test.go b/pkg/builders/buildpacks/builder_test.go index 9e703c157a..511b5635b9 100644 --- a/pkg/builders/buildpacks/builder_test.go +++ b/pkg/builders/buildpacks/builder_test.go @@ -220,7 +220,8 @@ func TestBuild_Errors(t *testing.T) { } // TestBuild_HomeAndPermissions ensures that function fails during build while HOME is -// not defined and different .config permission scenarios +// not defined +// TODO: can add more options with permissions for testing func TestBuild_HomeAndPermissions(t *testing.T) { testCases := []struct { name string @@ -229,6 +230,7 @@ func TestBuild_HomeAndPermissions(t *testing.T) { expectedErr bool errContains string }{ + // TODO: gauron99 -- maybe add test for PACK_HOME/ specific paths -- would require to change the impl {name: "xpct-fail - create pack client when HOME not defined", homePath: false, homePerms: 0, expectedErr: true, errContains: "$HOME is not defined"}, } @@ -260,14 +262,10 @@ func TestBuild_HomeAndPermissions(t *testing.T) { } t.Setenv("HOME", filepath.Join(tempdir, "home")) } - // TODO: gauron99 -- add test for pack_home? i.BuildFn = func(ctx context.Context, opts pack.BuildOptions) error { - packHome := os.Getenv("PACK_HOME") - if packHome == "" { - _, err := os.UserHomeDir() - if err != nil { - return errors.Wrap(err, "getting user home") - } + _, err := os.UserHomeDir() + if err != nil { + return errors.Wrap(err, "getting user home") } return nil } From ad43fbdb78e202987fdbc4f4962e5a69106dde4d Mon Sep 17 00:00:00 2001 From: gauron99 Date: Fri, 5 Apr 2024 10:33:42 +0200 Subject: [PATCH 05/25] og creds, small fixes Signed-off-by: gauron99 --- pkg/docker/creds/credentials.go | 4 +++- pkg/docker/creds/credentials_test.go | 6 ++++-- pkg/functions/client_int_test.go | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index 9825ba87a5..0de7fee1e7 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -291,7 +291,9 @@ func (c *credentialsProvider) getCredentials(ctx context.Context, image string) helper = strings.TrimPrefix(helper, "docker-credential-") err = setCredentialHelperToConfig(c.authFilePath, helper) if err != nil { - return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) + // TODO: gauron99 -- figure out what to do with this + fmt.Fprintf(os.Stderr, "Warning: failed to set the helper to the config with error: '%v'\n", err) + // return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) } err = setCredentialsByCredentialHelper(c.authFilePath, registry, result.Username, result.Password) if err != nil { diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 5581388772..47575731cd 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -478,11 +478,13 @@ func TestNewCredentialsSkipDockerConfigWhenNoHome(t *testing.T) { helper := newInMemoryHelper() setUpMockHelper("docker-credential-mock", helper)(t) - helper.Add(&credentials.Credentials{ + if err := helper.Add(&credentials.Credentials{ ServerURL: "docker.io", Username: dockerIoUser, Secret: dockerIoUserPwd, - }) + }); err != nil { + t.Error(err) + } // have docker config credential loader but HOME is not defined -- should return nil t.Setenv("HOME", "") diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 1d2dcd3508..0c4397e1cc 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -310,7 +310,7 @@ func TestBuildWithoutHome(t *testing.T) { } // TestDeployWithoutWritableDotConfig ensures that running client.New works without -// .config being accessable (write/read) +// .config being accessible (write/read) // TODO: change this test to for-loop of Runs with different dir permissions? func TestDeployWithoutWritableDotConfig(t *testing.T) { // defer Within(t, "tempdata/example.com/baddotconfig") From 67b35972c4225c63ab5a2ce29ea45151f12c73d5 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Mon, 8 Apr 2024 12:21:27 +0200 Subject: [PATCH 06/25] s2i test no home Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 34 ++++++++++++-------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 0c4397e1cc..54e43ab42e 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -9,7 +9,6 @@ import ( "io" "net/http" "os" - "path" "path/filepath" "reflect" "testing" @@ -309,35 +308,28 @@ func TestBuildWithoutHome(t *testing.T) { // dont call del() because function wasnt deployed } -// TestDeployWithoutWritableDotConfig ensures that running client.New works without -// .config being accessible (write/read) +// TestDeployWithoutHomeWithS2i ensures that running client.New works without +// home // TODO: change this test to for-loop of Runs with different dir permissions? -func TestDeployWithoutWritableDotConfig(t *testing.T) { - // defer Within(t, "tempdata/example.com/baddotconfig") +func TestDeployWithoutHomeWithS2i(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() - t.Setenv("HOME", root) + t.Setenv("HOME", "") verbose := true + name := "test-deploy-no-home" - // write new .config with no perms - err := os.Mkdir(path.Join(root, ".config"), 0000) - if err != nil { - t.Error(err) - } - - f := fn.Function{Runtime: "go", Name: "test-deploy-no-perms-config", Root: root} + f := fn.Function{Runtime: "node", Name: name, Root: root} - // client with pack builder - client := newClient(verbose) + // client with s2i builder + client := newClientWithS2i(verbose) - // expect to fail on build for HOME not being defined with pack builder - _, _, err = client.New(context.Background(), f) - // this error should be 'permission denied' for trying to write to open .config - if err == nil { - t.Fatalf("expected an error but got nil") + // expect to succeed + _, _, err := client.New(context.Background(), f) + if err != nil { + t.Fatalf("expected no errors but got %v", err) } - // dont call del() because function wasnt deployed + defer del(t, client, name) } // TestRemove ensures removal of a function instance. From ed51096eea2cd3c796676c269bc39f9c84699939 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Tue, 7 May 2024 09:51:00 +0200 Subject: [PATCH 07/25] remove unnecessary stuff Signed-off-by: gauron99 --- pkg/docker/creds/credentials.go | 33 ++++++++++++++----------- pkg/docker/creds/credentials_test.go | 37 ---------------------------- pkg/functions/client_int_test.go | 17 ------------- 3 files changed, 18 insertions(+), 69 deletions(-) diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index 0de7fee1e7..3c3747f6c2 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -189,20 +189,23 @@ func NewCredentialsProvider(configPath string, opts ...Opt) docker.CredentialsPr return getCredentialsByCredentialHelper(dockerConfigPath, registry) }) } - defaultCredentialLoaders = append(defaultCredentialLoaders, - func(registry string) (docker.Credentials, error) { - creds, err := dockerConfig.GetCredentials(sys, registry) - if err != nil { - return docker.Credentials{}, err - } - if creds.Username == "" || creds.Password == "" { - return docker.Credentials{}, ErrCredentialsNotFound - } - return docker.Credentials{ - Username: creds.Username, - Password: creds.Password, - }, nil - }) + // dont add this loader when configPath is empty + if configPath != "" { + defaultCredentialLoaders = append(defaultCredentialLoaders, + func(registry string) (docker.Credentials, error) { + creds, err := dockerConfig.GetCredentials(sys, registry) + if err != nil { + return docker.Credentials{}, err + } + if creds.Username == "" || creds.Password == "" { + return docker.Credentials{}, ErrCredentialsNotFound + } + return docker.Credentials{ + Username: creds.Username, + Password: creds.Password, + }, nil + }) + } defaultCredentialLoaders = append(defaultCredentialLoaders, func(registry string) (docker.Credentials, error) { // Fallback onto default docker config locations @@ -292,7 +295,7 @@ func (c *credentialsProvider) getCredentials(ctx context.Context, image string) err = setCredentialHelperToConfig(c.authFilePath, helper) if err != nil { // TODO: gauron99 -- figure out what to do with this - fmt.Fprintf(os.Stderr, "Warning: failed to set the helper to the config with error: '%v'\n", err) + fmt.Fprintf(os.Stderr, "Warning: failed to set helper to the config with error: '%v'\n", err) // return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) } err = setCredentialsByCredentialHelper(c.authFilePath, registry, result.Username, result.Password) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 47575731cd..7522bde3fd 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -465,43 +465,6 @@ func TestNewCredentialsProviderEmptyCreds(t *testing.T) { } } -// TestNewCredentialsSkipDockerConfigWhenNoHome ensures that docker .config -// credentials are NOT loaded when HOME is empty. -// (it would be at $HOME/.docker/config.json). Removing -// should result in returned loaded credentials instead. -func TestNewCredentialsSkipDockerConfigWhenNoHome(t *testing.T) { - resetHomeDir(t) - - // setup PATH, .docker/config.json (docker creds) and creds loader - withPopulatedDockerAuthConfig(t) - - helper := newInMemoryHelper() - setUpMockHelper("docker-credential-mock", helper)(t) - - if err := helper.Add(&credentials.Credentials{ - ServerURL: "docker.io", - Username: dockerIoUser, - Secret: dockerIoUserPwd, - }); err != nil { - t.Error(err) - } - - // have docker config credential loader but HOME is not defined -- should return nil - t.Setenv("HOME", "") - credentialsProvider := creds.NewCredentialsProvider(testConfigPath(t), - creds.WithVerifyCredentials(correctVerifyCbk), - ) - returnedCreds, err := credentialsProvider(context.Background(), "docker.io/someorg/someimage:sometag") - - // expect no credentials and error to be found here - if returnedCreds != (docker.Credentials{}) { - t.Errorf("expected no docker credentials to be found but got '%v'\n", returnedCreds) - } - if err != creds.ErrCredentialsNotFound { - t.Errorf("expected error to be '%v', but got '%v'", creds.ErrCredentialsNotFound, err) - } -} - func TestCredentialsProviderSavingFromUserInput(t *testing.T) { resetHomeDir(t) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 54e43ab42e..5a3a90d3ef 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -292,25 +292,8 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } } -// TestBuildWithoutHome ensures that running func without HOME fails for pack builder -func TestBuildWithoutHome(t *testing.T) { - root, cleanup := Mktemp(t) - defer cleanup() - t.Setenv("HOME", "") - verbose := true - - f := fn.Function{Runtime: "go", Name: "test-deploy-without-home", Root: root} - client := newClient(verbose) - if _, err := client.Build(context.Background(), f); err == nil { - t.Fatalf("expected an error for HOME not defined with pack builder, got none") - } - - // dont call del() because function wasnt deployed -} - // TestDeployWithoutHomeWithS2i ensures that running client.New works without // home -// TODO: change this test to for-loop of Runs with different dir permissions? func TestDeployWithoutHomeWithS2i(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() From d05ff29f683751efaec46e550f34e41f64cd5f92 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Fri, 10 May 2024 14:39:42 +0200 Subject: [PATCH 08/25] deploy test without home Signed-off-by: gauron99 --- pkg/builders/buildpacks/builder_test.go | 71 ------------------------- 1 file changed, 71 deletions(-) diff --git a/pkg/builders/buildpacks/builder_test.go b/pkg/builders/buildpacks/builder_test.go index 511b5635b9..7ed7acf9c7 100644 --- a/pkg/builders/buildpacks/builder_test.go +++ b/pkg/builders/buildpacks/builder_test.go @@ -2,15 +2,12 @@ package buildpacks import ( "context" - "io/fs" "os" "path/filepath" "reflect" - "strings" "testing" pack "github.com/buildpacks/pack/pkg/client" - "github.com/pkg/errors" "knative.dev/func/pkg/builders" fn "knative.dev/func/pkg/functions" ) @@ -219,74 +216,6 @@ func TestBuild_Errors(t *testing.T) { } } -// TestBuild_HomeAndPermissions ensures that function fails during build while HOME is -// not defined -// TODO: can add more options with permissions for testing -func TestBuild_HomeAndPermissions(t *testing.T) { - testCases := []struct { - name string - homePath bool - homePerms fs.FileMode - expectedErr bool - errContains string - }{ - // TODO: gauron99 -- maybe add test for PACK_HOME/ specific paths -- would require to change the impl - {name: "xpct-fail - create pack client when HOME not defined", homePath: false, homePerms: 0, expectedErr: true, errContains: "$HOME is not defined"}, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - tc := tc - var ( - i = &mockImpl{} - b = NewBuilder( // Func Builder logic - WithName(builders.Pack), - WithImpl(i)) - f = fn.Function{ - Runtime: "go", - Build: fn.BuildSpec{Image: "example.com/parent/name"}, - } - ) - // set temporary dir and assign it as func directory - tempdir := t.TempDir() - f.Root = tempdir - - // setup HOME env and its perms - if tc.homePath == false { - t.Setenv("HOME", "") - } else { - // setup new home with perms - err := os.MkdirAll(filepath.Join(tempdir, "home"), tc.homePerms) - if err != nil { - t.Fatal(err) - } - t.Setenv("HOME", filepath.Join(tempdir, "home")) - } - i.BuildFn = func(ctx context.Context, opts pack.BuildOptions) error { - _, err := os.UserHomeDir() - if err != nil { - return errors.Wrap(err, "getting user home") - } - return nil - } - - err := b.Build(context.Background(), f, nil) - // error scenarios - if tc.expectedErr { - if err == nil { - t.Fatalf("expected error but got nil") - } else if !strings.Contains(err.Error(), tc.errContains) { - t.Fatalf("expected error message '%v' was not found in the actual error: '%v'", tc.errContains, err) - } - } else { - if err != nil { - t.Fatalf("didnt expect an error but got '%v'", err) - } - } - }) - } -} - type mockImpl struct { BuildFn func(context.Context, pack.BuildOptions) error } From dcbaccfb8d00a3229888bb770f362cb380ce5207 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Tue, 21 May 2024 15:57:06 +0200 Subject: [PATCH 09/25] confict fix after rebase Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 5a3a90d3ef..19989af961 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -292,9 +292,9 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } } -// TestDeployWithoutHomeWithS2i ensures that running client.New works without +// TestDeployS2iBuilderWithoutHome ensures that running client.New works without // home -func TestDeployWithoutHomeWithS2i(t *testing.T) { +func TestDeployS2iBuilderWithoutHome(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() @@ -302,7 +302,7 @@ func TestDeployWithoutHomeWithS2i(t *testing.T) { verbose := true name := "test-deploy-no-home" - f := fn.Function{Runtime: "node", Name: name, Root: root} + f := fn.Function{Runtime: "node", Name: name, Root: root, Namespace: DefaultNamespace} // client with s2i builder client := newClientWithS2i(verbose) @@ -312,7 +312,7 @@ func TestDeployWithoutHomeWithS2i(t *testing.T) { if err != nil { t.Fatalf("expected no errors but got %v", err) } - defer del(t, client, name) + defer del(t, client, name, DefaultNamespace) } // TestRemove ensures removal of a function instance. From fcd72b4affadca37a472d1fcfc847d76ea8939c7 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 22 May 2024 16:30:02 +0200 Subject: [PATCH 10/25] move test, dont delete Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 19989af961..f04ad81b63 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -292,29 +292,6 @@ func TestUpdateWithAnnotationsAndLabels(t *testing.T) { } } -// TestDeployS2iBuilderWithoutHome ensures that running client.New works without -// home -func TestDeployS2iBuilderWithoutHome(t *testing.T) { - root, cleanup := Mktemp(t) - defer cleanup() - - t.Setenv("HOME", "") - verbose := true - name := "test-deploy-no-home" - - f := fn.Function{Runtime: "node", Name: name, Root: root, Namespace: DefaultNamespace} - - // client with s2i builder - client := newClientWithS2i(verbose) - - // expect to succeed - _, _, err := client.New(context.Background(), f) - if err != nil { - t.Fatalf("expected no errors but got %v", err) - } - defer del(t, client, name, DefaultNamespace) -} - // TestRemove ensures removal of a function instance. func TestRemove(t *testing.T) { defer Within(t, "testdata/example.com/remove")() @@ -588,7 +565,15 @@ func TestDeployWithoutHome(t *testing.T) { t.Fatalf("expected no errors but got %v", err) } +<<<<<<< HEAD defer del(t, client, name, DefaultNamespace) +======= + // NOTE: gauron99: this del is commented out until client.delete is resolved. + // Currently, the remover is looking for resources to delete and is taking about + // triple the time of current timeout. + // For remover/deleter resolution, see issue: https://github.com/knative/func/issues/2316 + // defer del(t, client, name, DefaultNamespace) +>>>>>>> 6f70b7d2 (move test, dont delete) } // *********** From bef1d4a20623106d2b23c08bbbecd7068667b063 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 23 May 2024 10:45:12 +0200 Subject: [PATCH 11/25] runtime change Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index f04ad81b63..0fc35ef5db 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -554,7 +554,7 @@ func TestDeployWithoutHome(t *testing.T) { verbose := false name := "test-deploy-no-home" - f := fn.Function{Runtime: "node", Name: name, Root: root, Namespace: DefaultNamespace} + f := fn.Function{Runtime: "python", Name: name, Root: root, Namespace: DefaultNamespace} // client with s2i builder because pack needs HOME client := newClientWithS2i(verbose) @@ -565,6 +565,7 @@ func TestDeployWithoutHome(t *testing.T) { t.Fatalf("expected no errors but got %v", err) } +<<<<<<< HEAD <<<<<<< HEAD defer del(t, client, name, DefaultNamespace) ======= @@ -574,6 +575,9 @@ func TestDeployWithoutHome(t *testing.T) { // For remover/deleter resolution, see issue: https://github.com/knative/func/issues/2316 // defer del(t, client, name, DefaultNamespace) >>>>>>> 6f70b7d2 (move test, dont delete) +======= + defer del(t, client, name, DefaultNamespace) +>>>>>>> edfd905c (runtime change) } // *********** From e4eefd09de5feb50734b759e188210a015f0c541 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 23 May 2024 16:53:30 +0200 Subject: [PATCH 12/25] node image signals fixed and smaller size for GH actions Signed-off-by: gauron99 --- pkg/functions/client_int_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 0fc35ef5db..6c32a6195c 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -554,7 +554,7 @@ func TestDeployWithoutHome(t *testing.T) { verbose := false name := "test-deploy-no-home" - f := fn.Function{Runtime: "python", Name: name, Root: root, Namespace: DefaultNamespace} + f := fn.Function{Runtime: "node", Name: name, Root: root, Namespace: DefaultNamespace} // client with s2i builder because pack needs HOME client := newClientWithS2i(verbose) From f97a20d84e3e78e54b278e9a3b50f2bd353bc020 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 30 May 2024 10:17:51 +0200 Subject: [PATCH 13/25] return err Signed-off-by: gauron99 --- pkg/docker/creds/credentials.go | 36 +++++++++++++++------------------ 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index 3c3747f6c2..8986e62001 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -189,23 +189,20 @@ func NewCredentialsProvider(configPath string, opts ...Opt) docker.CredentialsPr return getCredentialsByCredentialHelper(dockerConfigPath, registry) }) } - // dont add this loader when configPath is empty - if configPath != "" { - defaultCredentialLoaders = append(defaultCredentialLoaders, - func(registry string) (docker.Credentials, error) { - creds, err := dockerConfig.GetCredentials(sys, registry) - if err != nil { - return docker.Credentials{}, err - } - if creds.Username == "" || creds.Password == "" { - return docker.Credentials{}, ErrCredentialsNotFound - } - return docker.Credentials{ - Username: creds.Username, - Password: creds.Password, - }, nil - }) - } + defaultCredentialLoaders = append(defaultCredentialLoaders, + func(registry string) (docker.Credentials, error) { + creds, err := dockerConfig.GetCredentials(sys, registry) + if err != nil { + return docker.Credentials{}, err + } + if creds.Username == "" || creds.Password == "" { + return docker.Credentials{}, ErrCredentialsNotFound + } + return docker.Credentials{ + Username: creds.Username, + Password: creds.Password, + }, nil + }) defaultCredentialLoaders = append(defaultCredentialLoaders, func(registry string) (docker.Credentials, error) { // Fallback onto default docker config locations @@ -294,9 +291,8 @@ func (c *credentialsProvider) getCredentials(ctx context.Context, image string) helper = strings.TrimPrefix(helper, "docker-credential-") err = setCredentialHelperToConfig(c.authFilePath, helper) if err != nil { - // TODO: gauron99 -- figure out what to do with this - fmt.Fprintf(os.Stderr, "Warning: failed to set helper to the config with error: '%v'\n", err) - // return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) + // fmt.Fprintf(os.Stderr, "Warning: failed to set helper to the config with error: '%v'\n", err) + return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) } err = setCredentialsByCredentialHelper(c.authFilePath, registry, result.Username, result.Password) if err != nil { From 5c47202c888117068c11f23999a8996f490d2b29 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Mon, 3 Jun 2024 09:15:42 +0200 Subject: [PATCH 14/25] clean up comments Signed-off-by: gauron99 --- pkg/docker/creds/credentials.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index 8986e62001..9825ba87a5 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -291,7 +291,6 @@ func (c *credentialsProvider) getCredentials(ctx context.Context, image string) helper = strings.TrimPrefix(helper, "docker-credential-") err = setCredentialHelperToConfig(c.authFilePath, helper) if err != nil { - // fmt.Fprintf(os.Stderr, "Warning: failed to set helper to the config with error: '%v'\n", err) return docker.Credentials{}, fmt.Errorf("faild to set the helper to the config: %w", err) } err = setCredentialsByCredentialHelper(c.authFilePath, registry, result.Username, result.Password) From 2a4e1030d74bc659fa0e769ce84768ba8b630921 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Tue, 11 Jun 2024 19:08:46 +0200 Subject: [PATCH 15/25] creds and test Signed-off-by: gauron99 --- pkg/config/config.go | 6 +- pkg/docker/creds/credentials.go | 13 +- pkg/docker/creds/credentials_test.go | 202 ++++++++++++++++++++++++--- 3 files changed, 196 insertions(+), 25 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index e357b733d4..9ace1cddf5 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -81,8 +81,12 @@ func NewDefault() (cfg Global, err error) { cp := File() bb, err := os.ReadFile(cp) if err != nil { + // config file is not required if os.IsNotExist(err) { - err = nil // config file is not required + err = nil + // } else if os.IsPermission(err) { + // fmt.Printf("Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp) + // err = nil // HOME perms are insufficient for config file -> warning } return } diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index 9825ba87a5..afbfc40bc9 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -184,10 +184,15 @@ func NewCredentialsProvider(configPath string, opts ...Opt) docker.CredentialsPr home, err := os.UserHomeDir() if err == nil { dockerConfigPath := filepath.Join(home, ".docker", "config.json") - defaultCredentialLoaders = append(defaultCredentialLoaders, - func(registry string) (docker.Credentials, error) { - return getCredentialsByCredentialHelper(dockerConfigPath, registry) - }) + if info, err := os.Stat(dockerConfigPath); err == nil { + // Check if the docker config file is readable by the user. + if info.Mode().Perm()&(0400) != 0 { + defaultCredentialLoaders = append(defaultCredentialLoaders, + func(registry string) (docker.Credentials, error) { + return getCredentialsByCredentialHelper(dockerConfigPath, registry) + }) + } + } } defaultCredentialLoaders = append(defaultCredentialLoaders, func(registry string) (docker.Credentials, error) { diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 7522bde3fd..50971219d8 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "io" + "io/fs" "math/big" "net" "net/http" @@ -549,8 +550,9 @@ func TestCredentialsProviderSavingFromUserInput(t *testing.T) { } } -// TestCredentialsWithoutHome tests different scenarios when HOME is not set -func TestCredentialsWithoutHome(t *testing.T) { +// TestCredentialsHome ensures that credentials are used properly when HOME is +// not set +func TestCredentialsHome(t *testing.T) { type args struct { promptUser creds.CredentialsCallback verifyCredentials creds.VerifyCredentialsCallback @@ -587,32 +589,160 @@ func TestCredentialsWithoutHome(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // set up HOME + if tt.testHomePathEmpty { + os.Unsetenv("HOME") + } else { + os.Setenv("HOME", "/tmp") + } + + // ASSERT HERE + }) + } +} + +// TestCredentialsHomePermissions tests whether the credentials provider +// works in scenarios where HOME has different permissions +func TestCredentialsHomePermissions(t *testing.T) { + type args struct { + promptUser creds.CredentialsCallback + verifyCredentials creds.VerifyCredentialsCallback + registry string + setUpEnv setUpEnv + } + tests := []struct { + name string + perms os.FileMode + args args + expPermsDeniedErr bool + want Credentials + }{ + // testing different permissions + // { + // name: "home with 0000 permissions (no perms)", + // perms: 0000, + // args: args{ + // promptUser: pwdCbkThatShallNotBeCalled(t), + // setUpEnv: setHomeWithPermissions(0000), + // }, + // expPermsDeniedErr: true, + // }, + { + name: "home with 0333 permissions (write-execute only)", + perms: 0333, + args: args{ + + promptUser: pwdCbkThatShallNotBeCalled(t), + verifyCredentials: correctVerifyCbk, + registry: "docker.io", + setUpEnv: all( + withPopulatedDockerAuthConfig, + setUpMockHelper("docker-credential-mock", newInMemoryHelper()), + setHomeWithPermissions(0333)), + }, + + expPermsDeniedErr: false, + want: Credentials{Username: dockerIoUser, Password: dockerIoUserPwd}, + }, + { + name: "home with 0444 permissions (read-only)", + perms: 0444, + args: args{ + promptUser: pwdCbkThatShallNotBeCalled(t), + setUpEnv: setHomeWithPermissions(0444)}, + expPermsDeniedErr: true, + }, + { + name: "home with 0555 permissions (read-execute-only)", + perms: 0555, + args: args{ + promptUser: pwdCbkThatShallNotBeCalled(t), + setUpEnv: setHomeWithPermissions(0555), + }, + expPermsDeniedErr: true, + }, + { + name: "home with 0666 permissions (read-write-execute)", + perms: 0666, + args: args{ + promptUser: pwdCbkThatShallNotBeCalled(t), + setUpEnv: setHomeWithPermissions(0666), + }, + expPermsDeniedErr: true, + }, + { + name: "home with 0777 permissions (full access)", + perms: 0777, + + args: args{ + promptUser: pwdCbkThatShallNotBeCalled(t), + verifyCredentials: correctVerifyCbk, + registry: "docker.io", + setUpEnv: all( + withPopulatedDockerAuthConfig, + setUpMockHelper("docker-credential-mock", newInMemoryHelper()), + setHomeWithPermissions(0777), + ), + }, + expPermsDeniedErr: false, + want: Credentials{Username: dockerIoUser, Password: dockerIoUserPwd}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + resetHomePermissions(t) //reset home permissions to 0777 (fs.ModePerm) resetHomeDir(t) + if tt.args.setUpEnv != nil { tt.args.setUpEnv(t) } - // prepare config path for credentials provider - var configPath string - if tt.testHomePathEmpty { - configPath = "" - } else { - configPath = testConfigPath(t) + // try to create HOME/.config/func + _, err := testConfigPathError(t) + if err != nil { // If error was returned + if os.IsPermission(err) { // and its a permission error + if !tt.expPermsDeniedErr { // but it wasnt expected + t.Fatalf("didnt expect permissions denied error, but got: %s", err) + } + + } else { // and it wasnt permission error + t.Fatalf("got unexpected error: %v", err) + } + } else { // Else no error was returned + if tt.expPermsDeniedErr { // but it was expected + t.Fatal("expected permissions denied error, but got none") + } } - credentialsProvider := creds.NewCredentialsProvider( - configPath, - creds.WithPromptForCredentials(tt.args.promptUser), - creds.WithVerifyCredentials(tt.args.verifyCredentials), - ) + // if permissions were not denied, try to create Provider + if !tt.expPermsDeniedErr { + + // try to stat HOME for permissions + info, err := os.Stat(os.Getenv(testHomeEnvName())) + if err != nil { + t.Fatalf("failed to stat HOME: %s", err) + } + + if info.Mode().Perm() != tt.perms { + t.Errorf("expected permissions '%v', got '%v'", tt.perms, info.Mode().Perm()) + } + credentialsProvider := creds.NewCredentialsProvider( + testConfigPath(t), + creds.WithPromptForCredentials(tt.args.promptUser), + creds.WithVerifyCredentials(tt.args.verifyCredentials), + ) + + got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag") + if err != nil { + t.Errorf("%v", err) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("got: %v, want: %v", got, tt.want) + } - got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag") - if err != nil { - t.Errorf("unexpected error: %v", err) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("got: %v, want: %v", got, tt.want) } }) } @@ -630,6 +760,12 @@ func resetHomeDir(t *testing.T) { } } +func resetHomePermissions(t *testing.T) { + if err := os.Chmod(homeTempDir, fs.ModePerm); err != nil { + t.Fatal(err) + } +} + type setUpEnv = func(t *testing.T) func withPopulatedDockerAuthConfig(t *testing.T) { @@ -750,6 +886,13 @@ func testConfigPath(t *testing.T) string { return configPath } +func testConfigPathError(t *testing.T) (string, error) { + t.Helper() + home := os.Getenv(testHomeEnvName()) + configPath := filepath.Join(home, ".config", "func") + return configPath, os.MkdirAll(configPath, os.ModePerm) +} + func handlerForCredHelper(t *testing.T, credHelper credentials.Helper) http.Handler { return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { defer request.Body.Close() @@ -981,3 +1124,22 @@ func setEmptyHome(t *testing.T) { t.Setenv("HOME", "") t.Setenv("XDG_CONFIG_HOME", "") } + +// setHomeWithPermissions sets home dir to specified permissions and returns them +// to 755 perms after as a cleanup +func setHomeWithPermissions(perm os.FileMode) func(t *testing.T) { + return func(t *testing.T) { + t.Helper() + homeDir := os.Getenv("HOME") + + // if home is empty, nothing to do + if homeDir == "" { + t.Fatal("home dir is empty, cant set perms") + } + + err := os.Chmod(homeDir, perm) + if err != nil { + t.Fatalf("failed to set permissions on home dir: %s", err) + } + } +} From 339d8f7f3bb588298b45453416f44568b5d07d38 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Tue, 11 Jun 2024 19:15:24 +0200 Subject: [PATCH 16/25] test return commented code Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 41 ++++++++++++++++++---------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 50971219d8..264cbe2558 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -550,9 +550,9 @@ func TestCredentialsProviderSavingFromUserInput(t *testing.T) { } } -// TestCredentialsHome ensures that credentials are used properly when HOME is -// not set -func TestCredentialsHome(t *testing.T) { +// TestCredentialsWithoutHome ensures that credentials are used properly when HOME is +// not set or config is empty +func TestCredentialsWithoutHome(t *testing.T) { type args struct { promptUser creds.CredentialsCallback verifyCredentials creds.VerifyCredentialsCallback @@ -596,7 +596,21 @@ func TestCredentialsHome(t *testing.T) { os.Setenv("HOME", "/tmp") } - // ASSERT HERE + credentialsProvider := creds.NewCredentialsProvider( + testConfigPath(t), + creds.WithPromptForCredentials(tt.args.promptUser), + creds.WithVerifyCredentials(tt.args.verifyCredentials), + ) + + got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag") + if err != nil { + t.Errorf("%v", err) + return + } + // ASSERT + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("got: %v, want: %v", got, tt.want) + } }) } } @@ -617,16 +631,15 @@ func TestCredentialsHomePermissions(t *testing.T) { expPermsDeniedErr bool want Credentials }{ - // testing different permissions - // { - // name: "home with 0000 permissions (no perms)", - // perms: 0000, - // args: args{ - // promptUser: pwdCbkThatShallNotBeCalled(t), - // setUpEnv: setHomeWithPermissions(0000), - // }, - // expPermsDeniedErr: true, - // }, + { + name: "home with 0000 permissions (no perms)", + perms: 0000, + args: args{ + promptUser: pwdCbkThatShallNotBeCalled(t), + setUpEnv: setHomeWithPermissions(0000), + }, + expPermsDeniedErr: true, + }, { name: "home with 0333 permissions (write-execute only)", perms: 0333, From 27b810a58176011f36a2202a85dfb138503f5dba Mon Sep 17 00:00:00 2001 From: gauron99 Date: Wed, 12 Jun 2024 13:17:49 +0200 Subject: [PATCH 17/25] config warning Signed-off-by: gauron99 --- pkg/config/config.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 9ace1cddf5..a0067fbb1c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -84,9 +84,11 @@ func NewDefault() (cfg Global, err error) { // config file is not required if os.IsNotExist(err) { err = nil - // } else if os.IsPermission(err) { - // fmt.Printf("Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp) - // err = nil // HOME perms are insufficient for config file -> warning + } else if os.IsPermission(err) { + err = nil // insufficient perms for config file -> warning + // TODO: gauron99 - this gets printed out for lot of commands so its a bit + // spammy, perhaps dont print here? + fmt.Fprintf(os.Stderr, "Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp) } return } From 4f3c2d67c46febf5e123331c9c6d2cc8dbf4169c Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 13 Jun 2024 09:18:00 +0200 Subject: [PATCH 18/25] cleanup Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 19 +++++++++++++++---- pkg/functions/client_int_test.go | 12 ------------ 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 264cbe2558..32b389fda5 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -15,7 +15,6 @@ import ( "errors" "fmt" "io" - "io/fs" "math/big" "net" "net/http" @@ -587,13 +586,19 @@ func TestCredentialsWithoutHome(t *testing.T) { }, } + // reset HOME to the original value after tests since they may change it + defer func() { + os.Setenv("HOME", homeTempDir) + }() + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + resetHomeDir(t) // set up HOME if tt.testHomePathEmpty { os.Unsetenv("HOME") } else { - os.Setenv("HOME", "/tmp") + os.Setenv("HOME", homeTempDir) } credentialsProvider := creds.NewCredentialsProvider( @@ -702,10 +707,15 @@ func TestCredentialsHomePermissions(t *testing.T) { }, } + // return HOME dir into its original state + defer func() { + resetHomePermissions(t) //reset home permissions to 0700 + }() + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - resetHomePermissions(t) //reset home permissions to 0777 (fs.ModePerm) + resetHomePermissions(t) //needs to be reset so that dir can be removed resetHomeDir(t) if tt.args.setUpEnv != nil { @@ -773,8 +783,9 @@ func resetHomeDir(t *testing.T) { } } +// resetHomePermissions resets the HOME perms to 0700 (same as resetHomeDir(t)) func resetHomePermissions(t *testing.T) { - if err := os.Chmod(homeTempDir, fs.ModePerm); err != nil { + if err := os.Chmod(homeTempDir, 0700); err != nil { t.Fatal(err) } } diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 6c32a6195c..47ade0755d 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -565,19 +565,7 @@ func TestDeployWithoutHome(t *testing.T) { t.Fatalf("expected no errors but got %v", err) } -<<<<<<< HEAD -<<<<<<< HEAD defer del(t, client, name, DefaultNamespace) -======= - // NOTE: gauron99: this del is commented out until client.delete is resolved. - // Currently, the remover is looking for resources to delete and is taking about - // triple the time of current timeout. - // For remover/deleter resolution, see issue: https://github.com/knative/func/issues/2316 - // defer del(t, client, name, DefaultNamespace) ->>>>>>> 6f70b7d2 (move test, dont delete) -======= - defer del(t, client, name, DefaultNamespace) ->>>>>>> edfd905c (runtime change) } // *********** From a7c99339486fed6696324e8e9551dd75b265b410 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 13 Jun 2024 11:29:15 +0200 Subject: [PATCH 19/25] cleanup Signed-off-by: gauron99 --- pkg/docker/creds/credentials.go | 13 ++++--------- pkg/docker/creds/credentials_test.go | 13 ++++++++----- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/pkg/docker/creds/credentials.go b/pkg/docker/creds/credentials.go index afbfc40bc9..9825ba87a5 100644 --- a/pkg/docker/creds/credentials.go +++ b/pkg/docker/creds/credentials.go @@ -184,15 +184,10 @@ func NewCredentialsProvider(configPath string, opts ...Opt) docker.CredentialsPr home, err := os.UserHomeDir() if err == nil { dockerConfigPath := filepath.Join(home, ".docker", "config.json") - if info, err := os.Stat(dockerConfigPath); err == nil { - // Check if the docker config file is readable by the user. - if info.Mode().Perm()&(0400) != 0 { - defaultCredentialLoaders = append(defaultCredentialLoaders, - func(registry string) (docker.Credentials, error) { - return getCredentialsByCredentialHelper(dockerConfigPath, registry) - }) - } - } + defaultCredentialLoaders = append(defaultCredentialLoaders, + func(registry string) (docker.Credentials, error) { + return getCredentialsByCredentialHelper(dockerConfigPath, registry) + }) } defaultCredentialLoaders = append(defaultCredentialLoaders, func(registry string) (docker.Credentials, error) { diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 32b389fda5..ad64b514c8 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -549,7 +549,7 @@ func TestCredentialsProviderSavingFromUserInput(t *testing.T) { } } -// TestCredentialsWithoutHome ensures that credentials are used properly when HOME is +// TestCredentialsWithoutHome ensures that credentialProvider works when HOME is // not set or config is empty func TestCredentialsWithoutHome(t *testing.T) { type args struct { @@ -600,7 +600,7 @@ func TestCredentialsWithoutHome(t *testing.T) { } else { os.Setenv("HOME", homeTempDir) } - + fmt.Printf("testconfigpath is: %v\n", testConfigPath(t)) credentialsProvider := creds.NewCredentialsProvider( testConfigPath(t), creds.WithPromptForCredentials(tt.args.promptUser), @@ -903,9 +903,12 @@ func testHomeEnvName() string { func testConfigPath(t *testing.T) string { t.Helper() home := os.Getenv(testHomeEnvName()) - configPath := filepath.Join(home, ".config", "func") - if err := os.MkdirAll(configPath, os.ModePerm); err != nil { - t.Fatal(err) + var configPath string + if home != "" { // if HOME is not set, don't create config dir + configPath = filepath.Join(home, ".config", "func") + if err := os.MkdirAll(configPath, os.ModePerm); err != nil { + t.Fatal(err) + } } return configPath } From e851eef7833f6ca70db4332a6ab7138e76112bde Mon Sep 17 00:00:00 2001 From: gauron99 Date: Thu, 13 Jun 2024 11:31:01 +0200 Subject: [PATCH 20/25] fix Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index ad64b514c8..f0b62a1605 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -600,7 +600,6 @@ func TestCredentialsWithoutHome(t *testing.T) { } else { os.Setenv("HOME", homeTempDir) } - fmt.Printf("testconfigpath is: %v\n", testConfigPath(t)) credentialsProvider := creds.NewCredentialsProvider( testConfigPath(t), creds.WithPromptForCredentials(tt.args.promptUser), From b09cdc0c920324dca941d2d3609433ab5d2d4309 Mon Sep 17 00:00:00 2001 From: gauron99 Date: Fri, 14 Jun 2024 08:19:51 +0200 Subject: [PATCH 21/25] fix Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index f0b62a1605..6cdaf15cf5 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -607,11 +607,12 @@ func TestCredentialsWithoutHome(t *testing.T) { ) got, err := credentialsProvider(context.Background(), tt.args.registry+"/someorg/someimage:sometag") + + // ASSERT if err != nil { t.Errorf("%v", err) return } - // ASSERT if !reflect.DeepEqual(got, tt.want) { t.Errorf("got: %v, want: %v", got, tt.want) } @@ -912,6 +913,8 @@ func testConfigPath(t *testing.T) string { return configPath } +// testConfigPathError tries to create a config dir in HOME/.config/func. +// Compared to testConfigPath, this returns the path AND error instead of failing func testConfigPathError(t *testing.T) (string, error) { t.Helper() home := os.Getenv(testHomeEnvName()) @@ -1151,8 +1154,7 @@ func setEmptyHome(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", "") } -// setHomeWithPermissions sets home dir to specified permissions and returns them -// to 755 perms after as a cleanup +// setHomeWithPermissions sets home dir to specified permissions func setHomeWithPermissions(perm os.FileMode) func(t *testing.T) { return func(t *testing.T) { t.Helper() @@ -1163,6 +1165,7 @@ func setHomeWithPermissions(perm os.FileMode) func(t *testing.T) { t.Fatal("home dir is empty, cant set perms") } + fmt.Printf("setting permissions (%v) on home dir: %s\n", perm, homeDir) err := os.Chmod(homeDir, perm) if err != nil { t.Fatalf("failed to set permissions on home dir: %s", err) From 3bd502a8613c7c8e4d5fba4a213e08d940047bcd Mon Sep 17 00:00:00 2001 From: gauron99 Date: Mon, 12 Aug 2024 11:18:06 +0200 Subject: [PATCH 22/25] skip test for windows Signed-off-by: gauron99 --- pkg/docker/creds/credentials_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 6cdaf15cf5..9edc53c6e1 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -623,6 +623,9 @@ func TestCredentialsWithoutHome(t *testing.T) { // TestCredentialsHomePermissions tests whether the credentials provider // works in scenarios where HOME has different permissions func TestCredentialsHomePermissions(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("skip windows perms for this test until windows perms are added") // TODO: gauron99 fix this + } type args struct { promptUser creds.CredentialsCallback verifyCredentials creds.VerifyCredentialsCallback From 32088dddbb5b0b1687ea110b4a8d95b5bc38323b Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Wed, 28 Aug 2024 19:57:32 +0200 Subject: [PATCH 23/25] skip for prow Signed-off-by: David Fridrich --- pkg/docker/creds/credentials_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/docker/creds/credentials_test.go b/pkg/docker/creds/credentials_test.go index 9edc53c6e1..c1227d152e 100644 --- a/pkg/docker/creds/credentials_test.go +++ b/pkg/docker/creds/credentials_test.go @@ -624,8 +624,14 @@ func TestCredentialsWithoutHome(t *testing.T) { // works in scenarios where HOME has different permissions func TestCredentialsHomePermissions(t *testing.T) { if runtime.GOOS == "windows" { - t.Skip("skip windows perms for this test until windows perms are added") // TODO: gauron99 fix this + t.Skip("skip windows perms for this test until windows perms are added") } + + if os.Getenv("GITHUB_ACTION") == "" { + // skip for prow because its running as root + t.Skip() + } + type args struct { promptUser creds.CredentialsCallback verifyCredentials creds.VerifyCredentialsCallback From 6f37e0cb4b8835d9e81f071c6412f53befd72749 Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Fri, 6 Sep 2024 14:21:12 +0200 Subject: [PATCH 24/25] fix repo on create, move warning up a function Signed-off-by: David Fridrich --- cmd/root.go | 5 +++++ pkg/config/config.go | 9 +++------ pkg/functions/repositories.go | 6 +++--- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 254104df30..6ad99c8000 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -64,6 +64,11 @@ Learn more about Knative at: https://knative.dev`, cfg.Name), viper.SetEnvPrefix("func") // ensure that all have the prefix viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + // check if permissions for FUNC HOME are sufficient; warn if otherwise + cp := config.File() + if _, err := os.ReadFile(cp); os.IsPermission(err) { + fmt.Fprintf(os.Stderr, "Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp) + } // Client // Use the provided ClientFactory or default to NewClient newClient := cfg.NewClient diff --git a/pkg/config/config.go b/pkg/config/config.go index a0067fbb1c..f5f5312f6a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -82,13 +82,10 @@ func NewDefault() (cfg Global, err error) { bb, err := os.ReadFile(cp) if err != nil { // config file is not required - if os.IsNotExist(err) { + // permissions warning printed in cmd/root.go as to not spam here + // TODO: gauron99 - review the whole process for simplification + if os.IsNotExist(err) || os.IsPermission(err) { err = nil - } else if os.IsPermission(err) { - err = nil // insufficient perms for config file -> warning - // TODO: gauron99 - this gets printed out for lot of commands so its a bit - // spammy, perhaps dont print here? - fmt.Fprintf(os.Stderr, "Warning: Insufficient permissions to read config file at '%s' - continuing without it\n", cp) } return } diff --git a/pkg/functions/repositories.go b/pkg/functions/repositories.go index 3288a84dfd..e859b0b3ed 100644 --- a/pkg/functions/repositories.go +++ b/pkg/functions/repositories.go @@ -100,9 +100,9 @@ func (r *Repositories) All() (repos []Repository, err error) { if r.path == "" { return } - - // Return empty if path does not exist - if _, err = os.Stat(r.path); os.IsNotExist(err) { + fmt.Printf("repos: %v\n\n", repos) + // Return empty if path does not exist or insufficient permissions + if _, err = os.Stat(r.path); os.IsNotExist(err) || os.IsPermission(err) { return repos, nil } From 1fbb2a88d33afb38ec27c8362132699a735333ee Mon Sep 17 00:00:00 2001 From: David Fridrich Date: Fri, 6 Sep 2024 14:29:58 +0200 Subject: [PATCH 25/25] fix print Signed-off-by: David Fridrich --- pkg/functions/repositories.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/functions/repositories.go b/pkg/functions/repositories.go index e859b0b3ed..a48038f341 100644 --- a/pkg/functions/repositories.go +++ b/pkg/functions/repositories.go @@ -100,7 +100,6 @@ func (r *Repositories) All() (repos []Repository, err error) { if r.path == "" { return } - fmt.Printf("repos: %v\n\n", repos) // Return empty if path does not exist or insufficient permissions if _, err = os.Stat(r.path); os.IsNotExist(err) || os.IsPermission(err) { return repos, nil