From 1e671c596daddee34cf4b81beebabbb39be5725d Mon Sep 17 00:00:00 2001 From: grokspawn Date: Fri, 21 Nov 2025 10:59:00 -0600 Subject: [PATCH 1/2] new tag symmetry and required validations Signed-off-by: grokspawn --- hack/tools/crd-generator/README.md | 20 +++ hack/tools/crd-generator/main.go | 98 ++++++++--- hack/tools/crd-generator/main_test.go | 243 ++++++++++++++++++++++++++ 3 files changed, 337 insertions(+), 24 deletions(-) diff --git a/hack/tools/crd-generator/README.md b/hack/tools/crd-generator/README.md index 4334721672..83fb63e21c 100644 --- a/hack/tools/crd-generator/README.md +++ b/hack/tools/crd-generator/README.md @@ -33,6 +33,14 @@ A semi-colon separated list of enumerations, similar to the `+kubebuilder:valida An XValidation scheme, similar to the `+kubebuilder:validation:XValidation` scheme, but more limited. +* `Optional` + +Indicating that this field should not be listed as required in its parent. + +* `Required` + +Indicating that this field should be listed as required in its parent. + ## Experimental Description * Start Tag: `` @@ -44,6 +52,18 @@ All text between the tags is included in the experimental CRD, but removed from This is only useful if the field is included in the standard CRD, but there's additional meaning in the experimental CRD when feature gates are enabled. +## Standard Description + +* Start Tag: `` +* End Tag: `` + +Descriptive text that is only included as part of the field description within the standard CRD. +All text between the tags is included in the standard CRD, but removed from the experimental CRD. + +This is useful if the field is included in the standard CRD and has differing meaning than when the +field is used in the experimental CRD when feature gates are enabled. + + ## Exclude from CRD Description * Start Tag: `` diff --git a/hack/tools/crd-generator/main.go b/hack/tools/crd-generator/main.go index 9687489f45..f5d10206c3 100644 --- a/hack/tools/crd-generator/main.go +++ b/hack/tools/crd-generator/main.go @@ -23,6 +23,7 @@ import ( "log" "os" "regexp" + "slices" "strings" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -136,7 +137,7 @@ func runGenerator(args ...string) { if channel == StandardChannel && strings.Contains(version.Name, "alpha") { channelCrd.Spec.Versions[i].Served = false } - version.Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties) + version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required) } conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) @@ -179,25 +180,51 @@ func runGenerator(args ...string) { } } -func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { +// Apply Opcon specific tweaks to all properties in a map, and return a list of required fields according to opcon tags. +// For opcon validation optional/required tags, the required list is updated accordingly. +func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps, existingRequired []string) (map[string]apiextensionsv1.JSONSchemaProps, []string) { + newRequired := slices.Clone(existingRequired) + for name := range props { jsonProps := props[name] - p := opconTweaks(channel, name, jsonProps) + p, reqStatus := opconTweaks(channel, name, jsonProps) if p == nil { delete(props, name) } else { props[name] = *p + // Update required list based on tag + switch reqStatus { + case statusRequired: + if !slices.Contains(newRequired, name) { + newRequired = append(newRequired, name) + } + case statusOptional: + newRequired = slices.DeleteFunc(newRequired, func(s string) bool { return s == name }) + default: + // "" (unspecified) means keep existing status + } } } - return props + return props, newRequired } // Custom Opcon API Tweaks for tags prefixed with `") { - return nil + return nil, statusNoOpinion } } @@ -219,7 +246,7 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche numValid++ jsonProps.Enum = []apiextensionsv1.JSON{} - for _, val := range strings.Split(enumMatch[1], ";") { + for val := range strings.SplitSeq(enumMatch[1], ";") { jsonProps.Enum = append(jsonProps.Enum, apiextensionsv1.JSON{Raw: []byte("\"" + val + "\"")}) } } @@ -237,6 +264,21 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche Rule: celMatch[2], }) } + optReqRe := regexp.MustCompile(validationPrefix + "(Optional|Required)>") + optReqMatches := optReqRe.FindAllStringSubmatch(jsonProps.Description, 64) + for _, optReqMatch := range optReqMatches { + if len(optReqMatch) != 2 { + log.Fatalf("Invalid %s Optional/Required tag for %s", validationPrefix, name) + } + + numValid++ + switch optReqMatch[1] { + case "Optional": + requiredStatus = statusOptional + case "Required": + requiredStatus = statusRequired + } + } } if numValid < numExpressions { @@ -246,34 +288,42 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche jsonProps.Description = formatDescription(jsonProps.Description, channel, name) if len(jsonProps.Properties) > 0 { - jsonProps.Properties = opconTweaksMap(channel, jsonProps.Properties) + jsonProps.Properties, jsonProps.Required = opconTweaksMap(channel, jsonProps.Properties, jsonProps.Required) } else if jsonProps.Items != nil && jsonProps.Items.Schema != nil { - jsonProps.Items.Schema = opconTweaks(channel, name, *jsonProps.Items.Schema) + jsonProps.Items.Schema, _ = opconTweaks(channel, name, *jsonProps.Items.Schema) } - return &jsonProps + return &jsonProps, requiredStatus } func formatDescription(description string, channel string, name string) string { - startTag := "" - endTag := "" - if channel == StandardChannel && strings.Contains(description, startTag) { - regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` - re := regexp.MustCompile(regexPattern) - match := re.FindStringSubmatch(description) - if len(match) != 2 { - log.Fatalf("Invalid tag for %s", name) + tagset := []struct { + channel string + start string + end string + }{ + {channel: ExperimentalChannel, start: "", end: ""}, + {channel: StandardChannel, start: "", end: ""}, + } + for _, ts := range tagset { + if channel == ts.channel && strings.Contains(description, ts.start) { + regexPattern := `\n*` + regexp.QuoteMeta(ts.start) + `(?s:(.*?))` + regexp.QuoteMeta(ts.end) + `\n*` + re := regexp.MustCompile(regexPattern) + match := re.FindStringSubmatch(description) + if len(match) != 2 { + log.Fatalf("Invalid tag for %s", name) + } + description = re.ReplaceAllString(description, "\n\n") + } else { + description = strings.ReplaceAll(description, ts.start, "") + description = strings.ReplaceAll(description, ts.end, "") } - description = re.ReplaceAllString(description, "\n\n") - } else { - description = strings.ReplaceAll(description, startTag, "") - description = strings.ReplaceAll(description, endTag, "") } // Comments within "opcon:util:excludeFromCRD" tag are not included in the generated CRD and all trailing \n operators before // and after the tags are removed and replaced with three \n operators. - startTag = "" - endTag = "" + startTag := "" + endTag := "" if strings.Contains(description, startTag) { regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` re := regexp.MustCompile(regexPattern) diff --git a/hack/tools/crd-generator/main_test.go b/hack/tools/crd-generator/main_test.go index d2eb28d61f..ecfc7d8a72 100644 --- a/hack/tools/crd-generator/main_test.go +++ b/hack/tools/crd-generator/main_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) const controllerToolsVersion = "v0.19.0" @@ -75,6 +76,248 @@ func TestTags(t *testing.T) { compareFiles(t, f1, f2) } +func TestFormatDescription(t *testing.T) { + tests := []struct { + name string + channel string + fieldName string + input string + expected string + }{ + { + name: "standard channel removes experimental description", + channel: StandardChannel, + fieldName: "testField", + input: "Base description.\n\nExperimental content.\n\nMore content.", + expected: "Base description.\n\nMore content.", + }, + { + name: "experimental channel removes standard description", + channel: ExperimentalChannel, + fieldName: "testField", + input: "Base description.\n\nStandard content.\n\nMore content.", + expected: "Base description.\n\nMore content.", + }, + { + name: "excludeFromCRD tag removes content", + channel: StandardChannel, + fieldName: "testField", + input: "Before.\n\n\nExcluded content.\n\n\nAfter.", + expected: "Before.\n\nAfter.", + }, + { + name: "three hyphens removes trailing content", + channel: StandardChannel, + fieldName: "testField", + input: "Visible content.\n---\nHidden content after separator.", + expected: "Visible content.", + }, + { + name: "multiple newlines collapsed to double", + channel: StandardChannel, + fieldName: "testField", + input: "Line one.\n\n\n\n\nLine two.", + expected: "Line one.\n\nLine two.", + }, + { + name: "trailing newlines removed", + channel: StandardChannel, + fieldName: "testField", + input: "Content with trailing newlines.\n\n\n", + expected: "Content with trailing newlines.", + }, + { + name: "combined tags and formatting", + channel: ExperimentalChannel, + fieldName: "testField", + input: "Main text.\n\nStandard only.\n\n\n\n\nInternal notes.\n\n\nFinal text.\n\n\n", + expected: "Main text.\n\nFinal text.", + }, + { + name: "empty input", + channel: StandardChannel, + fieldName: "testField", + input: "", + expected: "", + }, + { + name: "no tags plain text", + channel: StandardChannel, + fieldName: "testField", + input: "Simple description without any tags.", + expected: "Simple description without any tags.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := formatDescription(tt.input, tt.channel, tt.fieldName) + require.Equal(t, tt.expected, result) + }) + } +} + +// TestOpconTweaksOptionalRequired tests the opconTweaks function for handling +// optional and required tags in field descriptions. +func TestOpconTweaksOptionalRequired(t *testing.T) { + tests := []struct { + name string + channel string + fieldName string + description string + expectedStatus string + }{ + { + name: "optional tag in standard channel", + channel: StandardChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusOptional, + }, + { + name: "required tag in standard channel", + channel: StandardChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusRequired, + }, + { + name: "optional tag in experimental channel", + channel: ExperimentalChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusOptional, + }, + { + name: "required tag in experimental channel", + channel: ExperimentalChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusRequired, + }, + { + name: "no validation tag", + channel: StandardChannel, + fieldName: "testField", + description: "Field description without tags.", + expectedStatus: statusNoOpinion, + }, + { + name: "experimental tag in standard channel ignored", + channel: StandardChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusNoOpinion, + }, + { + name: "standard tag in experimental channel ignored", + channel: ExperimentalChannel, + fieldName: "testField", + description: "Field description.\n", + expectedStatus: statusNoOpinion, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + jsonProps := apiextensionsv1.JSONSchemaProps{ + Description: tt.description, + Type: "string", + } + _, status := opconTweaks(tt.channel, tt.fieldName, jsonProps) + require.Equal(t, tt.expectedStatus, status) + }) + } +} + +// TestOpconTweaksMapRequiredList tests the opconTweaksMap function for correctly +// updating the required list based on field descriptions. +func TestOpconTweaksMapRequiredList(t *testing.T) { + tests := []struct { + name string + channel string + props map[string]apiextensionsv1.JSONSchemaProps + existingRequired []string + expectedRequired []string + }{ + { + name: "add field to required list if not required but opcon required", + channel: StandardChannel, + props: map[string]apiextensionsv1.JSONSchemaProps{ + "field1": { + Description: "Field 1.\n", + Type: "string", + }, + }, + existingRequired: []string{}, + expectedRequired: []string{"field1"}, + }, + { + name: "remove field from required list if required but opcon optional", + channel: StandardChannel, + props: map[string]apiextensionsv1.JSONSchemaProps{ + "field1": { + Description: "Field 1.\n", + Type: "string", + }, + }, + existingRequired: []string{"field1"}, + expectedRequired: []string{}, + }, + { + name: "preserve existing required without overriding opcon tag", + channel: StandardChannel, + props: map[string]apiextensionsv1.JSONSchemaProps{ + "field1": { + Description: "Field 1 without tag.", + Type: "string", + }, + }, + existingRequired: []string{"field1"}, + expectedRequired: []string{"field1"}, + }, + { + name: "multiple fields with mixed optional/required tags", + channel: StandardChannel, + props: map[string]apiextensionsv1.JSONSchemaProps{ + "field1": { + Description: "Field 1.\n", + Type: "string", + }, + "field2": { + Description: "Field 2.\n", + Type: "string", + }, + "field3": { + Description: "Field 3 without tag.", + Type: "string", + }, + }, + existingRequired: []string{"field2", "field3"}, + expectedRequired: []string{"field3", "field1"}, + }, + { + name: "no duplicate in required list when tag/opcon-tag both required", + channel: StandardChannel, + props: map[string]apiextensionsv1.JSONSchemaProps{ + "field1": { + Description: "Field 1.\n", + Type: "string", + }, + }, + existingRequired: []string{"field1"}, + expectedRequired: []string{"field1"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, required := opconTweaksMap(tt.channel, tt.props, tt.existingRequired) + require.ElementsMatch(t, tt.expectedRequired, required) + }) + } +} + func compareFiles(t *testing.T, file1, file2 string) { f1, err := os.Open(file1) require.NoError(t, err) From e0806cf744f800b90a9c88e83fd7b7cfcb59bbda Mon Sep 17 00:00:00 2001 From: grokspawn Date: Fri, 21 Nov 2025 15:18:27 -0600 Subject: [PATCH 2/2] review updates Signed-off-by: grokspawn --- hack/tools/crd-generator/main.go | 56 +++++++++++++++------------ hack/tools/crd-generator/main_test.go | 8 +++- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/hack/tools/crd-generator/main.go b/hack/tools/crd-generator/main.go index f5d10206c3..edc254494e 100644 --- a/hack/tools/crd-generator/main.go +++ b/hack/tools/crd-generator/main.go @@ -137,7 +137,7 @@ func runGenerator(args ...string) { if channel == StandardChannel && strings.Contains(version.Name, "alpha") { channelCrd.Spec.Versions[i].Served = false } - version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required) + channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema) } conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) @@ -180,10 +180,11 @@ func runGenerator(args ...string) { } } -// Apply Opcon specific tweaks to all properties in a map, and return a list of required fields according to opcon tags. -// For opcon validation optional/required tags, the required list is updated accordingly. -func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps, existingRequired []string) (map[string]apiextensionsv1.JSONSchemaProps, []string) { - newRequired := slices.Clone(existingRequired) +// Apply Opcon specific tweaks to all properties in a map, and update the parent schema's required list according to opcon tags. +// For opcon validation optional/required tags, the parent schema's required list is mutated directly. +// TODO: if we need to support other conditions from opconTweaks, it will likely be preferable to convey the parent schema to facilitate direct alteration. +func opconTweaksMap(channel string, parentSchema *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { + props := parentSchema.Properties for name := range props { jsonProps := props[name] @@ -195,30 +196,29 @@ func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaP // Update required list based on tag switch reqStatus { case statusRequired: - if !slices.Contains(newRequired, name) { - newRequired = append(newRequired, name) + if !slices.Contains(parentSchema.Required, name) { + parentSchema.Required = append(parentSchema.Required, name) } case statusOptional: - newRequired = slices.DeleteFunc(newRequired, func(s string) bool { return s == name }) + parentSchema.Required = slices.DeleteFunc(parentSchema.Required, func(s string) bool { return s == name }) default: // "" (unspecified) means keep existing status } } } - return props, newRequired + return props } -// Custom Opcon API Tweaks for tags prefixed with `") optReqMatches := optReqRe.FindAllStringSubmatch(jsonProps.Description, 64) + hasOptional := false + hasRequired := false for _, optReqMatch := range optReqMatches { if len(optReqMatch) != 2 { log.Fatalf("Invalid %s Optional/Required tag for %s", validationPrefix, name) @@ -274,11 +276,16 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche numValid++ switch optReqMatch[1] { case "Optional": + hasOptional = true requiredStatus = statusOptional case "Required": + hasRequired = true requiredStatus = statusRequired } } + if hasOptional && hasRequired { + log.Fatalf("Field %s has both Optional and Required validation tags for channel %s", name, channel) + } } if numValid < numExpressions { @@ -288,7 +295,7 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche jsonProps.Description = formatDescription(jsonProps.Description, channel, name) if len(jsonProps.Properties) > 0 { - jsonProps.Properties, jsonProps.Required = opconTweaksMap(channel, jsonProps.Properties, jsonProps.Required) + jsonProps.Properties = opconTweaksMap(channel, &jsonProps) } else if jsonProps.Items != nil && jsonProps.Items.Schema != nil { jsonProps.Items.Schema, _ = opconTweaks(channel, name, *jsonProps.Items.Schema) } @@ -299,24 +306,25 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche func formatDescription(description string, channel string, name string) string { tagset := []struct { channel string - start string - end string + tag string }{ - {channel: ExperimentalChannel, start: "", end: ""}, - {channel: StandardChannel, start: "", end: ""}, + {channel: ExperimentalChannel, tag: "opcon:standard:description"}, + {channel: StandardChannel, tag: "opcon:experimental:description"}, } for _, ts := range tagset { - if channel == ts.channel && strings.Contains(description, ts.start) { - regexPattern := `\n*` + regexp.QuoteMeta(ts.start) + `(?s:(.*?))` + regexp.QuoteMeta(ts.end) + `\n*` + startTag := fmt.Sprintf("<%s>", ts.tag) + endTag := fmt.Sprintf("", ts.tag) + if channel == ts.channel && strings.Contains(description, ts.tag) { + regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` re := regexp.MustCompile(regexPattern) match := re.FindStringSubmatch(description) if len(match) != 2 { - log.Fatalf("Invalid tag for %s", name) + log.Fatalf("Invalid %s tag for %s", startTag, name) } description = re.ReplaceAllString(description, "\n\n") } else { - description = strings.ReplaceAll(description, ts.start, "") - description = strings.ReplaceAll(description, ts.end, "") + description = strings.ReplaceAll(description, startTag, "") + description = strings.ReplaceAll(description, endTag, "") } } diff --git a/hack/tools/crd-generator/main_test.go b/hack/tools/crd-generator/main_test.go index ecfc7d8a72..99f71a497e 100644 --- a/hack/tools/crd-generator/main_test.go +++ b/hack/tools/crd-generator/main_test.go @@ -312,8 +312,12 @@ func TestOpconTweaksMapRequiredList(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, required := opconTweaksMap(tt.channel, tt.props, tt.existingRequired) - require.ElementsMatch(t, tt.expectedRequired, required) + parentSchema := &apiextensionsv1.JSONSchemaProps{ + Properties: tt.props, + Required: tt.existingRequired, + } + opconTweaksMap(tt.channel, parentSchema) + require.ElementsMatch(t, tt.expectedRequired, parentSchema.Required) }) } }