Skip to content

Commit b24b411

Browse files
dlipovetskydlipovetsky
authored
fix: Do not treat expected preflight check failures as internal errors (#1195)
**What problem does this PR solve?**: Internal errors are meant to signal some unexpected failure. We have been treating a number of expected failures as internal errors, and this PR fixes these. It also makes CheckResult interface easier to understand by renaming the Error field to InternalError. **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 3ff7fbc commit b24b411

16 files changed

+383
-173
lines changed

‎pkg/webhook/preflight/generic/registry.go

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/regclient/regclient/types/ping"
1515
"github.com/regclient/regclient/types/ref"
1616
corev1 "k8s.io/api/core/v1"
17+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1718
"k8s.io/apimachinery/pkg/types"
1819
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
1920
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -76,7 +77,7 @@ func (r *registryCheck) checkRegistry(
7677
registryURLParsed, err := url.ParseRequestURI(registryURL)
7778
if err != nil {
7879
result.Allowed = false
79-
result.Error = true
80+
result.InternalError = false
8081
result.Causes = append(result.Causes,
8182
preflight.Cause{
8283
Message: fmt.Sprintf("failed to parse registry url %s with error: %s", registryURL, err),
@@ -98,9 +99,20 @@ func (r *registryCheck) checkRegistry(
9899
},
99100
mirrorCredentialsSecret,
100101
)
102+
if apierrors.IsNotFound(err) {
103+
result.Allowed = false
104+
result.InternalError = false
105+
result.Causes = append(result.Causes,
106+
preflight.Cause{
107+
Message: fmt.Sprintf("Registry credentials Secret %q not found", credentials.SecretRef.Name),
108+
Field: r.field + ".credentials.secretRef",
109+
},
110+
)
111+
return result
112+
}
101113
if err != nil {
102114
result.Allowed = false
103-
result.Error = true
115+
result.InternalError = true
104116
result.Causes = append(result.Causes,
105117
preflight.Cause{
106118
Message: fmt.Sprintf("failed to get Registry credentials Secret: %s", err),
@@ -125,19 +137,15 @@ func (r *registryCheck) checkRegistry(
125137
regclient.WithConfigHost(mirrorHost),
126138
regclient.WithUserAgent("regclient/example"),
127139
)
128-
mirrorRef, err := ref.NewHost(registryURLParsed.Host)
129-
if err != nil {
130-
result.Allowed = false
131-
result.Error = true
132-
result.Causes = append(result.Causes,
133-
preflight.Cause{
134-
Message: fmt.Sprintf("failed to create a client to verify registry configuration %s", err.Error()),
135-
Field: r.field,
136-
},
137-
)
138-
return result
139-
}
140-
_, err = rc.Ping(ctx, mirrorRef) // ping will return an error for anything that's not 200
140+
_, err = rc.Ping(ctx,
141+
ref.Ref{
142+
// Because we ping the registry, we only need the "registry" part of the ref.
143+
Registry: registryURLParsed.Host,
144+
// The default scheme is "reg""
145+
Scheme: "reg",
146+
},
147+
)
148+
// Ping will return an error for anything that's not 200.
141149
if err != nil {
142150
result.Allowed = false
143151
result.Causes = append(result.Causes,
@@ -149,7 +157,7 @@ func (r *registryCheck) checkRegistry(
149157
return result
150158
}
151159
result.Allowed = true
152-
result.Error = false
160+
result.InternalError = false
153161
return result
154162
}
155163

‎pkg/webhook/preflight/generic/registry_test.go

Lines changed: 81 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/stretchr/testify/assert"
1616
"github.com/stretchr/testify/require"
1717
corev1 "k8s.io/api/core/v1"
18+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1819
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1920
"k8s.io/apimachinery/pkg/types"
2021
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -126,15 +127,47 @@ func TestRegistryCheck(t *testing.T) {
126127
obj ctrlclient.Object,
127128
opts ...ctrlclient.GetOption,
128129
) error {
129-
return fmt.Errorf("secret not found")
130+
return fmt.Errorf("fake error")
130131
},
131132
},
132133
want: preflight.CheckResult{
133-
Allowed: false,
134-
Error: true,
134+
Allowed: false,
135+
InternalError: true,
136+
Causes: []preflight.Cause{
137+
{
138+
Message: "failed to get Registry credentials Secret: fake error",
139+
//nolint:lll // this is a test for a field.
140+
Field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",
141+
},
142+
},
143+
},
144+
},
145+
{
146+
name: "registry mirror with missing credentials secret",
147+
field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror",
148+
registryMirror: &carenv1.GlobalImageRegistryMirror{
149+
URL: testRegistryURL,
150+
Credentials: &carenv1.RegistryCredentials{
151+
SecretRef: &carenv1.LocalObjectReference{
152+
Name: "test-secret",
153+
},
154+
},
155+
},
156+
kclient: &mockK8sClient{
157+
getSecretFunc: func(ctx context.Context,
158+
key types.NamespacedName,
159+
obj ctrlclient.Object,
160+
opts ...ctrlclient.GetOption,
161+
) error {
162+
return apierrors.NewNotFound(corev1.Resource("secrets"), "test-secret")
163+
},
164+
},
165+
want: preflight.CheckResult{
166+
Allowed: false,
167+
InternalError: false,
135168
Causes: []preflight.Cause{
136169
{
137-
Message: "failed to get Registry credentials Secret: secret not found",
170+
Message: "Registry credentials Secret \"test-secret\" not found",
138171
//nolint:lll // this is a test for a field.
139172
Field: "cluster.spec.topology.variables[.name=clusterConfig].value.globalImageRegistryMirror.credentials.secretRef",
140173
},
@@ -199,6 +232,49 @@ func TestRegistryCheck(t *testing.T) {
199232
Allowed: true,
200233
},
201234
},
235+
{
236+
name: "image registry with invalid URL",
237+
field: "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0]",
238+
imageRegistry: &carenv1.ImageRegistry{
239+
URL: "invalid-url",
240+
Credentials: &carenv1.RegistryCredentials{
241+
SecretRef: &carenv1.LocalObjectReference{
242+
Name: "test-secret",
243+
},
244+
},
245+
},
246+
kclient: &mockK8sClient{
247+
getSecretFunc: func(ctx context.Context,
248+
key types.NamespacedName,
249+
obj ctrlclient.Object,
250+
opts ...ctrlclient.GetOption,
251+
) error {
252+
secret := obj.(*corev1.Secret)
253+
secret.Data = map[string][]byte{
254+
"username": []byte("testuser"),
255+
"password": []byte("testpass"),
256+
"ca.crt": []byte("test-ca-cert"),
257+
}
258+
return nil
259+
},
260+
},
261+
mockRegClientPingerFactory: func(...regclient.Opt) regClientPinger {
262+
return &mockRegClient{
263+
pingFunc: func(ref.Ref) error { return nil },
264+
}
265+
},
266+
want: preflight.CheckResult{
267+
Allowed: false,
268+
InternalError: false,
269+
Causes: []preflight.Cause{
270+
{
271+
Message: fmt.Sprintf("failed to parse registry url %s with error: "+
272+
"parse \"invalid-url\": invalid URI for request", "invalid-url"),
273+
Field: "cluster.spec.topology.variables[.name=clusterConfig].value.imageRegistries[0].url",
274+
},
275+
},
276+
},
277+
},
202278
}
203279

204280
for _, tc := range testCases {
@@ -235,7 +311,7 @@ func TestRegistryCheck(t *testing.T) {
235311

236312
// Verify the result
237313
assert.Equal(t, tc.want.Allowed, got.Allowed, "(allowed) mismatch for test "+tc.name)
238-
assert.Equal(t, tc.want.Error, got.Error, "(error) mismatch test "+tc.name)
314+
assert.Equal(t, tc.want.InternalError, got.InternalError, "(error) mismatch test "+tc.name)
239315
assert.Equal(t, tc.want.Causes, got.Causes, "(causes) mismatch test "+tc.name)
240316
})
241317
}

‎pkg/webhook/preflight/generic/spec.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ func newConfigurationCheck(
4242
)
4343
if err != nil {
4444
configurationCheck.result.Allowed = false
45-
configurationCheck.result.Error = true
45+
configurationCheck.result.InternalError = true
4646
configurationCheck.result.Causes = append(configurationCheck.result.Causes,
4747
preflight.Cause{
4848
Message: fmt.Sprintf("Failed to unmarshal cluster variable %s: %s",

‎pkg/webhook/preflight/generic/spec_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,8 @@ func TestNewConfigurationCheck(t *testing.T) {
102102
},
103103
},
104104
expectedResult: preflight.CheckResult{
105-
Allowed: false,
106-
Error: true,
105+
Allowed: false,
106+
InternalError: true,
107107
Causes: []preflight.Cause{
108108
{
109109
Message: "Failed to unmarshal cluster variable clusterConfig: failed to unmarshal json:" +

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"fmt"
99

1010
corev1 "k8s.io/api/core/v1"
11+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1112
"k8s.io/apimachinery/pkg/types"
1213

1314
prismgoclient "github.com/nutanix-cloud-native/prism-go-client"
@@ -52,7 +53,6 @@ func newCredentialsCheck(
5253
// However, the credentials configuration is missing, so we cannot perform the check.
5354
if cd.nutanixClusterConfigSpec == nil || cd.nutanixClusterConfigSpec.Nutanix == nil {
5455
credentialsCheck.result.Allowed = false
55-
credentialsCheck.result.Error = true
5656
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
5757
preflight.Cause{
5858
Message: "Nutanix cluster configuration is not defined in the cluster spec",
@@ -69,7 +69,6 @@ func newCredentialsCheck(
6969
if err != nil {
7070
// Should not happen if the cluster passed CEL validation rules.
7171
credentialsCheck.result.Allowed = false
72-
credentialsCheck.result.Error = true
7372
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
7473
preflight.Cause{
7574
Message: fmt.Sprintf("failed to parse Prism Central endpoint URL: %s", err),
@@ -88,25 +87,39 @@ func newCredentialsCheck(
8887
},
8988
credentialsSecret,
9089
)
90+
if apierrors.IsNotFound(err) {
91+
credentialsCheck.result.Allowed = false
92+
credentialsCheck.result.InternalError = false
93+
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
94+
preflight.Cause{
95+
Message: fmt.Sprintf("Prism Central credentials Secret %q not found",
96+
prismCentralEndpointSpec.Credentials.SecretRef.Name),
97+
Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
98+
},
99+
)
100+
return credentialsCheck
101+
}
91102
if err != nil {
92103
credentialsCheck.result.Allowed = false
93-
credentialsCheck.result.Error = true
104+
credentialsCheck.result.InternalError = true
94105
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
95106
preflight.Cause{
96-
Message: fmt.Sprintf("failed to get Prism Central credentials Secret: %s", err),
97-
Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
107+
Message: fmt.Sprintf("Failed to get Prism Central credentials Secret %q: %s",
108+
prismCentralEndpointSpec.Credentials.SecretRef.Name,
109+
err,
110+
),
111+
Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
98112
},
99113
)
100114
return credentialsCheck
101115
}
102116

103117
if len(credentialsSecret.Data) == 0 {
104118
credentialsCheck.result.Allowed = false
105-
credentialsCheck.result.Error = true
106119
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
107120
preflight.Cause{
108121
Message: fmt.Sprintf(
109-
"credentials Secret '%s' is empty",
122+
"credentials Secret %q is empty",
110123
prismCentralEndpointSpec.Credentials.SecretRef.Name,
111124
),
112125
Field: "cluster.spec.topology.variables[.name=clusterConfig].nutanix.prismCentralEndpoint.credentials.secretRef",
@@ -118,11 +131,10 @@ func newCredentialsCheck(
118131
data, ok := credentialsSecret.Data[credentialsSecretDataKey]
119132
if !ok {
120133
credentialsCheck.result.Allowed = false
121-
credentialsCheck.result.Error = true
122134
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
123135
preflight.Cause{
124136
Message: fmt.Sprintf(
125-
"credentials Secret '%s' does not contain key '%s'",
137+
"credentials Secret %q does not contain key %q",
126138
prismCentralEndpointSpec.Credentials.SecretRef.Name,
127139
credentialsSecretDataKey,
128140
),
@@ -135,7 +147,6 @@ func newCredentialsCheck(
135147
usernamePassword, err := prismcredentials.ParseCredentials(data)
136148
if err != nil {
137149
credentialsCheck.result.Allowed = false
138-
credentialsCheck.result.Error = true
139150
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
140151
preflight.Cause{
141152
Message: fmt.Sprintf("failed to parse Prism Central credentials: %s", err),
@@ -158,7 +169,7 @@ func newCredentialsCheck(
158169
nclient, err := nclientFactory(credentials)
159170
if err != nil {
160171
credentialsCheck.result.Allowed = false
161-
credentialsCheck.result.Error = true
172+
credentialsCheck.result.InternalError = true
162173
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
163174
preflight.Cause{
164175
Message: fmt.Sprintf("Failed to initialize Nutanix client: %s", err),
@@ -172,7 +183,7 @@ func newCredentialsCheck(
172183
_, err = nclient.GetCurrentLoggedInUser(ctx)
173184
if err != nil {
174185
credentialsCheck.result.Allowed = false
175-
credentialsCheck.result.Error = true
186+
credentialsCheck.result.InternalError = true
176187
credentialsCheck.result.Causes = append(credentialsCheck.result.Causes,
177188
preflight.Cause{
178189
Message: fmt.Sprintf("Failed to validate credentials using the v3 API client. "+

0 commit comments

Comments
 (0)