fix empty client in action config

[ankitathomas]

Fixing the panic because of the empty client in action config

$ kubectl operator olmv1 create catalog operatorhub quay.io/operatorhubio/catalog:latest
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x184c59c]

goroutine 1 [running]:
github.com/operator-framework/kubectl-operator/internal/pkg/v1/action.(*CatalogCreate).Run(0xc0001126c0, {0x1f6f468, 0xc0002b19d0})
	kubectl-operator/internal/pkg/v1/action/catalog_create.go:41 +0x1dc
github.com/operator-framework/kubectl-operator/internal/cmd/internal/olmv1.NewCatalogCreateCmd.func1(0xc000374c08?, {0xc00030ed60?, 0x4?, 0x1c59745?})
	kubectl-operator/internal/cmd/internal/olmv1/catalog_create.go:28 +0xaf
github.com/spf13/cobra.(*Command).execute(0xc000374c08, {0xc00030ed20, 0x2, 0x2})
	/home/lmohanty/go/pkg/mod/github.com/spf13/cobra@v1.8.1/command.go:989 +0xa91
github.com/spf13/cobra.(*Command).ExecuteC(0xc00040e908)
	/home/lmohanty/go/pkg/mod/github.com/spf13/cobra@v1.8.1/command.go:1117 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
	/home/lmohanty/go/pkg/mod/github.com/spf13/cobra@v1.8.1/command.go:1041
github.com/operator-framework/kubectl-operator/internal/cmd.Execute()
	kubectl-operator/internal/cmd/root.go:14 +0x18
main.main()
	kubectl-operator/main.go:8 +0xf

Also adding some nil value checks for printing clusterCatalogs and clusterExtensions.

[ankitathomas]

/hold

[ankitathomas]

hold till #225 merges

[azych]

@ankitathomas could you explain what the issue with the client is and how it can be reproduced?

Can cfg *action.Configuration somehow be nil when it's passed to NewCatalogCreateCmd, which would make attempting to pass .Client panic?

If not and if the Client can be nil, how do the changes in this PR fix the situation?
We're still not validating if the i.config.Client is nil before calling it, just like we didin't validate if i.client was nil before.

Can the cfg.Client be modified/replaced during cmd.Execute?

[ankitathomas]

Can cfg *action.Configuration somehow be nil when it's passed to NewCatalogCreateCmd, which would make attempting to pass .Client panic?

what happened was that when v1action.NewCatalogCreate(cfg.Client) gets called within NewCatalogCreateCmd, we pass it into CatalogCreate{}.

	return &CatalogCreate{
		client: client,
		Logf:   func(string, ...interface{}) {},
	}

This would be ok if the actionConfig client is initialized beforehand, but that happens in the PersistentPreRunE for the root command and until that runs, actionConfig.client is nil.

Since command registration happens before PersistentPreRunE gets called, we end up passing the value of cfg.Client (nil) rather than the pointer to cfg.Client, setting CatalogCreate.client = cfg.Client or CatalogCreate.client = nil.

A workaround was to pass the entire actionConfig so by the time we end up using the client in CatalogCreate.Run, PersistentPreRunE gets called and CatalogCreate.cfg.client is initialized.

[LalatenduMohanty]

/lgtm

[LalatenduMohanty]

hold till #225 merges

@ankitathomas Can you please clarify why we need to wait for #225?

[LalatenduMohanty]

/hold cancel

[LalatenduMohanty]

@ankitathomas Looks like lint test is failing with below errors

Run make lint
(re)installing /home/runner/go/bin/golangci-lint-v1.63.4
/home/runner/go/bin/golangci-lint-v1.63.4 --timeout 3m run
internal/pkg/v1/action/catalog_create_test.go:43:24   errcheck  Error return value of `mockClient.Initialize` is not checked
internal/pkg/v1/action/catalog_create_test.go:125:24  errcheck  Error return value of `mockClient.Client.Get` is not checked
internal/pkg/v1/action/catalog_create.go:13:6         unused    type `createClient` is unused
internal/pkg/v1/action/interfaces.go:9:6              unused    type `creator` is unused
internal/pkg/v1/action/catalog_create.go:7:1          gci       File is not properly formatted
internal/pkg/v1/action/catalog_create.go:10:1         gci       File is not properly formatted
internal/pkg/v1/action/action_suite_test.go:10:1      gci       File is not properly formatted
make: *** [Makefile:61: lint] Error 1
Error: Process completed with exit code 2.

[LalatenduMohanty]

/lgtm

[LalatenduMohanty]

/lgtm cancel

[LalatenduMohanty]

@ankitathomas Looks like the code for fixing the empty client in action config is missing now. I am wondering how the tests are passing now.

I was trying out a different solution.

I tried moving the *actionConfig.Load() out of PersistentPrerun to the root command declaration - that works to initialize the actionConfig client before it gets passed to the catalog create client, but looking further into it we may run into issues with how we're loading the config if we don't parse the subcommand flags before loading the actionConfig client - mostly because it makes use of config overrides from specific flags (auth, namespace, context etc) that we may support in future.

I didn't want this to be a potential bug in future so I switched the implementation back, it's now what it used to be.

[LalatenduMohanty]

/lgtm