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

lakectl doctor command to run a basic diagnose on lakeFS configuration #2948

Merged
merged 32 commits into from
Feb 21, 2022
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
29522b5
New lakectl command 'doctor' to run a basic diagnosis of lakeFS conf…
Feb 14, 2022
2528ff0
Fixed command outputs
Feb 14, 2022
3e69a10
Fixed command outputs
Feb 14, 2022
531371c
Fixed linter comments
Feb 14, 2022
9c4e5de
Fixed linter comments
Feb 14, 2022
91b1b89
Fixed a bug of ignoring the '--config' flag
Feb 14, 2022
c2fb59b
Added tests for bad config files
Feb 14, 2022
7e77e9e
Fixed tests to pass on runners
Feb 14, 2022
23d3eed
changed IsValidAccessKeyID to be more readable
idanovo Feb 14, 2022
8cf0aa6
Fixed PR review
Feb 14, 2022
77f172b
Changed IsContainOnlyUpperLettersOrNumber func to work with regex
Feb 14, 2022
6c80ac2
Fixed a test to pass on runners
Feb 15, 2022
d19fcf1
Changed IsValidAccessKeyID to work with regex
Feb 15, 2022
d93230a
Fixed tests and changed 'Lakectl' func to reuse 'LakectlWithParams' code
Feb 15, 2022
6cc1ce6
Checking config file only if the basic command did not work
Feb 15, 2022
de505a9
Added unittests and changed IsValidAccessKeyID func to be more specif…
Feb 15, 2022
d371c8a
Fixed linter errors
Feb 15, 2022
e78fbb0
Advising the user to use command on failure
Feb 15, 2022
c6b4a90
Apply style suggestions from code review
idanovo Feb 15, 2022
f6cc60c
Added a funnc that returnn what was the error we got while analyzing…
Feb 15, 2022
8575f73
Changed text for the default config error
Feb 16, 2022
a3c5a42
small styling change
Feb 16, 2022
db49f19
Return if no configuration issue was found
Feb 16, 2022
5a45e32
Fixed a test
Feb 16, 2022
2665e12
removed unused file
Feb 16, 2022
f91f9fa
Added a new line on output
Feb 16, 2022
834c113
Refcator for errors messages
Feb 17, 2022
3d13630
Changed errors structs names
idanovo Feb 20, 2022
4644576
Fixed linter errors
idanovo Feb 20, 2022
5fc6254
Fixed golden file
idanovo Feb 20, 2022
7a1853b
Fixed linter errors
idanovo Feb 20, 2022
04e4185
empty commit
idanovo Feb 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cmd/lakectl/cmd/common_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ package cmd

idanovo marked this conversation as resolved.
Show resolved Hide resolved
import (
"bytes"
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"os"
"regexp"
"strings"
"text/template"
"time"
Expand All @@ -27,6 +29,8 @@ var noColorRequested = false
// ErrInvalidValueInList is an error returned when a parameter of type list contains an empty string
var ErrInvalidValueInList = errors.New("empty string in list")

var accessKeyRegexp = regexp.MustCompile(`^AKIA[I|J][A-Z0-9]{14}Q$`)

const (
LakectlInteractive = "LAKECTL_INTERACTIVE"
LakectlInteractiveDisable = "no"
Expand Down Expand Up @@ -267,3 +271,16 @@ func MustParsePathURI(name, s string) *uri.URI {
}
return u
}

func IsValidAccessKeyID(accessKeyID string) bool {
return accessKeyRegexp.MatchString(accessKeyID)
}

func IsValidSecretAccessKey(secretAccessKey string) bool {
return IsBase64(secretAccessKey) && len(secretAccessKey) == 40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread gone because the line changed...

Cool. But then please return a string explaining to the user what is suspicious about the URL: is it impossible to parse (which is definitely wrong), or does it have a weird suffix (which is probably wrong). The user can and should take different steps in each case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might still want to do this :-)

}

func IsBase64(s string) bool {
_, err := base64.StdEncoding.DecodeString(s)
return err == nil
}
52 changes: 52 additions & 0 deletions cmd/lakectl/cmd/common_helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package cmd

import "testing"

