Skip to content

Commit 8338dc5

Browse files
author
Per Goncalves da Silva
committed
Add name validation for strategy deployment and webhook configurations
Signed-off-by: Per Goncalves da Silva <[email protected]>
1 parent 715461b commit 8338dc5

File tree

2 files changed

+141
-10
lines changed

2 files changed

+141
-10
lines changed

‎internal/operator-controller/rukpak/render/validators/validator.go

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@ import (
44
"cmp"
55
"errors"
66
"fmt"
7-
"k8s.io/apimachinery/pkg/util/validation"
87
"maps"
98
"slices"
109
"strings"
1110

1211
"k8s.io/apimachinery/pkg/util/sets"
12+
"k8s.io/apimachinery/pkg/util/validation"
1313

1414
"github.com/operator-framework/api/pkg/operators/v1alpha1"
1515

@@ -22,11 +22,13 @@ var RegistryV1BundleValidator = render.BundleValidator{
2222
// you bring the same changes over to that test. This helps ensure all validation rules are executed
2323
// while giving us the flexibility to test each validation function individually
2424
CheckDeploymentSpecUniqueness,
25+
CheckDeploymentNameIsDNS1123SubDomain,
2526
CheckCRDResourceUniqueness,
2627
CheckOwnedCRDExistence,
2728
CheckPackageNameNotEmpty,
2829
CheckWebhookDeploymentReferentialIntegrity,
2930
CheckWebhookNameUniqueness,
31+
CheckWebhookNameIsDNS1123SubDomain,
3032
CheckConversionWebhookCRDReferenceUniqueness,
3133
CheckConversionWebhooksReferenceOwnedCRDs,
3234
}
@@ -62,7 +64,7 @@ func CheckDeploymentNameIsDNS1123SubDomain(rv1 *render.RegistryV1) []error {
6264
}
6365
}
6466

65-
var errs []error
67+
errs := make([]error, 0, len(deploymentNameErrMap))
6668
for _, dep := range slices.Sorted(maps.Keys(deploymentNameErrMap)) {
6769
errs = append(errs, fmt.Errorf("invalid cluster service version strategy deployment name '%s': %s", dep, strings.Join(deploymentNameErrMap[dep], ", ")))
6870
}
@@ -158,7 +160,7 @@ func CheckWebhookDeploymentReferentialIntegrity(rv1 *render.RegistryV1) []error
158160
return cmp.Or(cmp.Compare(a.Type, b.Type), cmp.Compare(a.GenerateName, b.GenerateName))
159161
})
160162
for _, webhookDef := range webhookDefns {
161-
errs = append(errs, fmt.Errorf("webhook '%s' of type '%s' references non-existent deployment '%s'", webhookDef.GenerateName, webhookDef.Type, webhookDef.DeploymentName))
163+
errs = append(errs, fmt.Errorf("webhook of type '%s' with name '%s' references non-existent deployment '%s'", webhookDef.Type, webhookDef.GenerateName, webhookDef.DeploymentName))
162164
}
163165
}
164166
return errs
@@ -256,3 +258,26 @@ func CheckConversionWebhookCRDReferenceUniqueness(rv1 *render.RegistryV1) []erro
256258
}
257259
return errs
258260
}
261+
262+
// CheckWebhookNameIsDNS1123SubDomain checks each webhook configuration name complies with the Kubernetes resource naming conversions
263+
func CheckWebhookNameIsDNS1123SubDomain(rv1 *render.RegistryV1) []error {
264+
invalidWebhooksByType := map[v1alpha1.WebhookAdmissionType]map[string][]string{}
265+
for _, wh := range rv1.CSV.Spec.WebhookDefinitions {
266+
if _, ok := invalidWebhooksByType[wh.Type]; !ok {
267+
invalidWebhooksByType[wh.Type] = map[string][]string{}
268+
}
269+
errs := validation.IsDNS1123Subdomain(wh.GenerateName)
270+
if len(errs) > 0 {
271+
slices.Sort(errs)
272+
invalidWebhooksByType[wh.Type][wh.GenerateName] = errs
273+
}
274+
}
275+
276+
var errs []error
277+
for _, whType := range slices.Sorted(maps.Keys(invalidWebhooksByType)) {
278+
for _, webhookName := range slices.Sorted(maps.Keys(invalidWebhooksByType[whType])) {
279+
errs = append(errs, fmt.Errorf("webhook of type '%s' has invalid name '%s': %s", whType, webhookName, strings.Join(invalidWebhooksByType[whType][webhookName], ",")))
280+
}
281+
}
282+
return errs
283+
}

