From de5b09e395eab7e7acb2e33678fcfcd795218150 Mon Sep 17 00:00:00 2001 From: faiq Date: Wed, 3 Jan 2024 13:55:19 -0500 Subject: [PATCH 1/6] fix: use wrapper errors to clearly denote issues in client building --- pkg/client/client.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index c89eacc796..29c7f83b73 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -69,9 +69,10 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta if prismCentralInfo.Port == 0 { return nil, fmt.Errorf("cannot get credentials if Prism Port is not set") } - credentialRef := prismCentralInfo.CredentialRef - if credentialRef == nil { - return nil, fmt.Errorf("credentialRef must be set on prismCentral attribute for cluster %s in namespace %s", nutanixCluster.Name, nutanixCluster.Namespace) + credentialRef, err := GetCredentialRefForCluster(nutanixCluster) + if err != nil { + //nolint:wrapcheck // error is alredy wrapped + return nil, err } // If namespace is empty, use the cluster namespace if credentialRef.Namespace == "" { @@ -94,7 +95,7 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // Add env provider for CAPX manager npe, err := n.getManagerNutanixPrismEndpoint() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create prism endpoint %w", err) } // If namespaces is not set, set it to the namespace of the CAPX manager if npe.CredentialRef.Namespace == "" { @@ -123,7 +124,7 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // fetch endpoint details me, err := env.GetManagementEndpoint(envTypes.Topology{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get management endpoint object %w", err) } creds := prismgoclient.Credentials{ URL: me.Address.Host, @@ -155,15 +156,14 @@ func (n *NutanixClientHelper) GetClient(cred prismgoclient.Credentials, addition } cli, err := nutanixClientV3.NewV3Client(cred, clientOpts...) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create new nutanix client %w", err) } // Check if the client is working _, err = cli.V3.GetCurrentLoggedInUser(context.Background()) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get current logged in user with client %w", err) } - return cli, nil } @@ -171,10 +171,10 @@ func (n *NutanixClientHelper) getManagerNutanixPrismEndpoint() (*credentialTypes npe := &credentialTypes.NutanixPrismEndpoint{} config, err := n.readEndpointConfig() if err != nil { - return npe, err + return nil, fmt.Errorf("failed to read config %w", err) } if err = json.Unmarshal(config, npe); err != nil { - return npe, err + return nil, fmt.Errorf("failed to unmarshal config %w", err) } if npe.CredentialRef == nil { return nil, fmt.Errorf("credentialRef must be set on CAPX manager") @@ -196,16 +196,16 @@ func GetCredentialRefForCluster(nutanixCluster *infrav1.NutanixCluster) (*creden if nutanixCluster == nil { return nil, fmt.Errorf("cannot get credential reference if nutanix cluster object is nil") } - prismCentralinfo := nutanixCluster.Spec.PrismCentral - if prismCentralinfo == nil { + prismCentralInfo := nutanixCluster.Spec.PrismCentral + if prismCentralInfo == nil { return nil, nil } - if prismCentralinfo.CredentialRef == nil { + if prismCentralInfo.CredentialRef == nil { return nil, fmt.Errorf("credentialRef must be set on prismCentral attribute for cluster %s in namespace %s", nutanixCluster.Name, nutanixCluster.Namespace) } - if prismCentralinfo.CredentialRef.Kind != credentialTypes.SecretKind { + if prismCentralInfo.CredentialRef.Kind != credentialTypes.SecretKind { return nil, nil } - return prismCentralinfo.CredentialRef, nil + return prismCentralInfo.CredentialRef, nil } From babe2c649bc677009c7e4effb5631f087e474fd9 Mon Sep 17 00:00:00 2001 From: faiq Date: Wed, 3 Jan 2024 15:41:13 -0500 Subject: [PATCH 2/6] fix: adds a function to properly sanitize the address --- pkg/client/client.go | 24 +++++++++++++++--- pkg/client/client_test.go | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) create mode 100644 pkg/client/client_test.go diff --git a/pkg/client/client.go b/pkg/client/client.go index 29c7f83b73..1ad0a57538 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "net/url" "os" "path/filepath" @@ -43,6 +44,8 @@ const ( capxNamespaceKey = "POD_NAMESPACE" ) +var ErrPrismAddressNotSet = fmt.Errorf("cannot get credentials if Prism Address is not set") + type NutanixClientHelper struct { secretInformer coreinformers.SecretInformer configMapInformer coreinformers.ConfigMapInformer @@ -63,9 +66,11 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // If PrismCentral is set, add the required env provider prismCentralInfo := nutanixCluster.Spec.PrismCentral if prismCentralInfo != nil { - if prismCentralInfo.Address == "" { - return nil, fmt.Errorf("cannot get credentials if Prism Address is not set") + address, err := validateAndSanitizePrismCentralInfoAddress(prismCentralInfo.Address) + if err != nil { + return nil, err } + prismCentralInfo.Address = address if prismCentralInfo.Port == 0 { return nil, fmt.Errorf("cannot get credentials if Prism Port is not set") } @@ -116,7 +121,6 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta *npe, n.secretInformer, n.configMapInformer)) - // init env with providers env := environment.NewEnvironment( providers..., @@ -209,3 +213,17 @@ func GetCredentialRefForCluster(nutanixCluster *infrav1.NutanixCluster) (*creden return prismCentralInfo.CredentialRef, nil } + +func validateAndSanitizePrismCentralInfoAddress(address string) (string, error) { + if address == "" { + return "", ErrPrismAddressNotSet + } + u, err := url.Parse(address) + if err != nil { + return "", fmt.Errorf("failed to parse url from given address %w", err) + } + if u.Scheme != "" || u.Port() != "" { + return u.Hostname(), nil + } + return address, nil +} diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go new file mode 100644 index 0000000000..f36664ad71 --- /dev/null +++ b/pkg/client/client_test.go @@ -0,0 +1,52 @@ +package client + +import ( + "errors" + "testing" +) + +func Test_validateAndSanitizePrismCentralInfoAddress(t *testing.T) { + tests := []struct { + name string + providedAddress string + expectedAddress string + expectedErr error + }{ + { + "with scheme", + "https://prism-bowser.ntnxsherlock.com", + "prism-bowser.ntnxsherlock.com", + nil, + }, + { + "with scheme and port", + "https://prism-bowser.ntnxsherlock.com:9440", + "prism-bowser.ntnxsherlock.com", + nil, + }, + { + "as expected from the user", + "prism-bowser.ntnxsherlock.com", + "prism-bowser.ntnxsherlock.com", + nil, + }, + { + "not set", + "", + "", + ErrPrismAddressNotSet, + }, + } + + for _, test := range tests { + s, err := validateAndSanitizePrismCentralInfoAddress(test.providedAddress) + if err != nil { + if test.expectedErr == nil || !errors.Is(err, test.expectedErr) { + t.Errorf("validateAndSanitizePrismCentralInfoAddress() error = %v, wantErr = %v", err, test.expectedErr) + } + } + if s != test.expectedAddress { + t.Errorf("validateAndSanitizePrismCentralInfoAddress() got = %v, want = %v", s, test.expectedAddress) + } + } +} From 0396a8496151dce53b68cb6d1f52ac2e24a5d4da Mon Sep 17 00:00:00 2001 From: faiq Date: Thu, 4 Jan 2024 10:53:37 -0500 Subject: [PATCH 3/6] fix: adds tests for ip address case given --- pkg/client/client.go | 25 ++++++++++++++++++++++--- pkg/client/client_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 1ad0a57538..1412b37c5a 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "net" "net/url" "os" "path/filepath" @@ -218,12 +219,30 @@ func validateAndSanitizePrismCentralInfoAddress(address string) (string, error) if address == "" { return "", ErrPrismAddressNotSet } - u, err := url.Parse(address) - if err != nil { - return "", fmt.Errorf("failed to parse url from given address %w", err) + u, urlParseErr := url.Parse(address) + if urlParseErr != nil { + ipHost, ipErr := parseIP(address) + if ipErr != nil { + return "", fmt.Errorf("failed to resolve %s as url or ip addr %w", address, ipErr) + } + return ipHost, nil } if u.Scheme != "" || u.Port() != "" { return u.Hostname(), nil } return address, nil } + +func parseIP(s string) (string, error) { + ip, _, err := net.SplitHostPort(s) + if err == nil { + return ip, nil + } + + ip2 := net.ParseIP(s) + if ip2 == nil { + return "", fmt.Errorf("invalid IP %s", ip2) + } + + return ip2.String(), nil +} diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index f36664ad71..190e3f663a 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -36,6 +36,30 @@ func Test_validateAndSanitizePrismCentralInfoAddress(t *testing.T) { "", ErrPrismAddressNotSet, }, + { + "using a IP with scheme and port", + "https://1.2.3.4:9440", + "1.2.3.4", + nil, + }, + { + "using a IP with scheme", + "https://1.2.3.4", + "1.2.3.4", + nil, + }, + { + "using a IP with port", + "1.2.3.4:9440", + "1.2.3.4", + nil, + }, + { + "just an IP", + "1.2.3.4", + "1.2.3.4", + nil, + }, } for _, test := range tests { From f335e7ad4de592768b45f5a177c4b3c94aa58f51 Mon Sep 17 00:00:00 2001 From: faiq Date: Thu, 4 Jan 2024 11:02:58 -0500 Subject: [PATCH 4/6] fix: uses a defined type for port error --- pkg/client/client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 1412b37c5a..5f6ec85710 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -46,6 +46,7 @@ const ( ) var ErrPrismAddressNotSet = fmt.Errorf("cannot get credentials if Prism Address is not set") +var ErrPrismPortNotSet = fmt.Errorf("cannot get credentials if Prism Port is not set") type NutanixClientHelper struct { secretInformer coreinformers.SecretInformer @@ -73,7 +74,7 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta } prismCentralInfo.Address = address if prismCentralInfo.Port == 0 { - return nil, fmt.Errorf("cannot get credentials if Prism Port is not set") + return nil, ErrPrismPortNotSet } credentialRef, err := GetCredentialRefForCluster(nutanixCluster) if err != nil { From cf34f59162dafe0bb3a7ff4fe9101675918dedbb Mon Sep 17 00:00:00 2001 From: faiq Date: Thu, 4 Jan 2024 17:22:55 -0500 Subject: [PATCH 5/6] fix: clean up variable naming --- pkg/client/client.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 5f6ec85710..c6d2705e51 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -235,15 +235,13 @@ func validateAndSanitizePrismCentralInfoAddress(address string) (string, error) } func parseIP(s string) (string, error) { - ip, _, err := net.SplitHostPort(s) + ipStr, _, err := net.SplitHostPort(s) if err == nil { - return ip, nil + return ipStr, nil } - - ip2 := net.ParseIP(s) - if ip2 == nil { - return "", fmt.Errorf("invalid IP %s", ip2) + ip := net.ParseIP(s) + if ip == nil { + return "", fmt.Errorf("invalid IP %s", ip) } - - return ip2.String(), nil + return ip.String(), nil } From 40872bd1793bfacfbc95ca06b86299a81a28bf6d Mon Sep 17 00:00:00 2001 From: faiq Date: Mon, 8 Jan 2024 10:47:08 -0500 Subject: [PATCH 6/6] fix: remove validation here to be moved into prism-client --- pkg/client/client.go | 50 +++++--------------------- pkg/client/client_test.go | 76 --------------------------------------- 2 files changed, 8 insertions(+), 118 deletions(-) delete mode 100644 pkg/client/client_test.go diff --git a/pkg/client/client.go b/pkg/client/client.go index c6d2705e51..35dc3bb017 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -20,8 +20,6 @@ import ( "context" "encoding/json" "fmt" - "net" - "net/url" "os" "path/filepath" @@ -68,11 +66,9 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // If PrismCentral is set, add the required env provider prismCentralInfo := nutanixCluster.Spec.PrismCentral if prismCentralInfo != nil { - address, err := validateAndSanitizePrismCentralInfoAddress(prismCentralInfo.Address) - if err != nil { - return nil, err + if prismCentralInfo.Address == "" { + return nil, ErrPrismAddressNotSet } - prismCentralInfo.Address = address if prismCentralInfo.Port == 0 { return nil, ErrPrismPortNotSet } @@ -102,7 +98,7 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // Add env provider for CAPX manager npe, err := n.getManagerNutanixPrismEndpoint() if err != nil { - return nil, fmt.Errorf("failed to create prism endpoint %w", err) + return nil, fmt.Errorf("failed to create prism endpoint: %w", err) } // If namespaces is not set, set it to the namespace of the CAPX manager if npe.CredentialRef.Namespace == "" { @@ -130,7 +126,7 @@ func (n *NutanixClientHelper) GetClientFromEnvironment(ctx context.Context, nuta // fetch endpoint details me, err := env.GetManagementEndpoint(envTypes.Topology{}) if err != nil { - return nil, fmt.Errorf("failed to get management endpoint object %w", err) + return nil, fmt.Errorf("failed to get management endpoint object: %w", err) } creds := prismgoclient.Credentials{ URL: me.Address.Host, @@ -162,13 +158,13 @@ func (n *NutanixClientHelper) GetClient(cred prismgoclient.Credentials, addition } cli, err := nutanixClientV3.NewV3Client(cred, clientOpts...) if err != nil { - return nil, fmt.Errorf("failed to create new nutanix client %w", err) + return nil, fmt.Errorf("failed to create new nutanix client: %w", err) } // Check if the client is working _, err = cli.V3.GetCurrentLoggedInUser(context.Background()) if err != nil { - return nil, fmt.Errorf("failed to get current logged in user with client %w", err) + return nil, fmt.Errorf("failed to get current logged in user with client: %w", err) } return cli, nil } @@ -177,10 +173,10 @@ func (n *NutanixClientHelper) getManagerNutanixPrismEndpoint() (*credentialTypes npe := &credentialTypes.NutanixPrismEndpoint{} config, err := n.readEndpointConfig() if err != nil { - return nil, fmt.Errorf("failed to read config %w", err) + return nil, fmt.Errorf("failed to read config: %w", err) } if err = json.Unmarshal(config, npe); err != nil { - return nil, fmt.Errorf("failed to unmarshal config %w", err) + return nil, fmt.Errorf("failed to unmarshal config: %w", err) } if npe.CredentialRef == nil { return nil, fmt.Errorf("credentialRef must be set on CAPX manager") @@ -215,33 +211,3 @@ func GetCredentialRefForCluster(nutanixCluster *infrav1.NutanixCluster) (*creden return prismCentralInfo.CredentialRef, nil } - -func validateAndSanitizePrismCentralInfoAddress(address string) (string, error) { - if address == "" { - return "", ErrPrismAddressNotSet - } - u, urlParseErr := url.Parse(address) - if urlParseErr != nil { - ipHost, ipErr := parseIP(address) - if ipErr != nil { - return "", fmt.Errorf("failed to resolve %s as url or ip addr %w", address, ipErr) - } - return ipHost, nil - } - if u.Scheme != "" || u.Port() != "" { - return u.Hostname(), nil - } - return address, nil -} - -func parseIP(s string) (string, error) { - ipStr, _, err := net.SplitHostPort(s) - if err == nil { - return ipStr, nil - } - ip := net.ParseIP(s) - if ip == nil { - return "", fmt.Errorf("invalid IP %s", ip) - } - return ip.String(), nil -} diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go deleted file mode 100644 index 190e3f663a..0000000000 --- a/pkg/client/client_test.go +++ /dev/null @@ -1,76 +0,0 @@ -package client - -import ( - "errors" - "testing" -) - -func Test_validateAndSanitizePrismCentralInfoAddress(t *testing.T) { - tests := []struct { - name string - providedAddress string - expectedAddress string - expectedErr error - }{ - { - "with scheme", - "https://prism-bowser.ntnxsherlock.com", - "prism-bowser.ntnxsherlock.com", - nil, - }, - { - "with scheme and port", - "https://prism-bowser.ntnxsherlock.com:9440", - "prism-bowser.ntnxsherlock.com", - nil, - }, - { - "as expected from the user", - "prism-bowser.ntnxsherlock.com", - "prism-bowser.ntnxsherlock.com", - nil, - }, - { - "not set", - "", - "", - ErrPrismAddressNotSet, - }, - { - "using a IP with scheme and port", - "https://1.2.3.4:9440", - "1.2.3.4", - nil, - }, - { - "using a IP with scheme", - "https://1.2.3.4", - "1.2.3.4", - nil, - }, - { - "using a IP with port", - "1.2.3.4:9440", - "1.2.3.4", - nil, - }, - { - "just an IP", - "1.2.3.4", - "1.2.3.4", - nil, - }, - } - - for _, test := range tests { - s, err := validateAndSanitizePrismCentralInfoAddress(test.providedAddress) - if err != nil { - if test.expectedErr == nil || !errors.Is(err, test.expectedErr) { - t.Errorf("validateAndSanitizePrismCentralInfoAddress() error = %v, wantErr = %v", err, test.expectedErr) - } - } - if s != test.expectedAddress { - t.Errorf("validateAndSanitizePrismCentralInfoAddress() got = %v, want = %v", s, test.expectedAddress) - } - } -}