func TestIsValidAccessKeyID(t *testing.T) {
type args struct {
accessKeyID string
}
tests := []struct {
name string
args args
want bool
}{
{name: "valid access key id", args: args{accessKeyID: "AKIAJ12ZZZZZZZZZZZZQ"}, want: true},
{name: "access key id with lower case char", args: args{accessKeyID: "AKIAJ12zZZZZZZZZZZZQ"}, want: false},
{name: "access key id with invalid char", args: args{accessKeyID: "AKIAJ12!ZZZZZZZZZZZQ"}, want: false},
{name: "access key id with extra char", args: args{accessKeyID: "AKIAJ123ZZZZZZZZZZZZQ"}, want: false},
{name: "access key id with missing char", args: args{accessKeyID: "AKIAJ1ZZZZZZZZZZZZQ"}, want: false},
{name: "access key id with wrong prefix", args: args{accessKeyID: "AKIAM12ZZZZZZZZZZZZQ"}, want: false},
{name: "access key id with wrong suffiix", args: args{accessKeyID: "AKIAJ12ZZZZZZZZZZZZA"}, want: false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsValidAccessKeyID(tt.args.accessKeyID); got != tt.want {
t.Errorf("IsValidAccessKeyID() = %v, want %v", got, tt.want)
}
})
}
}

func TestIsValidSecretAccessKey(t *testing.T) {
type args struct {
secretAccessKey string
}
tests := []struct {
name string
args args
want bool
}{
{name: "valid secret access key", args: args{secretAccessKey: "TQG5JcovOozCGJnIRmIKH7Flq1tLxrUbyi9/WmJy"}, want: true},
{name: "secret access key id with invalid char", args: args{secretAccessKey: "!QG5JcovOozCGJnIRmIKH7Flq1tLxrUbyi9/WmJy"}, want: false},
{name: "secret access key id with extra char", args: args{secretAccessKey: "aTQG5JcovOozCGJnIRmIKH7Flq1tLxrUbyi9/WmJy"}, want: false},
{name: "secret access key id with missing char", args: args{secretAccessKey: "QG5JcovOozCGJnIRmIKH7Flq1tLxrUbyi9/WmJy"}, want: false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsValidSecretAccessKey(tt.args.secretAccessKey); got != tt.want {
t.Errorf("IsValidSecretAccessKey() = %v, want %v", got, tt.want)
}
})
}
}
141 changes: 141 additions & 0 deletions cmd/lakectl/cmd/doctor.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package cmd

import (
"context"
"errors"
"net/url"
"strings"

"github.com/spf13/cobra"
"github.com/spf13/viper"
"github.com/treeverse/lakefs/pkg/api"
)

type Detailed interface {
GetDetails() string
}

type CredentialsError struct {
Message string
Details string
}

func (e *CredentialsError) Error() string { return e.Message }

