Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] flytectl panics when wrong config path is passed #4891

Open
2 tasks done
debajyoti-truefoundry opened this issue Feb 13, 2024 · 4 comments · Fixed by #5972
Open
2 tasks done

[BUG] flytectl panics when wrong config path is passed #4891

debajyoti-truefoundry opened this issue Feb 13, 2024 · 4 comments · Fixed by #5972
Assignees
Labels
bug Something isn't working flytectl Issues related to flytectl -Flytes CLI

Comments

@debajyoti-truefoundry
Copy link

Describe the bug

❯ flytectl version
INFO[0000] [0] Couldn't find a config file []. Relying on env vars and pflags.

 A new release of flytectl is available: 0.8.11 → v0.8.12
To upgrade, run: brew update && brew upgrade flytectl
https://github.com/flyteorg/flytectl/releases/tag/v0.8.12

{
  "App": "flytectl",
  "Build": "71fbaf4",
  "Version": "0.8.11",
  "BuildTime": "2024-02-13 22:09:28.318664 +0530 IST m=+0.027465309"
}
pipelines/flyte/my_workflow on  main [$!?⇡] via 🐍 v3.11.7 (venv)
❯ flytectl --config config.yaml update launchplan --project flytesnacks --domain development --deactivate train-cron --version 1707840433
INFO[0000] [0] Couldn't find a config file []. Relying on env vars and pflags.
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x28af821]

goroutine 1 [running]:
github.com/flyteorg/flytectl/cmd/core.CommandContext.AdminClient(...)
        /home/runner/work/flytectl/flytectl/cmd/core/cmd_ctx.go:57
github.com/flyteorg/flytectl/cmd/update.updateLPFunc({0x33da9d8, 0xc000054068}, {0xc00094f5e0, 0x1, 0x33da680?}, {0x0, {0x0, 0x0}, {0x0, 0x0}, ...})
        /home/runner/work/flytectl/flytectl/cmd/update/launch_plan.go:76 +0x381
github.com/flyteorg/flytectl/cmd/core.generateCommandFunc.func1(0xc0005a9900?, {0xc00094f5e0, 0x1, 0xa})
        /home/runner/work/flytectl/flytectl/cmd/core/cmd.go:70 +0x93d
github.com/spf13/cobra.(*Command).execute(0xc0005a9900, {0xc00094f360, 0xa, 0xa})
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:856 +0x67c
github.com/spf13/cobra.(*Command).ExecuteC(0xc0005e8f00)
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:974 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:902
github.com/flyteorg/flytectl/cmd.ExecuteCmd()
        /home/runner/work/flytectl/flytectl/cmd/root.go:160 +0x1e
main.main()
        /home/runner/work/flytectl/flytectl/main.go:12 +0x1d

Expected behavior

It will not panic.

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@debajyoti-truefoundry debajyoti-truefoundry added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Feb 13, 2024
Copy link

dosubot bot commented Feb 13, 2024

🤖

Hey @debajyoti-truefoundry! Great to see you back. Hope you're doing well.

Thank you for bringing this to our attention. The panic you're experiencing when a wrong config path is passed is likely due to the fact that the error is not properly handled in the initConfig function. When the UpdateConfig function is called, it may return an error if the config file is not found or is invalid. However, this error is not checked and the function continues to execute, which may lead to a panic later on.

To resolve this issue, you should add error handling after the UpdateConfig function call. If an error is returned, you should return it immediately to prevent further execution. Here is the updated initConfig function:

func (r *RootOptions) initConfig(cmd *cobra.Command, _ []string) error {
	r.configAccessor = viper.NewAccessor(config.Options{
		StrictMode:  true,
		SearchPaths: []string{r.cfgFile},
	})

	rootCmd := cmd
	for rootCmd.Parent() != nil {
		rootCmd = rootCmd.Parent()
	}

	// persistent flags were initially bound to the root command so we must bind to the same command to avoid
	r.configAccessor.InitializePflags(rootCmd.PersistentFlags())

	err := r.configAccessor.UpdateConfig(context.TODO())
	if err != nil {
		// Return the error immediately if the config file is not found or is invalid
		return errors.Wrap(err, "failed to update config")
	}
	return nil
}

With this change, if a wrong config path is passed, the initConfig function will return an error and the command execution will stop, preventing a panic.

This solution is similar to the one used in a previously solved issue: [BUG] Can't create project from file in demo.

Please let me know if this resolves your issue or if you need further assistance.

Sources

About Dosu

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@pingsutw pingsutw added flytectl Issues related to flytectl -Flytes CLI and removed untriaged This issues has not yet been looked at by the Maintainers labels Feb 14, 2024
@Terryhung
Copy link
Contributor

#take

@Terryhung
Copy link
Contributor

I found that the root cause is our failure to return an error when the config file is missing. As a result, errors occur when certain commands require the config. I’d like to understand why we chose not to return an error for an empty or missing config file—is there a specific reason for this?

Related:
code1
code2

@eapolinario
Copy link
Contributor

Reverting #6100 for now as this is causing issues for users and in CI in a bunch of our repos (e.g. https://github.com/flyteorg/flytekit/actions/runs/12266004823/job/34223269731?pr=2995).

This change went out in flytectl 0.9.3. I'm looking into how to remove that release once a new version containing the revert is published.

@eapolinario eapolinario reopened this Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working flytectl Issues related to flytectl -Flytes CLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants