Skip to content

Commit c997c0f

Browse files
dlipovetskydlipovetsky
authored
fix: Nuanced image Kubernetes version check errors (#1211)
- Return an internal error only when parsing fails. - Add more context to messages. **What problem does this PR solve?**: **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. -->
1 parent eacc96b commit c997c0f

File tree

2 files changed

+66
-46
lines changed

2 files changed

+66
-46
lines changed

‎pkg/webhook/preflight/nutanix/imagekubernetesversioncheck.go

Lines changed: 59 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"strings"
1010

1111
"github.com/blang/semver/v4"
12-
vmmv4 "github.com/nutanix/ntnx-api-golang-clients/vmm-go-client/v4/models/vmm/v4/content"
1312

1413
carenv1 "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
1514
"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/webhook/preflight"
@@ -29,8 +28,13 @@ func (c *imageKubernetesVersionCheck) Name() string {
2928
func (c *imageKubernetesVersionCheck) Run(ctx context.Context) preflight.CheckResult {
3029
if c.machineDetails.ImageLookup != nil {
3130
return preflight.CheckResult{
32-
Allowed: true,
33-
Warnings: []string{fmt.Sprintf("%s uses imageLookup, which is not yet supported by checks", c.field)},
31+
Allowed: true,
32+
Warnings: []string{
33+
fmt.Sprintf(
34+
"%s uses imageLookup, which is not yet supported by checks",
35+
c.field,
36+
),
37+
},
3438
}
3539
}
3640

@@ -42,8 +46,12 @@ func (c *imageKubernetesVersionCheck) Run(ctx context.Context) preflight.CheckRe
4246
InternalError: true,
4347
Causes: []preflight.Cause{
4448
{
45-
Message: fmt.Sprintf("Failed to get VM Image: %s", err),
46-
Field: c.field + ".image",
49+
Message: fmt.Sprintf(
50+
"Failed to get VM Image %q: %s. This is usually a temporary error. Please retry.",
51+
c.machineDetails.Image,
52+
err,
53+
),
54+
Field: c.field + ".image",
4755
},
4856
},
4957
}
@@ -54,51 +62,66 @@ func (c *imageKubernetesVersionCheck) Run(ctx context.Context) preflight.CheckRe
5462
Allowed: true,
5563
}
5664
}
65+
image := images[0]
5766

58-
if err := c.checkKubernetesVersion(&images[0]); err != nil {
67+
if image.Name == nil || *image.Name == "" {
5968
return preflight.CheckResult{
6069
Allowed: false,
6170
InternalError: false,
6271
Causes: []preflight.Cause{
6372
{
64-
Message: "Kubernetes version check failed: " + err.Error(),
65-
Field: c.field + ".image",
73+
Message: fmt.Sprintf(
74+
"The VM Image identified by %q has no name. Give the VM Image a name, or use a different VM Image. Make sure the VM Image contains the Kubernetes version supported by the VM Image. Choose a VM Image that supports the cluster Kubernetes version: %q", //nolint:lll // The message is long.
75+
*c.machineDetails.Image,
76+
c.clusterK8sVersion,
77+
),
78+
Field: c.field + ".image",
6679
},
6780
},
6881
}
6982
}
70-
}
71-
72-
return preflight.CheckResult{Allowed: true}
73-
}
74-
75-
func (c *imageKubernetesVersionCheck) checkKubernetesVersion(image *vmmv4.Image) error {
76-
imageName := ""
77-
if image.Name != nil {
78-
imageName = *image.Name
79-
}
80-
81-
if imageName == "" {
82-
return fmt.Errorf("VM image name is empty")
83-
}
84-
85-
parsedVersion, err := semver.Parse(c.clusterK8sVersion)
86-
if err != nil {
87-
return fmt.Errorf("failed to parse kubernetes version %q: %v", c.clusterK8sVersion, err)
88-
}
8983

90-
// For example, "1.33.1+fips.0" becomes "1.33.1".
91-
k8sVersion := parsedVersion.FinalizeVersion()
84+
// Uses the same function that is used by the Cluster API topology validation webhook.
85+
parsedClusterK8sVersion, err := semver.ParseTolerant(c.clusterK8sVersion)
86+
if err != nil {
87+
return preflight.CheckResult{
88+
Allowed: false,
89+
// The Cluster API topology validation webhook should prevent this from happening,
90+
// so if it does, treat it as an internal error.
91+
InternalError: true,
92+
Causes: []preflight.Cause{
93+
{
94+
Message: fmt.Sprintf(
95+
"The Cluster Kubernetes version %q is not a valid semantic version. This error should not happen under normal circumstances. Please report.", //nolint:lll // The message is long.
96+
c.clusterK8sVersion,
97+
),
98+
Field: c.field + ".image",
99+
},
100+
},
101+
}
102+
}
92103

93-
if !strings.Contains(imageName, k8sVersion) {
94-
return fmt.Errorf(
95-
"kubernetes version %q is not part of image name %q",
96-
c.clusterK8sVersion,
97-
imageName,
98-
)
104+
finalizedClusterK8sVersion := parsedClusterK8sVersion.FinalizeVersion()
105+
if !strings.Contains(*image.Name, finalizedClusterK8sVersion) {
106+
return preflight.CheckResult{
107+
Allowed: false,
108+
InternalError: false,
109+
Causes: []preflight.Cause{
110+
{
111+
Message: fmt.Sprintf(
112+
"The VM Image identified by %q has the name %q. Make sure the VM Image name contains the Kubernetes version supported by the VM Image. Choose a VM Image that supports the cluster Kubernetes version: %q.", //nolint:lll // The message is long.
113+
*c.machineDetails.Image,
114+
*image.Name,
115+
finalizedClusterK8sVersion,
116+
),
117+
Field: c.field + ".image",
118+
},
119+
},
120+
}
121+
}
99122
}
100123

101-
return nil
124+
return preflight.CheckResult{Allowed: true}
102125
}
103126

104127
func newVMImageKubernetesVersionChecks(

‎pkg/webhook/preflight/nutanix/imagekubernetesversioncheck_test.go

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,7 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
7979
InternalError: false,
8080
Causes: []preflight.Cause{
8181
{
82-
///nolint:lll // The message is long.
83-
Message: "Kubernetes version check failed: kubernetes version \"1.32.3\" is not part of image name \"kubedistro-ubuntu-22.04-vgpu-1.31.5-20250604180644\"",
82+
Message: "The VM Image identified by \"test-uuid\" has the name \"kubedistro-ubuntu-22.04-vgpu-1.31.5-20250604180644\". Make sure the VM Image name contains the Kubernetes version supported by the VM Image. Choose a VM Image that supports the cluster Kubernetes version: \"1.32.3\".", ///nolint:lll // The message is long.
8483
Field: "machineDetails.image",
8584
},
8685
},
@@ -112,7 +111,7 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
112111
},
113112
},
114113
{
115-
name: "custom image name - extraction fails",
114+
name: "custom image name has no Kubernetes version",
116115
nclient: &mocknclient{
117116
getImageByIdFunc: func(uuid *string) (*vmmv4.GetImageApiResponse, error) {
118117
resp := &vmmv4.GetImageApiResponse{}
@@ -137,8 +136,7 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
137136
InternalError: false,
138137
Causes: []preflight.Cause{
139138
{
140-
///nolint:lll // The message is long.
141-
Message: "Kubernetes version check failed: kubernetes version \"1.32.3\" is not part of image name \"my-custom-image-name\"",
139+
Message: "The VM Image identified by \"test-uuid\" has the name \"my-custom-image-name\". Make sure the VM Image name contains the Kubernetes version supported by the VM Image. Choose a VM Image that supports the cluster Kubernetes version: \"1.32.3\".", ///nolint:lll // The message is long.
142140
Field: "machineDetails.image",
143141
},
144142
},
@@ -167,11 +165,10 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
167165
clusterK8sVersion: "invalid.version",
168166
want: preflight.CheckResult{
169167
Allowed: false,
170-
InternalError: false,
168+
InternalError: true,
171169
Causes: []preflight.Cause{
172170
{
173-
//nolint:lll // The message is long.
174-
Message: "Kubernetes version check failed: failed to parse kubernetes version \"invalid.version\": No Major.Minor.Patch elements found",
171+
Message: "The Cluster Kubernetes version \"invalid.version\" is not a valid semantic version. This error should not happen under normal circumstances. Please report.", //nolint:lll // The message is long.
175172
Field: "machineDetails.image",
176173
},
177174
},
@@ -203,7 +200,7 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
203200
InternalError: false,
204201
Causes: []preflight.Cause{
205202
{
206-
Message: "Kubernetes version check failed: VM image name is empty",
203+
Message: "The VM Image identified by \"test-uuid\" has no name. Give the VM Image a name, or use a different VM Image. Make sure the VM Image contains the Kubernetes version supported by the VM Image. Choose a VM Image that supports the cluster Kubernetes version: \"1.32.3\"", //nolint:lll // The message is long.
207204
Field: "machineDetails.image",
208205
},
209206
},
@@ -260,7 +257,7 @@ func TestVMImageCheckWithKubernetesVersion(t *testing.T) {
260257
InternalError: true,
261258
Causes: []preflight.Cause{
262259
{
263-
Message: "Failed to get VM Image: some error",
260+
Message: "Failed to get VM Image \"test-uuid\": some error. This is usually a temporary error. Please retry.", //nolint:lll // The message is long.
264261
Field: "machineDetails.image",
265262
},
266263
},

0 commit comments

Comments
 (0)