Skip to content

Commit 21293e9

Browse files
manoj-nutanixmanoj-nutanix
committed
refactor: incorporates review comments
1 parent 4314fe9 commit 21293e9

File tree

5 files changed

+172
-31
lines changed

5 files changed

+172
-31
lines changed

‎api/v1alpha1/common_types_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import (
88
"github.com/stretchr/testify/assert"
99
)
1010

11-
func TestControlPlaneEndpointIP(t *testing.T) {
11+
func TestVirtualIPAddress(t *testing.T) {
1212
tests := []struct {
1313
name string
1414
spec ControlPlaneEndpointSpec

‎api/v1alpha1/nutanix_clusterconfig_types.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package v1alpha1
55

66
import (
77
"fmt"
8+
"net/netip"
89
"net/url"
910
"strconv"
1011
)
@@ -76,3 +77,17 @@ func (s NutanixPrismCentralEndpointSpec) ParseURL() (string, uint16, error) {
7677

7778
return hostname, uint16(port), nil
7879
}
80+
81+
func (s NutanixPrismCentralEndpointSpec) ParseIP() (netip.Addr, error) {
82+
pcHostname, _, err := s.ParseURL()
83+
if err != nil {
84+
return netip.Addr{}, err
85+
}
86+
87+
pcIP, err := netip.ParseAddr(pcHostname)
88+
if err != nil {
89+
return netip.Addr{}, fmt.Errorf("error parsing Prism Central IP: %w", err)
90+
}
91+
92+
return pcIP, nil
93+
}

‎api/v1alpha1/nutanix_clusterconfig_types_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
// Copyright 2024 Nutanix. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package v1alpha1
5+
6+
import (
7+
"fmt"
8+
"net/netip"
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
func TestParseURL(t *testing.T) {
16+
tests := []struct {
17+
name string
18+
spec NutanixPrismCentralEndpointSpec
19+
expectedHost string
20+
expectedPort uint16
21+
expectedErr error
22+
}{
23+
{
24+
name: "Valid URL with port",
25+
spec: NutanixPrismCentralEndpointSpec{
26+
URL: "https://192.168.1.1:9440",
27+
},
28+
expectedHost: "192.168.1.1",
29+
expectedPort: 9440,
30+
expectedErr: nil,
31+
},
32+
{
33+
name: "Valid URL without port",
34+
spec: NutanixPrismCentralEndpointSpec{
35+
URL: "https://192.168.1.1",
36+
},
37+
expectedHost: "192.168.1.1",
38+
expectedPort: 9440,
39+
expectedErr: nil,
40+
},
41+
{
42+
name: "Invalid URL",
43+
spec: NutanixPrismCentralEndpointSpec{
44+
URL: "invalid-url",
45+
},
46+
expectedHost: "",
47+
expectedPort: 0,
48+
expectedErr: fmt.Errorf(
49+
"error parsing Prism Central URL: parse %q: invalid URI for request",
50+
"invalid-url",
51+
),
52+
},
53+
{
54+
name: "Invalid port",
55+
spec: NutanixPrismCentralEndpointSpec{
56+
URL: "https://192.168.1.1:invalid-port",
57+
},
58+
expectedHost: "",
59+
expectedPort: 0,
60+
expectedErr: fmt.Errorf(
61+
"error parsing Prism Central URL: parse %q: invalid port %q after host",
62+
"https://192.168.1.1:invalid-port",
63+
":invalid-port",
64+
),
65+
},
66+
}
67+
68+
for _, tt := range tests {
69+
t.Run(tt.name, func(t *testing.T) {
70+
host, port, err := tt.spec.ParseURL()
71+
if tt.expectedErr != nil {
72+
require.EqualError(t, err, tt.expectedErr.Error())
73+
} else {
74+
require.NoError(t, err)
75+
}
76+
assert.Equal(t, tt.expectedHost, host)
77+
assert.Equal(t, tt.expectedPort, port)
78+
})
79+
}
80+
}
81+
82+
func TestParseIP(t *testing.T) {
83+
tests := []struct {
84+
name string
85+
spec NutanixPrismCentralEndpointSpec
86+
expectedIP netip.Addr
87+
expectedErr error
88+
}{
89+
{
90+
name: "Valid IP",
91+
spec: NutanixPrismCentralEndpointSpec{
92+
URL: "https://192.168.1.1:9440",
93+
},
94+
expectedIP: netip.MustParseAddr("192.168.1.1"),
95+
expectedErr: nil,
96+
},
97+
{
98+
name: "Invalid URL",
99+
spec: NutanixPrismCentralEndpointSpec{
100+
URL: "invalid-url",
101+
},
102+
expectedIP: netip.Addr{},
103+
expectedErr: fmt.Errorf(
104+
"error parsing Prism Central URL: parse %q: invalid URI for request",
105+
"invalid-url",
106+
),
107+
},
108+
{
109+
name: "Invalid IP",
110+
spec: NutanixPrismCentralEndpointSpec{
111+
URL: "https://invalid-ip:9440",
112+
},
113+
expectedIP: netip.Addr{},
114+
expectedErr: fmt.Errorf(
115+
"error parsing Prism Central IP: ParseAddr(%q): unable to parse IP",
116+
"invalid-ip",
117+
),
118+
},
119+
}
120+
121+
for _, tt := range tests {
122+
t.Run(tt.name, func(t *testing.T) {
123+
ip, err := tt.spec.ParseIP()
124+
if tt.expectedErr != nil {
125+
require.EqualError(t, err, tt.expectedErr.Error())
126+
} else {
127+
require.NoError(t, err)
128+
}
129+
assert.Equal(t, tt.expectedIP, ip)
130+
})
131+
}
132+
}

‎pkg/webhook/cluster/nutanix_validator.go

Lines changed: 19 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"context"
88
"errors"
99
"fmt"
10-
"net"
1110
"net/http"
1211
"net/netip"
1312

@@ -72,7 +71,7 @@ func (a *nutanixValidator) validate(
7271
}
7372

7473
if clusterConfig.Nutanix != nil {
75-
if err := checkIfPrismCentralAndControlPlaneIPSame(
74+
if err := validatePrismCentralIPDoesNotEqualControlPlaneIP(
7675
clusterConfig.Nutanix.PrismCentralEndpoint,
7776
clusterConfig.Nutanix.ControlPlaneEndpoint,
7877
); err != nil {
@@ -105,14 +104,10 @@ func validatePrismCentralIPNotInLoadBalancerIPRange(
105104
return nil
106105
}
107106

108-
pcHostname, _, err := pcEndpoint.ParseURL()
107+
pcIP, err := pcEndpoint.ParseIP()
109108
if err != nil {
110-
return err
111-
}
112-
113-
pcIP := net.ParseIP(pcHostname)
114-
// PC URL can contain IP/FQDN, so compare only if PC is an IP address.
115-
if pcIP == nil {
109+
// If it's not able to parse IP correctly then, ignore the error as
110+
// we want to compare only IP addresses.
116111
return nil
117112
}
118113

@@ -141,31 +136,33 @@ func validatePrismCentralIPNotInLoadBalancerIPRange(
141136
return nil
142137
}
143138

144-
// checkIfPrismCentralAndControlPlaneIPSame checks if Prism Central and Control Plane IP are same.
145-
// It compares strictly IP addresses(no FQDN) and doesn't involve any network calls.
146-
// This is a temporary check until we have a better way to handle this by reserving IPs
147-
// using IPAM provider.
148-
func checkIfPrismCentralAndControlPlaneIPSame(
139+
// validatePrismCentralIPDoesNotEqualControlPlaneIP checks if Prism Central and Control Plane IP are same,
140+
// error out if they are same.
141+
// It strictly compares IP addresses(no FQDN) and doesn't involve any network calls.
142+
func validatePrismCentralIPDoesNotEqualControlPlaneIP(
149143
pcEndpoint v1alpha1.NutanixPrismCentralEndpointSpec,
150144
controlPlaneEndpointSpec v1alpha1.ControlPlaneEndpointSpec,
151145
) error {
152-
controlPlaneEndpointIP, err := netip.ParseAddr(controlPlaneEndpointSpec.VirtualIPAddress())
146+
controlPlaneVIP, err := netip.ParseAddr(controlPlaneEndpointSpec.VirtualIPAddress())
153147
if err != nil {
154148
// If controlPlaneEndpointIP is a hostname, we cannot compare it with PC IP
155149
// so return directly.
156150
return nil
157151
}
158152

159-
pcHostname, _, err := pcEndpoint.ParseURL()
153+
pcIP, err := pcEndpoint.ParseIP()
160154
if err != nil {
161-
return err
155+
// If it's not able to parse IP correctly then, ignore the error as
156+
// we want to compare only IP addresses.
157+
return nil
162158
}
163159

164-
pcIP, err := netip.ParseAddr(pcHostname)
165-
// PC URL can contain IP/FQDN, so compare only if PC is an IP address i.e. error is nil.
166-
if err == nil && pcIP.Compare(controlPlaneEndpointIP) == 0 {
167-
return fmt.Errorf("prism central and control plane endpoint cannot have the same IP %q",
168-
pcIP.String())
160+
if pcIP.Compare(controlPlaneVIP) == 0 {
161+
errMsg := fmt.Sprintf(
162+
"Prism Central and control plane endpoint cannot have the same IP %q",
163+
pcIP.String(),
164+
)
165+
return errors.New(errMsg)
169166
}
170167

171168
return nil

‎pkg/webhook/cluster/nutanix_validator_test.go

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ func TestValidatePrismCentralIPNotInLoadBalancerIPRange(t *testing.T) {
113113
}
114114
}
115115

116-
func TestCheckIfPrismCentralAndControlPlaneIPSame(t *testing.T) {
116+
func TestValidatePrismCentralIPDoesNotEqualControlPlaneIP(t *testing.T) {
117117
tests := []struct {
118118
name string
119119
pcEndpoint v1alpha1.NutanixPrismCentralEndpointSpec
@@ -139,7 +139,7 @@ func TestCheckIfPrismCentralAndControlPlaneIPSame(t *testing.T) {
139139
Host: "192.168.1.1",
140140
},
141141
expectedErr: fmt.Errorf(
142-
"prism central and control plane endpoint cannot have the same IP %q",
142+
"Prism Central and control plane endpoint cannot have the same IP %q",
143143
"192.168.1.1",
144144
),
145145
},
@@ -161,10 +161,7 @@ func TestCheckIfPrismCentralAndControlPlaneIPSame(t *testing.T) {
161161
controlPlaneEndpointSpec: v1alpha1.ControlPlaneEndpointSpec{
162162
Host: "192.168.1.2",
163163
},
164-
expectedErr: fmt.Errorf(
165-
"error parsing Prism Central URL: parse %q: invalid URI for request",
166-
"invalid-url",
167-
),
164+
expectedErr: nil,
168165
},
169166
{
170167
name: "Prism Central URL is FQDN",
@@ -191,7 +188,7 @@ func TestCheckIfPrismCentralAndControlPlaneIPSame(t *testing.T) {
191188
},
192189
},
193190
expectedErr: fmt.Errorf(
194-
"prism central and control plane endpoint cannot have the same IP %q",
191+
"Prism Central and control plane endpoint cannot have the same IP %q",
195192
"192.168.1.1",
196193
),
197194
},
@@ -215,7 +212,7 @@ func TestCheckIfPrismCentralAndControlPlaneIPSame(t *testing.T) {
215212

216213
for _, tt := range tests {
217214
t.Run(tt.name, func(t *testing.T) {
218-
err := checkIfPrismCentralAndControlPlaneIPSame(
215+
err := validatePrismCentralIPDoesNotEqualControlPlaneIP(
219216
tt.pcEndpoint,
220217
tt.controlPlaneEndpointSpec,
221218
)

0 commit comments

Comments
 (0)