diff --git a/hack/tools/crd-generator/README.md b/hack/tools/crd-generator/README.md index 433472167..83fb63e21 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 9687489f4..53c2cd5d0 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) + channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema) } conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) @@ -179,14 +180,29 @@ 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 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. +func opconTweaksMap(channel string, parentSchema *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { + props := parentSchema.Properties + 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(parentSchema.Required, name) { + parentSchema.Required = append(parentSchema.Required, name) + } + case statusOptional: + parentSchema.Required = slices.DeleteFunc(parentSchema.Required, func(s string) bool { return s == name }) + default: + // "" (unspecified) means keep existing status + } } } return props @@ -194,10 +210,21 @@ func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaP // 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,43 @@ 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 = opconTweaksMap(channel, &jsonProps) } 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 + tag string + }{ + {channel: ExperimentalChannel, tag: "opcon:standard:description"}, + {channel: StandardChannel, tag: "opcon:experimental:description"}, + } + for _, ts := range tagset { + 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 %s tag for %s", startTag, name) + } + description = re.ReplaceAllString(description, "\n\n") + } else { + description = strings.ReplaceAll(description, startTag, "") + description = strings.ReplaceAll(description, endTag, "") } - 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 d2eb28d61..99f71a497 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,252 @@ 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) { + parentSchema := &apiextensionsv1.JSONSchemaProps{ + Properties: tt.props, + Required: tt.existingRequired, + } + opconTweaksMap(tt.channel, parentSchema) + require.ElementsMatch(t, tt.expectedRequired, parentSchema.Required) + }) + } +} + func compareFiles(t *testing.T, file1, file2 string) { f1, err := os.Open(file1) require.NoError(t, err)