‎internal/operator-controller/rukpak/render/validators/validator_test.go

Lines changed: 113 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,13 @@ import (
1919
func Test_BundleValidatorHasAllValidationFns(t *testing.T) {
2020
expectedValidationFns := []func(v1 *render.RegistryV1) []error{
2121
validators.CheckDeploymentSpecUniqueness,
22+
validators.CheckDeploymentNameIsDNS1123SubDomain,
2223
validators.CheckCRDResourceUniqueness,
2324
validators.CheckOwnedCRDExistence,
2425
validators.CheckPackageNameNotEmpty,
2526
validators.CheckWebhookDeploymentReferentialIntegrity,
2627
validators.CheckWebhookNameUniqueness,
28+
validators.CheckWebhookNameIsDNS1123SubDomain,
2729
validators.CheckConversionWebhookCRDReferenceUniqueness,
2830
validators.CheckConversionWebhooksReferenceOwnedCRDs,
2931
}
@@ -92,6 +94,47 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) {
9294
}
9395
}
9496

97+
func Test_CheckDeploymentNameIsDNS1123SubDomain(t *testing.T) {
98+
for _, tc := range []struct {
99+
name string
100+
bundle *render.RegistryV1
101+
expectedErrs []error
102+
}{
103+
{
104+
name: "accepts valid deployment strategy spec names",
105+
bundle: &render.RegistryV1{
106+
CSV: MakeCSV(
107+
WithStrategyDeploymentSpecs(
108+
v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"},
109+
v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"},
110+
),
111+
),
112+
},
113+
}, {
114+
name: "rejects bundles with invalid deployment strategy spec names - errors are sorted by name",
115+
bundle: &render.RegistryV1{
116+
CSV: MakeCSV(
117+
WithStrategyDeploymentSpecs(
118+
v1alpha1.StrategyDeploymentSpec{Name: "-bad-name"},
119+
v1alpha1.StrategyDeploymentSpec{Name: "b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long"},
120+
v1alpha1.StrategyDeploymentSpec{Name: "a-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-"},
121+
),
122+
),
123+
},
124+
expectedErrs: []error{
125+
errors.New("invalid cluster service version strategy deployment name '-bad-name': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
126+
errors.New("invalid cluster service version strategy deployment name 'a-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'), must be no more than 253 characters"),
127+
errors.New("invalid cluster service version strategy deployment name 'b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long': must be no more than 253 characters"),
128+
},
129+
},
130+
} {
131+
t.Run(tc.name, func(t *testing.T) {
132+
errs := validators.CheckDeploymentNameIsDNS1123SubDomain(tc.bundle)
133+
require.Equal(t, tc.expectedErrs, errs)
134+
})
135+
}
136+
}
137+
95138
func Test_CRDResourceUniqueness(t *testing.T) {
96139
for _, tc := range []struct {
97140
name string
@@ -356,7 +399,7 @@ func Test_CheckWebhookDeploymentReferentialIntegrity(t *testing.T) {
356399
),
357400
},
358401
expectedErrs: []error{
359-
errors.New("webhook 'test-webhook' of type 'ValidatingAdmissionWebhook' references non-existent deployment 'test-deployment-two'"),
402+
errors.New("webhook of type 'ValidatingAdmissionWebhook' with name 'test-webhook' references non-existent deployment 'test-deployment-two'"),
360403
},
361404
}, {
362405
name: "errors are ordered by deployment strategy spec name, webhook type, and webhook name",
@@ -398,12 +441,12 @@ func Test_CheckWebhookDeploymentReferentialIntegrity(t *testing.T) {
398441
),
399442
},
400443
expectedErrs: []error{
401-
errors.New("webhook 'test-mute-webhook-a' of type 'MutatingAdmissionWebhook' references non-existent deployment 'test-deployment-a'"),
402-
errors.New("webhook 'test-conv-webhook-b' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-b'"),
403-
errors.New("webhook 'test-conv-webhook-c-a' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-c'"),
404-
errors.New("webhook 'test-conv-webhook-c-b' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-c'"),
405-
errors.New("webhook 'test-mute-webhook-c' of type 'MutatingAdmissionWebhook' references non-existent deployment 'test-deployment-c'"),
406-
errors.New("webhook 'test-val-webhook-c' of type 'ValidatingAdmissionWebhook' references non-existent deployment 'test-deployment-c'"),
444+
errors.New("webhook of type 'MutatingAdmissionWebhook' with name 'test-mute-webhook-a' references non-existent deployment 'test-deployment-a'"),
445+
errors.New("webhook of type 'ConversionWebhook' with name 'test-conv-webhook-b' references non-existent deployment 'test-deployment-b'"),
446+
errors.New("webhook of type 'ConversionWebhook' with name 'test-conv-webhook-c-a' references non-existent deployment 'test-deployment-c'"),
447+
errors.New("webhook of type 'ConversionWebhook' with name 'test-conv-webhook-c-b' references non-existent deployment 'test-deployment-c'"),
448+
errors.New("webhook of type 'MutatingAdmissionWebhook' with name 'test-mute-webhook-c' references non-existent deployment 'test-deployment-c'"),
449+
errors.New("webhook of type 'ValidatingAdmissionWebhook' with name 'test-val-webhook-c' references non-existent deployment 'test-deployment-c'"),
407450
},
408451
},
409452
} {
@@ -837,3 +880,66 @@ func Test_CheckConversionWebhookCRDReferenceUniqueness(t *testing.T) {
837880
})
838881
}
839882
}
883+
884+
func Test_CheckWebhookNameIsDNS1123SubDomain(t *testing.T) {
885+
for _, tc := range []struct {
886+
name string
887+
bundle *render.RegistryV1
888+
expectedErrs []error
889+
}{
890+
{
891+
name: "accepts bundles without webhook definitions",
892+
bundle: &render.RegistryV1{
893+
CSV: MakeCSV(),
894+
},
895+
}, {
896+
name: "rejects bundles with invalid webhook definitions names and orders errors by webhook type and name",
897+
bundle: &render.RegistryV1{
898+
CSV: MakeCSV(
899+
WithWebhookDefinitions(
900+
v1alpha1.WebhookDescription{
901+
Type: v1alpha1.ValidatingAdmissionWebhook,
902+
GenerateName: "b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-",
903+
}, v1alpha1.WebhookDescription{
904+
Type: v1alpha1.ValidatingAdmissionWebhook,
905+
GenerateName: "a-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long",
906+
},
907+
v1alpha1.WebhookDescription{
908+
Type: v1alpha1.ValidatingAdmissionWebhook,
909+
GenerateName: "-bad-name",
910+
},
911+
v1alpha1.WebhookDescription{
912+
Type: v1alpha1.ConversionWebhook,
913+
GenerateName: "b-bad-name-",
914+
},
915+
v1alpha1.WebhookDescription{
916+
Type: v1alpha1.MutatingAdmissionWebhook,
917+
GenerateName: "b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-",
918+
}, v1alpha1.WebhookDescription{
919+
Type: v1alpha1.MutatingAdmissionWebhook,
920+
GenerateName: "a-bad-name-",
921+
},
922+
v1alpha1.WebhookDescription{
923+
Type: v1alpha1.ConversionWebhook,
924+
GenerateName: "a-bad-name-",
925+
},
926+
),
927+
),
928+
},
929+
expectedErrs: []error{
930+
errors.New("webhook of type 'ConversionWebhook' has invalid name 'a-bad-name-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
931+
errors.New("webhook of type 'ConversionWebhook' has invalid name 'b-bad-name-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
932+
errors.New("webhook of type 'MutatingAdmissionWebhook' has invalid name 'a-bad-name-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
933+
errors.New("webhook of type 'MutatingAdmissionWebhook' has invalid name 'b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'),must be no more than 253 characters"),
934+
errors.New("webhook of type 'ValidatingAdmissionWebhook' has invalid name '-bad-name': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"),
935+
errors.New("webhook of type 'ValidatingAdmissionWebhook' has invalid name 'a-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long': must be no more than 253 characters"),
936+
errors.New("webhook of type 'ValidatingAdmissionWebhook' has invalid name 'b-name-is-waaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaay-too-long-and-bad-': a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'),must be no more than 253 characters"),
937+
},
938+
},
939+
} {
940+
t.Run(tc.name, func(t *testing.T) {
941+
errs := validators.CheckWebhookNameIsDNS1123SubDomain(tc.bundle)
942+
require.Equal(t, tc.expectedErrs, errs)
943+
})
944+
}
945+
}

0 commit comments

Comments
 (0)