func (e *CredentialsError) GetDetails() string {
return e.Message + "\n" + e.Details
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference (none of this is required in this PR, but it would be more Go-ish if you did do these): these errors work, but are a bit dirty. They break the errors wrapping chain. You could have instead

type WrappingError struct {
	Message string
	DetailedErr error
}

func (e *WrappingError) Error() string { return e.Message }

func (e *WrappingError) Unwrap() error { return e.DetailedErr }

func (e *WrappingError) Details() string { return c.DetailedErr.Error() }

The Unwrap call means the errors.Is and errors.As will continue to work on this error.

Now copy this type into every desired type, e.g.

type WrongEndpointURIError WrappingError

var ErrWrongEndpointURI = WrongEndpointERIError{}

and you get all the parts in one place.

type WrongEndpointURIError struct {
Message string
Details string
}

func (e *WrongEndpointURIError) Error() string { return e.Message }

func (e *WrongEndpointURIError) GetDetails() string {
return e.Message + "\n" + e.Details
}

type UnknownConfigError struct {
Message string
Details string
}

func (e *UnknownConfigError) Error() string { return e.Message }

func (e *UnknownConfigError) GetDetails() string {
return e.Message + "\n" + e.Details
}

type UserMessage struct {
Message string
}

var detailedErrorTemplate = `{{ .Message |red }}
{{ .Details }}
`
var errorTemplate = `{{ .Message |red }}
`

var successMessageTemplate = `{{ .Message | green}}
`

var analayzingMessageTemplate = `{{ .Message }}
`

// doctorCmd represents the doctor command
var doctorCmd = &cobra.Command{
Use: "doctor",
Short: "Run a basic diagnosis of the LakeFS configuration",
Run: func(cmd *cobra.Command, args []string) {
err := ListRepositoriesAndAnalyze(cmd.Context())
if err == nil {
Write(successMessageTemplate, &UserMessage{"Valid configuration"})
return
}

if detailedErr, ok := err.(Detailed); ok {
Write(detailedErrorTemplate, detailedErr)
} else {
Write(errorTemplate, err)
}

accessKeyID := cfg.Values.Credentials.AccessKeyID
if !IsValidAccessKeyID(accessKeyID) {
Write(analayzingMessageTemplate, &UserMessage{"access_key_id value looks suspicious: " + accessKeyID})
}

secretAccessKey := cfg.Values.Credentials.SecretAccessKey
if !IsValidSecretAccessKey(secretAccessKey) {
Write(analayzingMessageTemplate, &UserMessage{"secret_access_key value looks suspicious..."})
}

serverEndpoint := cfg.Values.Server.EndpointURL
if !strings.HasSuffix(serverEndpoint, api.BaseURL) {
Write(analayzingMessageTemplate, &UserMessage{"Suspicious URI format for server.endpoint_url: " + serverEndpoint + " doesn't end with: `" + api.BaseURL + "`."})
}
},
}

func ListRepositoriesAndAnalyze(ctx context.Context) error {
configFileName := viper.GetViper().ConfigFileUsed()
msgOnErrUnknownConfig := "It looks like you have a problem with your `" + configFileName + "` file."
msgOnErrWrongEndpointURI := "It looks like endpoint url is wrong."
msgOnErrCredential := "It seems like the `access_key_id` or `secret_access_key` you supplied are wrong."

// getClient might die on url.Parse error, so check it first.
serverEndpoint := cfg.Values.Server.EndpointURL
_, err := url.Parse(serverEndpoint)
arielshaqed marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return &WrongEndpointURIError{msgOnErrWrongEndpointURI, err.Error()}
}
client := getClient()
resp, err := client.ListRepositoriesWithResponse(ctx, &api.ListRepositoriesParams{})

switch {
case err != nil:
urlErr := &url.Error{}
if errors.As(err, &urlErr) {
return &WrongEndpointURIError{msgOnErrWrongEndpointURI, err.Error()}
}
return &UnknownConfigError{msgOnErrUnknownConfig, err.Error()}
case resp == nil:
break
case resp.JSON200 != nil:
return nil
case resp.JSON401 != nil:
return &CredentialsError{msgOnErrCredential, resp.JSON401.Message}
// In case we get the "not found" HTML page (the status is 200 and not 404 in this case)
case resp.HTTPResponse != nil && resp.HTTPResponse.StatusCode == 200:
return &WrongEndpointURIError{msgOnErrWrongEndpointURI, ""}
case resp.JSONDefault != nil:
return &UnknownConfigError{msgOnErrUnknownConfig, resp.JSONDefault.Message}
}
return &UnknownConfigError{msgOnErrUnknownConfig, "An unknown error accourd while trying to analyzing LakeCtl configuration."}
}

//nolint:gochecknoinits
func init() {
rootCmd.AddCommand(doctorCmd)
}
17 changes: 17 additions & 0 deletions docs/reference/commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -1735,6 +1735,23 @@ lakectl docs [outfile] [flags]



### lakectl doctor

Run a basic diagnosis of the LakeFS configuration
idanovo marked this conversation as resolved.
Show resolved Hide resolved

```
lakectl doctor [flags]
```

#### Options
{:.no_toc}

```
-h, --help help for doctor
```



### lakectl fs

View and manipulate objects
Expand Down
25 changes: 25 additions & 0 deletions nessie/doctor_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package nessie

import (
"os"
"testing"

"github.com/spf13/viper"
)

func TestDoctor(t *testing.T) {
idanovo marked this conversation as resolved.
Show resolved Hide resolved
accessKeyID := viper.GetString("access_key_id")
secretAccessKey := viper.GetString("secret_access_key")
endPointURL := viper.GetString("endpoint_url") + "/api/v1"
defaultConfigPath := "/root/.lakectl.yaml"
os.Create(defaultConfigPath)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams(accessKeyID, secretAccessKey, endPointURL)+" doctor", false, "lakectl_doctor_ok", emptyVars)
RunCmdAndVerifyFailureWithFile(t, lakectlLocation()+" doctor -c not_exits.yaml", false, "lakectl_doctor_not_exists_file", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams(accessKeyID, secretAccessKey, endPointURL+"1")+" doctor", false, "lakectl_doctor_wrong_endpoint", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams(accessKeyID, secretAccessKey, "wrong_uri")+" doctor", false, "lakectl_doctor_wrong_uri_format_endpoint", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams("AKIAJZZZZZZZZZZZZZZQ", secretAccessKey, endPointURL)+" doctor", false, "lakectl_doctor_with_wrong_credentials", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams("AKIAJOI!COZ5JBYHCSDQ", secretAccessKey, endPointURL)+" doctor", false, "lakectl_doctor_with_suspicious_access_key_id", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams(accessKeyID, "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", endPointURL)+" doctor", false, "lakectl_doctor_with_wrong_credentials", emptyVars)
RunCmdAndVerifySuccessWithFile(t, LakectlWithParams(accessKeyID, "TQG5JcovOozCGJnIRmIKH7Flq1tLxnuByi9/WmJ!", endPointURL)+" doctor", false, "lakectl_doctor_with_suspicious_secret_access_key", emptyVars)

}
2 changes: 2 additions & 0 deletions nessie/golden/lakectl_doctor_not_exists_file.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
error reading configuration file: open not_exits.yaml: no such file or directory
Error executing command.
1 change: 1 addition & 0 deletions nessie/golden/lakectl_doctor_ok.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Valid configuration
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
It seems like the `access_key_id` or `secret_access_key` you supplied are wrong.
error authenticating request
access_key_id value looks suspicious: AKIAJOI!COZ5JBYHCSDQ
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
It seems like the `access_key_id` or `secret_access_key` you supplied are wrong.
error authenticating request
secret_access_key value looks suspicious...
2 changes: 2 additions & 0 deletions nessie/golden/lakectl_doctor_with_wrong_credentials.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
It seems like the `access_key_id` or `secret_access_key` you supplied are wrong.
error authenticating request
3 changes: 3 additions & 0 deletions nessie/golden/lakectl_doctor_wrong_endpoint.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
It looks like endpoint url is wrong.

Suspicious URI format for server.endpoint_url: http://lakefs:8000/api/v11 doesn't end with: `/api/v1`.
3 changes: 3 additions & 0 deletions nessie/golden/lakectl_doctor_wrong_uri_format_endpoint.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
It looks like endpoint url is wrong.
Get "/wrong_uri/repositories": unsupported protocol scheme ""
Suspicious URI format for server.endpoint_url: wrong_uri doesn't end with: `/api/v1`.
1 change: 1 addition & 0 deletions nessie/golden/lakectl_help.golden
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Available Commands:
config Create/update local lakeFS configuration
dbt Integration with dbt commands
diff Show changes between two commits, or the currently uncommitted changes
doctor Run a basic diagnosis of the LakeFS configuration
fs View and manipulate objects
gc Manage garbage collection configuration
help Help about any command
Expand Down
12 changes: 8 additions & 4 deletions nessie/lakectl_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,20 @@ func lakectlLocation() string {
return viper.GetString("lakectl_dir") + "/lakectl"
}

func Lakectl() string {
func LakectlWithParams(accessKeyID, secretAccessKey, endPointURL string) string {
lakectlCmdline :=
"LAKECTL_CREDENTIALS_ACCESS_KEY_ID=" + viper.GetString("access_key_id") +
" LAKECTL_CREDENTIALS_SECRET_ACCESS_KEY=" + viper.GetString("secret_access_key") +
" LAKECTL_SERVER_ENDPOINT_URL=" + viper.GetString("endpoint_url") +
"LAKECTL_CREDENTIALS_ACCESS_KEY_ID=" + accessKeyID +
" LAKECTL_CREDENTIALS_SECRET_ACCESS_KEY=" + secretAccessKey +
" LAKECTL_SERVER_ENDPOINT_URL=" + endPointURL +
" " + lakectlLocation()

return lakectlCmdline
}

func Lakectl() string {
return LakectlWithParams(viper.GetString("access_key_id"), viper.GetString("secret_access_key"), viper.GetString("endpoint_url"))
}

func runShellCommand(command string, isTerminal bool) ([]byte, error) {
fmt.Println("Testing '", command, "'")
var cmd *exec.Cmd
Expand Down