Skip to content

Commit

Permalink
fix: namespace isolation
Browse files Browse the repository at this point in the history
  • Loading branch information
lkingland committed Mar 6, 2024
1 parent 2cc0396 commit c3917e5
Show file tree
Hide file tree
Showing 37 changed files with 720 additions and 731 deletions.
15 changes: 10 additions & 5 deletions cmd/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,15 @@ import (
// These are the minimum settings necessary to create the default client
// instance which has most subsystems initialized.
type ClientConfig struct {
// Namespace in the remote cluster to use for any client commands which
// PipelinesNamespace in the remote cluster to use for any client commands which
// touch the remote. Optional. Empty namespace indicates the namespace
// currently configured in the client's connection should be used.
Namespace string
//
// DEPRECATED: This is being removed. Individual commands should use
// either a supplied --namespace flag, the current active k8s context,
// the global default (if defined) or the static default "default", in
// that order.
PipelinesNamespace string

// Verbose logging. By default, logging output is kept to the bare minimum.
// Use this flag to configure verbose logging throughout.
Expand Down Expand Up @@ -63,15 +68,15 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) {
t = newTransport(cfg.InsecureSkipVerify) // may provide a custom impl which proxies
c = newCredentialsProvider(config.Dir(), t) // for accessing registries
d = newKnativeDeployer(cfg.Verbose)
pp = newTektonPipelinesProvider(cfg.Namespace, c, cfg.Verbose)
pp = newTektonPipelinesProvider(cfg.PipelinesNamespace, c, cfg.Verbose)
o = []fn.Option{ // standard (shared) options for all commands
fn.WithVerbose(cfg.Verbose),
fn.WithTransport(t),
fn.WithRepositoriesPath(config.RepositoriesPath()),
fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))),
fn.WithRemover(knative.NewRemover(cfg.Verbose)),
fn.WithDescriber(knative.NewDescriber(cfg.Namespace, cfg.Verbose)),
fn.WithLister(knative.NewLister(cfg.Namespace, cfg.Verbose)),
fn.WithDescriber(knative.NewDescriber(cfg.Verbose)),
fn.WithLister(knative.NewLister(cfg.Verbose)),
fn.WithDeployer(d),
fn.WithPipelinesProvider(pp),
fn.WithPusher(docker.NewPusher(
Expand Down
24 changes: 14 additions & 10 deletions cmd/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,34 @@ func Test_NewTestClient(t *testing.T) {

// Factory constructor options which should be used when invoking later
clientFn := NewTestClient(fn.WithRemover(remover))

// Factory should ignore options provided when invoking
client, _ := clientFn(ClientConfig{}, fn.WithDescriber(describer))

// Trigger an invocation of the mocks
err := client.Remove(context.Background(), fn.Function{Name: "test", Deploy: fn.DeploySpec{Namespace: namespace}}, true)
// Trigger an invocation of the mocks by running the associated client
// methods which depend on them
err := client.Remove(context.Background(), "myfunc", "myns", fn.Function{}, true)
if err != nil {
t.Fatal(err)
}
f, err := fn.NewFunction("")
if err != nil {
t.Fatal(err)
}
_, err = client.Describe(context.Background(), "test", f)
_, err = client.Describe(context.Background(), "myfunc", "myns", fn.Function{})
if err != nil {
t.Fatal(err)
}

// Ensure the first set of options, held on the factory, were used
// Ensure the first set of options, held on the factory (the mock remover)
// is invoked.
if !remover.RemoveInvoked {
t.Fatalf("factory (outer) options not carried through to final client instance")
}
// Ensure the second set of options, provided when constructing the
// client using the factory, were ignored
// Ensure the second set of options, provided when constructing the client
// using the factory, are ignored.
if describer.DescribeInvoked {
t.Fatalf("test client factory should ignore options when invoked.")
}

// This ensures that the NewTestClient function, when provided a set
// of optional implementations (mocks) will override any which are set
// by commands, allowing tests to "force" a command to use the mocked
// implementations.
}
4 changes: 2 additions & 2 deletions cmd/completion_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import (
)

func CompleteFunctionList(cmd *cobra.Command, args []string, toComplete string) (strings []string, directive cobra.ShellCompDirective) {
lister := knative.NewLister("", false)
lister := knative.NewLister(false)

list, err := lister.List(cmd.Context())
list, err := lister.List(cmd.Context(), "")
if err != nil {
directive = cobra.ShellCompDirectiveError
return
Expand Down
6 changes: 3 additions & 3 deletions cmd/config_git_remove.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ type configGitRemoveConfig struct {
// working directory of the process.
Path string

Namespace string
PipelinesNamespace string

// informs whether any specific flag for deleting only a subset of resources has been set
flagSet bool
Expand All @@ -93,7 +93,7 @@ func newConfigGitRemoveConfig(cmd *cobra.Command) (c configGitRemoveConfig) {
}

c = configGitRemoveConfig{
Namespace: viper.GetString("namespace"),
PipelinesNamespace: viper.GetString("namespace"),

flagSet: flagSet,

Expand Down Expand Up @@ -181,7 +181,7 @@ func runConfigGitRemoveCmd(cmd *cobra.Command, newClient ClientFactory) (err err
return
}

client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose})
client, done := newClient(ClientConfig{PipelinesNamespace: cfg.PipelinesNamespace, Verbose: cfg.Verbose})
defer done()

return client.RemovePAC(cmd.Context(), f, cfg.metadata)
Expand Down
8 changes: 4 additions & 4 deletions cmd/config_git_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func NewConfigGitSetCmd(newClient ClientFactory) *cobra.Command {
type configGitSetConfig struct {
buildConfig // further embeds config.Global

Namespace string
PipelinesNamespace string

GitProvider string
GitURL string
Expand Down Expand Up @@ -126,8 +126,8 @@ func newConfigGitSetConfig(cmd *cobra.Command) (c configGitSetConfig) {
}

c = configGitSetConfig{
buildConfig: newBuildConfig(),
Namespace: viper.GetString("namespace"),
buildConfig: newBuildConfig(),
PipelinesNamespace: viper.GetString("namespace"),

GitURL: viper.GetString("git-url"),
GitRevision: viper.GetString("git-branch"),
Expand Down Expand Up @@ -307,7 +307,7 @@ func runConfigGitSetCmd(cmd *cobra.Command, newClient ClientFactory) (err error)
return
}

client, done := newClient(ClientConfig{Namespace: cfg.Namespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
client, done := newClient(ClientConfig{PipelinesNamespace: cfg.PipelinesNamespace, Verbose: cfg.Verbose}, fn.WithRegistry(cfg.Registry))
defer done()

return client.ConfigurePAC(cmd.Context(), f, cfg.metadata)
Expand Down
84 changes: 35 additions & 49 deletions cmd/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ No local files are deleted.
{{rootCmdUse}} delete
# Undeploy the function 'myfunc' in namespace 'apps'
{{rootCmdUse}} delete -n apps myfunc
{{rootCmdUse}} delete myfunc --namespace apps
`,
SuggestFor: []string{"remove", "del"},
Aliases: []string{"rm"},
Expand All @@ -47,7 +47,7 @@ No local files are deleted.
}

// Flags
cmd.Flags().StringP("namespace", "n", cfg.Namespace, "The namespace in which to delete. ($FUNC_NAMESPACE)")
cmd.Flags().StringP("namespace", "n", defaultNamespace(fn.Function{}, false), "The namespace when deleting by name. ($FUNC_NAMESPACE)")
cmd.Flags().StringP("all", "a", "true", "Delete all resources created for a function, eg. Pipelines, Secrets, etc. ($FUNC_ALL) (allowed values: \"true\", \"false\")")
addConfirmFlag(cmd, cfg.Confirm)
addPathFlag(cmd)
Expand All @@ -57,77 +57,63 @@ No local files are deleted.
}

func runDelete(cmd *cobra.Command, args []string, newClient ClientFactory) (err error) {
cfg, err := newDeleteConfig(args).Prompt()
cfg, err := newDeleteConfig(cmd, args)
if err != nil {
return
}

// check that name is defined when deleting a Function in specific namespace
if cfg.Name == "" && cfg.Namespace != "" {
return fmt.Errorf("function name is required when namespace is specified")
if cfg, err = cfg.Prompt(); err != nil {
return
}

var function fn.Function
// initialize namespace from the config
var namespace = cfg.Namespace
client, done := newClient(ClientConfig{Verbose: cfg.Verbose})
defer done()

// Initialize func with explicit name (when provided)
if len(args) > 0 && args[0] != "" {
pathChanged := cmd.Flags().Changed("path")
if pathChanged {
return fmt.Errorf("only one of --path and [NAME] should be provided")
}
function = fn.Function{
Name: args[0],
}
} else {
function, err = fn.NewFunction(cfg.Path)
if cfg.Name != "" { // Delete by name if provided
return client.Remove(cmd.Context(), cfg.Name, cfg.Namespace, fn.Function{}, cfg.All)
} else { // Onterwise delete the function at path (cwd by default)
f, err := fn.NewFunction(cfg.Path)
if err != nil {
return
}

// Check if the function has been initialized
if !function.Initialized() {
return fn.NewErrNotInitialized(function.Root)
}

// use the function's extant namespace -- already deployed function
if !cmd.Flags().Changed("namespace") && function.Deploy.Namespace != "" {
namespace = function.Deploy.Namespace
return err
}
return client.Remove(cmd.Context(), "", "", f, cfg.All)
}

// Create a client instance from the now-final config
client, done := newClient(ClientConfig{Namespace: namespace, Verbose: cfg.Verbose})
defer done()

function.Deploy.Namespace = namespace
// Invoke remove using the concrete client impl
return client.Remove(cmd.Context(), function, cfg.DeleteAll)
}

type deleteConfig struct {
Name string
Namespace string
Path string
DeleteAll bool
All bool
Verbose bool
}

// newDeleteConfig returns a config populated from the current execution context
// (args, flags and environment variables)
func newDeleteConfig(args []string) deleteConfig {
func newDeleteConfig(cmd *cobra.Command, args []string) (cfg deleteConfig, err error) {
var name string
if len(args) > 0 {
name = args[0]
}
return deleteConfig{
Path: viper.GetString("path"),
cfg = deleteConfig{
All: viper.GetBool("all"),
Name: name, // args[0] or derived
Namespace: viper.GetString("namespace"),
DeleteAll: viper.GetBool("all"),
Name: deriveName(name, viper.GetString("path")), // args[0] or derived
Verbose: viper.GetBool("verbose"), // defined on root
Path: viper.GetString("path"),
Verbose: viper.GetBool("verbose"), // defined on root
}
if cfg.Name == "" && cmd.Flags().Changed("namespace") {
// logicially inconsistent to supply only a namespace.
// Either use the function's local state in its entirety, or specify
// both a name and a namespace to ignore any local function source.
err = fmt.Errorf("must also specify a name when specifying namespace.")
}
if cfg.Name != "" && cmd.Flags().Changed("path") {
// logically inconsistent to provide both a name and a path to source.
// Either use the function's local state on disk (--path), or specify
// a name and a namespace to ignore any local function source.
err = fmt.Errorf("only one of --path and [NAME] should be provided")
}
return
}

// Prompt the user with value of config members, allowing for interaractive changes.
Expand All @@ -151,7 +137,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) {
Name: "all",
Prompt: &survey.Confirm{
Message: "Do you want to delete all resources?",
Default: c.DeleteAll,
Default: c.All,
},
},
}
Expand All @@ -166,7 +152,7 @@ func (c deleteConfig) Prompt() (deleteConfig, error) {
}

dc.Name = answers.Name
dc.DeleteAll = answers.All
dc.All = answers.All

return dc, err
}
37 changes: 22 additions & 15 deletions cmd/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,40 +14,48 @@ import (
// with default options
func TestDelete_Default(t *testing.T) {
var (
err error
root = FromTempDirectory(t)
namespace = "myns"
name = "testdelete"
namespace = "testdeletedamespace"
remover = mock.NewRemover()
err error
ctx = context.Background()
)

remover.RemoveFn = func(_, ns string) error {
remover.RemoveFn = func(n, ns string) error {
if name != name {

Check failure on line 26 in cmd/delete_test.go

View workflow job for this annotation

GitHub Actions / Check Source (ubuntu-latest)

SA4000: identical expressions on the left and right side of the '!=' operator (staticcheck)

Check failure on line 26 in cmd/delete_test.go

View workflow job for this annotation

GitHub Actions / style / Golang / Lint

SA4000: identical expressions on the left and right side of the '!=' operator (staticcheck)
t.Fatalf("expected name '%v', got '%v'", name, n)
}
if ns != namespace {
t.Fatalf("expected delete namespace '%v', got '%v'", namespace, ns)
t.Fatalf("expected namespace '%v', got '%v'", namespace, ns)
}
return nil
}

client := fn.New(
fn.WithBuilder(mock.NewBuilder()),
fn.WithDeployer(mock.NewDeployer()),
fn.WithRemover(mock.NewRemover()),
)

// Ensure the extant function's namespace is used
f := fn.Function{
Root: root,
Runtime: "go",
Registry: TestRegistry,
Name: "testname",
Deploy: fn.DeploySpec{
Namespace: namespace, //simulate deployed Function
},
Runtime: "go",
Name: name,
Namespace: namespace,
Root: root,
Registry: TestRegistry,
}

if f, err = fn.New().Init(f); err != nil {
if _, f, err = client.New(ctx, f); err != nil {
t.Fatal(err)
}

if err = f.Write(); err != nil {
t.Fatal(err)
}

cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
cmd.SetArgs([]string{}) //dont give any arguments to 'func delete' -- default
cmd.SetArgs([]string{})
if err := cmd.Execute(); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -99,7 +107,6 @@ func TestDelete_ByName(t *testing.T) {
// with a mocked remover.
cmd := NewDeleteCmd(NewTestClient(fn.WithRemover(remover)))
cmd.SetArgs([]string{testname}) // run: func delete <name>

if err := cmd.Execute(); err != nil {
t.Fatal(err)
}
Expand Down
Loading

0 comments on commit c3917e5

Please sign in to comment.