From 0497abfaf859425382d22662abe9b2b0f10fea9f Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 19 Aug 2022 10:31:05 -0400 Subject: [PATCH 1/7] add metav1.OwnerReference to the default external configurations to ease generation --- .../code-generator/cmd/applyconfiguration-gen/args/args.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/args.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/args.go index 8a4fb14835a..78f364841f2 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/args.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/args.go @@ -50,8 +50,9 @@ func NewDefaults() (*args.GeneratorArgs, *CustomArgs) { customArgs := &CustomArgs{ ExternalApplyConfigurations: map[types.Name]string{ // Always include TypeMeta and ObjectMeta. They are sufficient for the vast majority of use cases. - {Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "TypeMeta"}: "k8s.io/client-go/applyconfigurations/meta/v1", - {Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ObjectMeta"}: "k8s.io/client-go/applyconfigurations/meta/v1", + {Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "TypeMeta"}: "k8s.io/client-go/applyconfigurations/meta/v1", + {Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "ObjectMeta"}: "k8s.io/client-go/applyconfigurations/meta/v1", + {Package: "k8s.io/apimachinery/pkg/apis/meta/v1", Name: "OwnerReference"}: "k8s.io/client-go/applyconfigurations/meta/v1", }, } genericArgs.CustomArgs = customArgs From 69a723e898f1b27cee2fb7b7aac79afb41b0c948 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 19 Aug 2022 10:43:15 -0400 Subject: [PATCH 2/7] applyconfiguration-gen handling of types that have a non-embedded use of TypeMeta --- .../cmd/applyconfiguration-gen/generators/applyconfiguration.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go index 5ebadb47517..b883398c3ec 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go @@ -118,7 +118,7 @@ func (g *applyConfigurationGenerator) GenerateType(c *generator.Context, t *type func hasTypeMetaField(t *types.Type) bool { for _, member := range t.Members { - if typeMeta.Name == member.Type.Name { + if typeMeta.Name == member.Type.Name && member.Embedded { return true } } From 59df3248ed58916386b95fb35d69d682c944ae39 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 19 Aug 2022 12:04:28 -0400 Subject: [PATCH 3/7] update the applyconfiguration-gen flag external-applyconfigurations to work --- .../args/externaltypes.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/externaltypes.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/externaltypes.go index 87a6badd71a..0785fbea0e3 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/externaltypes.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/args/externaltypes.go @@ -28,7 +28,6 @@ import ( type externalApplyConfigurationValue struct { externals *map[types.Name]string - changed bool } func NewExternalApplyConfigurationValue(externals *map[types.Name]string, def []string) *externalApplyConfigurationValue { @@ -45,10 +44,6 @@ func NewExternalApplyConfigurationValue(externals *map[types.Name]string, def [] var _ flag.Value = &externalApplyConfigurationValue{} func (s *externalApplyConfigurationValue) set(vs []string) error { - if !s.changed { - *s.externals = map[types.Name]string{} - } - for _, input := range vs { typ, pkg, err := parseExternalMapping(input) if err != nil { @@ -71,6 +66,7 @@ func (s *externalApplyConfigurationValue) Set(val string) error { if err := s.set(vs); err != nil { return err } + return nil } @@ -114,12 +110,13 @@ func parseExternalMapping(mapping string) (typ types.Name, pkg string, err error } packageTypeStr := parts[0] pkg = parts[1] - ptParts := strings.Split(packageTypeStr, ".") - if len(ptParts) != 2 { - return types.Name{}, "", fmt.Errorf("expected package and type of the form # but got %s", packageTypeStr) + // need to split on the *last* dot, since k8s.io (and other valid packages) have a dot in it + lastDot := strings.LastIndex(packageTypeStr, ".") + if lastDot == -1 || lastDot == len(packageTypeStr)-1 { + return types.Name{}, "", fmt.Errorf("expected package and type of the form . but got %s", packageTypeStr) } - structPkg := ptParts[0] - structType := ptParts[1] + structPkg := packageTypeStr[:lastDot] + structType := packageTypeStr[lastDot+1:] return types.Name{Package: structPkg, Name: structType}, pkg, nil } From 09d9d37c389d1c0ac6ec90a2cf680c0db1012fe6 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 19 Aug 2022 13:29:38 -0400 Subject: [PATCH 4/7] make applyconfiguration-gen work for resources without objectmeta --- .../generators/applyconfiguration.go | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go index b883398c3ec..e8ce5094e61 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go @@ -97,9 +97,12 @@ func (g *applyConfigurationGenerator) GenerateType(c *generator.Context, t *type g.generateStruct(sw, typeParams) if typeParams.Tags.GenerateClient { - if typeParams.Tags.NonNamespaced { + switch { + case !hasObjectMetaField(t): + sw.Do(clientgenTypeConstructorWithoutObjectMeta, typeParams) + case typeParams.Tags.NonNamespaced: sw.Do(clientgenTypeConstructorNonNamespaced, typeParams) - } else { + default: sw.Do(clientgenTypeConstructorNamespaced, typeParams) } if typeParams.OpenAPIType != nil { @@ -125,6 +128,15 @@ func hasTypeMetaField(t *types.Type) bool { return false } +func hasObjectMetaField(t *types.Type) bool { + for _, member := range t.Members { + if objectMeta.Name == member.Type.Name && member.Embedded { + return true + } + } + return false +} + func blocklisted(t *types.Type, member types.Member) bool { if objectMeta.Name == t.Name && member.Name == "ManagedFields" { return true @@ -330,6 +342,17 @@ func $.ApplyConfig.Type|public$(name string) *$.ApplyConfig.ApplyConfiguration|p } ` +var clientgenTypeConstructorWithoutObjectMeta = ` +// $.ApplyConfig.Type|public$ constructs an declarative configuration of the $.ApplyConfig.Type|public$ type for use with +// apply. +func $.ApplyConfig.Type|public$(name string) *$.ApplyConfig.ApplyConfiguration|public$ { + b := &$.ApplyConfig.ApplyConfiguration|public${} + b.WithKind("$.ApplyConfig.Type|singularKind$") + b.WithAPIVersion("$.APIVersion$") + return b +} +` + var constructorWithTypeMeta = ` // $.ApplyConfig.ApplyConfiguration|public$ constructs an declarative configuration of the $.ApplyConfig.Type|public$ type for use with // apply. @@ -375,7 +398,18 @@ func Extract$.ApplyConfig.Type|public$Status($.Struct|private$ *$.Struct|raw$, f } `, typeParams) } - sw.Do(` + if !hasObjectMetaField(typeParams.Struct) { + sw.Do(` +func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldManager string, subresource string) (*$.ApplyConfig.ApplyConfiguration|public$, error) { + b := &$.ApplyConfig.ApplyConfiguration|public${} + err := $.ExtractInto|raw$($.Struct|private$, $.ParserFunc|raw$().Type("$.OpenAPIType$"), fieldManager, b, subresource) + if err != nil { + return nil, err + } +`, typeParams) + + } else { // it has objectMeta + sw.Do(` func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldManager string, subresource string) (*$.ApplyConfig.ApplyConfiguration|public$, error) { b := &$.ApplyConfig.ApplyConfiguration|public${} err := $.ExtractInto|raw$($.Struct|private$, $.ParserFunc|raw$().Type("$.OpenAPIType$"), fieldManager, b, subresource) @@ -384,8 +418,9 @@ func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldMa } b.WithName($.Struct|private$.Name) `, typeParams) - if !typeParams.Tags.NonNamespaced { - sw.Do("b.WithNamespace($.Struct|private$.Namespace)\n", typeParams) + if !typeParams.Tags.NonNamespaced { + sw.Do("b.WithNamespace($.Struct|private$.Namespace)\n", typeParams) + } } sw.Do(` b.WithKind("$.ApplyConfig.Type|singularKind$") From e643b0e667741e732a9b7e0074f501d5c17e5272 Mon Sep 17 00:00:00 2001 From: David Eads Date: Fri, 19 Aug 2022 15:23:36 -0400 Subject: [PATCH 5/7] make applyconfiguration-gen handle pointers to slices --- .../generators/applyconfiguration.go | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go index e8ce5094e61..5dc3b8f2101 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go @@ -169,7 +169,6 @@ func (g *applyConfigurationGenerator) generateWithFuncs(t *types.Type, typeParam EmbeddedIn: embed, } if memberParams.Member.Embedded { - g.generateWithFuncs(member.Type, typeParams, sw, &memberParams) if !jsonTags.inline { // non-inlined embeds are nillable and need a "ensure exists" utility function @@ -177,12 +176,13 @@ func (g *applyConfigurationGenerator) generateWithFuncs(t *types.Type, typeParam } continue } + // For slices where the items are generated apply configuration types, accept varargs of // pointers of the type as "with" function arguments so the "with" function can be used like so: // WithFoos(Foo().WithName("x"), Foo().WithName("y")) if t := deref(member.Type); t.Kind == types.Slice && g.refGraph.isApplyConfig(t.Elem) { memberParams.ArgType = &types.Type{Kind: types.Pointer, Elem: memberType.Elem} - g.generateMemberWithForSlice(sw, memberParams) + g.generateMemberWithForSlice(sw, member, memberParams) continue } // Note: There are no maps where the values are generated apply configurations (because @@ -194,7 +194,7 @@ func (g *applyConfigurationGenerator) generateWithFuncs(t *types.Type, typeParam switch memberParams.Member.Type.Kind { case types.Slice: memberParams.ArgType = memberType.Elem - g.generateMemberWithForSlice(sw, memberParams) + g.generateMemberWithForSlice(sw, member, memberParams) case types.Map: g.generateMemberWithForMap(sw, memberParams) default: @@ -264,20 +264,39 @@ func (g *applyConfigurationGenerator) generateMemberWith(sw *generator.SnippetWr sw.Do("}\n", memberParams) } -func (g *applyConfigurationGenerator) generateMemberWithForSlice(sw *generator.SnippetWriter, memberParams memberParams) { +func (g *applyConfigurationGenerator) generateMemberWithForSlice(sw *generator.SnippetWriter, member types.Member, memberParams memberParams) { + memberIsPointerToSlice := member.Type.Kind == types.Pointer + if memberIsPointerToSlice { + sw.Do(ensureNonEmbedSliceExists, memberParams) + } + sw.Do("// With$.Member.Name$ adds the given value to the $.Member.Name$ field in the declarative configuration\n", memberParams) sw.Do("// and returns the receiver, so that objects can be build by chaining \"With\" function invocations.\n", memberParams) sw.Do("// If called multiple times, values provided by each call will be appended to the $.Member.Name$ field.\n", memberParams) sw.Do("func (b *$.ApplyConfig.ApplyConfiguration|public$) With$.Member.Name$(values ...$.ArgType|raw$) *$.ApplyConfig.ApplyConfiguration|public$ {\n", memberParams) g.ensureEnbedExistsIfApplicable(sw, memberParams) + + if memberIsPointerToSlice { + sw.Do("b.ensure$.MemberType.Elem|public$Exists()\n", memberParams) + } + sw.Do(" for i := range values {\n", memberParams) if memberParams.ArgType.Kind == types.Pointer { sw.Do("if values[i] == nil {\n", memberParams) sw.Do(" panic(\"nil value passed to With$.Member.Name$\")\n", memberParams) sw.Do("}\n", memberParams) - sw.Do("b.$.Member.Name$ = append(b.$.Member.Name$, *values[i])\n", memberParams) + + if memberIsPointerToSlice { + sw.Do("*b.$.Member.Name$ = append(*b.$.Member.Name$, *values[i])\n", memberParams) + } else { + sw.Do("b.$.Member.Name$ = append(b.$.Member.Name$, *values[i])\n", memberParams) + } } else { - sw.Do("b.$.Member.Name$ = append(b.$.Member.Name$, values[i])\n", memberParams) + if memberIsPointerToSlice { + sw.Do("*b.$.Member.Name$ = append(*b.$.Member.Name$, values[i])\n", memberParams) + } else { + sw.Do("b.$.Member.Name$ = append(b.$.Member.Name$, values[i])\n", memberParams) + } } sw.Do(" }\n", memberParams) sw.Do(" return b\n", memberParams) @@ -317,6 +336,14 @@ func (b *$.ApplyConfig.ApplyConfiguration|public$) ensure$.MemberType.Elem|publi } ` +var ensureNonEmbedSliceExists = ` +func (b *$.ApplyConfig.ApplyConfiguration|public$) ensure$.MemberType.Elem|public$Exists() { + if b.$.Member.Name$ == nil { + b.$.Member.Name$ = &[]$.MemberType.Elem|raw${} + } +} +` + var clientgenTypeConstructorNamespaced = ` // $.ApplyConfig.Type|public$ constructs an declarative configuration of the $.ApplyConfig.Type|public$ type for use with // apply. From 06497aa13ce1dcdc6efa383c5a728054a34f2ea9 Mon Sep 17 00:00:00 2001 From: David Eads Date: Mon, 22 Aug 2022 16:01:55 -0400 Subject: [PATCH 6/7] update the apply generator to use the same package the the client generator expects --- .../cmd/applyconfiguration-gen/generators/packages.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go index 53bea62fa56..ec716d529be 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go @@ -30,6 +30,7 @@ import ( "k8s.io/klog/v2" applygenargs "k8s.io/code-generator/cmd/applyconfiguration-gen/args" + "k8s.io/code-generator/cmd/client-gen/generators/util" clientgentypes "k8s.io/code-generator/cmd/client-gen/types" ) @@ -236,8 +237,10 @@ func packageTypesForInputDirs(context *generator.Context, inputDirs []string, ou klog.Warningf("Skipping internal package: %s", p.Path) continue } - gv := groupVersion(p) - pkg := filepath.Join(outputPath, gv.Group.PackageName(), strings.ToLower(gv.Version.NonEmpty())) + // this is how the client generator finds the package we are creating. It uses the API package name, not the group name. + _, gvPackageString := util.ParsePathGroupVersion(p.Path) + + pkg := filepath.Join(outputPath, strings.ToLower(gvPackageString)) pkgTypes[pkg] = p } return pkgTypes From 45c4311b4e5926ef00ebcccff8e60716bc844b6f Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 25 Aug 2022 16:29:08 -0400 Subject: [PATCH 7/7] make applyconfiguration-gen skip generation for types that have generated clients and lack objectmeta --- .../generators/applyconfiguration.go | 45 +++---------------- .../generators/packages.go | 22 ++++++++- 2 files changed, 25 insertions(+), 42 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go index 5dc3b8f2101..8e02bb233bc 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/applyconfiguration.go @@ -97,12 +97,9 @@ func (g *applyConfigurationGenerator) GenerateType(c *generator.Context, t *type g.generateStruct(sw, typeParams) if typeParams.Tags.GenerateClient { - switch { - case !hasObjectMetaField(t): - sw.Do(clientgenTypeConstructorWithoutObjectMeta, typeParams) - case typeParams.Tags.NonNamespaced: + if typeParams.Tags.NonNamespaced { sw.Do(clientgenTypeConstructorNonNamespaced, typeParams) - default: + } else { sw.Do(clientgenTypeConstructorNamespaced, typeParams) } if typeParams.OpenAPIType != nil { @@ -128,15 +125,6 @@ func hasTypeMetaField(t *types.Type) bool { return false } -func hasObjectMetaField(t *types.Type) bool { - for _, member := range t.Members { - if objectMeta.Name == member.Type.Name && member.Embedded { - return true - } - } - return false -} - func blocklisted(t *types.Type, member types.Member) bool { if objectMeta.Name == t.Name && member.Name == "ManagedFields" { return true @@ -369,17 +357,6 @@ func $.ApplyConfig.Type|public$(name string) *$.ApplyConfig.ApplyConfiguration|p } ` -var clientgenTypeConstructorWithoutObjectMeta = ` -// $.ApplyConfig.Type|public$ constructs an declarative configuration of the $.ApplyConfig.Type|public$ type for use with -// apply. -func $.ApplyConfig.Type|public$(name string) *$.ApplyConfig.ApplyConfiguration|public$ { - b := &$.ApplyConfig.ApplyConfiguration|public${} - b.WithKind("$.ApplyConfig.Type|singularKind$") - b.WithAPIVersion("$.APIVersion$") - return b -} -` - var constructorWithTypeMeta = ` // $.ApplyConfig.ApplyConfiguration|public$ constructs an declarative configuration of the $.ApplyConfig.Type|public$ type for use with // apply. @@ -425,18 +402,7 @@ func Extract$.ApplyConfig.Type|public$Status($.Struct|private$ *$.Struct|raw$, f } `, typeParams) } - if !hasObjectMetaField(typeParams.Struct) { - sw.Do(` -func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldManager string, subresource string) (*$.ApplyConfig.ApplyConfiguration|public$, error) { - b := &$.ApplyConfig.ApplyConfiguration|public${} - err := $.ExtractInto|raw$($.Struct|private$, $.ParserFunc|raw$().Type("$.OpenAPIType$"), fieldManager, b, subresource) - if err != nil { - return nil, err - } -`, typeParams) - - } else { // it has objectMeta - sw.Do(` + sw.Do(` func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldManager string, subresource string) (*$.ApplyConfig.ApplyConfiguration|public$, error) { b := &$.ApplyConfig.ApplyConfiguration|public${} err := $.ExtractInto|raw$($.Struct|private$, $.ParserFunc|raw$().Type("$.OpenAPIType$"), fieldManager, b, subresource) @@ -445,9 +411,8 @@ func extract$.ApplyConfig.Type|public$($.Struct|private$ *$.Struct|raw$, fieldMa } b.WithName($.Struct|private$.Name) `, typeParams) - if !typeParams.Tags.NonNamespaced { - sw.Do("b.WithNamespace($.Struct|private$.Namespace)\n", typeParams) - } + if !typeParams.Tags.NonNamespaced { + sw.Do("b.WithNamespace($.Struct|private$.Namespace)\n", typeParams) } sw.Do(` b.WithKind("$.ApplyConfig.Type|singularKind$") diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go index ec716d529be..bfeffda593d 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/packages.go @@ -82,6 +82,13 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat var toGenerate []applyConfig for _, t := range p.Types { + // If we don't have an ObjectMeta field, we lack the information required to make the Apply or ApplyStatus call + // to the kube-apiserver, so we don't need to generate the type at all + clientTags := genclientTags(t) + if clientTags.GenerateClient && !hasObjectMetaField(t) { + klog.V(5).Infof("skipping type %v because does not have ObjectMeta", t) + continue + } if typePkg, ok := refs[t.Name]; ok { toGenerate = append(toGenerate, applyConfig{ Type: t, @@ -237,9 +244,11 @@ func packageTypesForInputDirs(context *generator.Context, inputDirs []string, ou klog.Warningf("Skipping internal package: %s", p.Path) continue } - // this is how the client generator finds the package we are creating. It uses the API package name, not the group name. + // This is how the client generator finds the package we are creating. It uses the API package name, not the group name. + // This matches the approach of the client-gen, so the two generator can work together. + // For example, if openshift/api/cloudnetwork/v1 contains an apigroup cloud.network.openshift.io, the client-gen + // builds a package called cloudnetwork/v1 to contain it. This change makes the applyconfiguration-gen use the same. _, gvPackageString := util.ParsePathGroupVersion(p.Path) - pkg := filepath.Join(outputPath, strings.ToLower(gvPackageString)) pkgTypes[pkg] = p } @@ -277,3 +286,12 @@ func isInternal(m types.Member) bool { _, ok := lookupJSONTags(m) return !ok } + +func hasObjectMetaField(t *types.Type) bool { + for _, member := range t.Members { + if objectMeta.Name == member.Type.Name && member.Embedded { + return true + } + } + return false +}