From 8ef1aa2a8cf1938a607bc7e2d11f5d3826b9862b Mon Sep 17 00:00:00 2001 From: Nic Cope Date: Sat, 11 Nov 2023 23:08:51 -0800 Subject: [PATCH 1/2] Add server-side apply merge strategy markers This commit adds SSA merge strategy markers that allow the API server to granularly merge lists, rather than atomically replacing them. Composition functions use SSA, so this change means that a function can return only the list elements it desires (e.g. tags) and those elements will be merged into any existing elements, without replacing them. For the moment I've only covered two cases: * Lists that we know are sets of scalar values (generated from Terraform sets) * Maps of scalar values (generated from Terraform maps) I'm hopeful that in both of these cases it _should_ be possible to allow the map or set to be granularly merged, not atomically replaced. https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy Signed-off-by: Nic Cope --- pkg/types/field.go | 30 ++++++++++++ pkg/types/markers/options.go | 4 +- pkg/types/markers/ssa.go | 87 +++++++++++++++++++++++++++++++++++ pkg/types/markers/ssa_test.go | 49 ++++++++++++++++++++ 4 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 pkg/types/markers/ssa.go create mode 100644 pkg/types/markers/ssa_test.go diff --git a/pkg/types/field.go b/pkg/types/field.go index 838d9073..d2ed2325 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -15,6 +15,7 @@ import ( "github.com/crossplane/upjet/pkg" "github.com/crossplane/upjet/pkg/config" "github.com/crossplane/upjet/pkg/types/comments" + "github.com/crossplane/upjet/pkg/types/markers" "github.com/crossplane/upjet/pkg/types/name" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/pkg/errors" @@ -151,9 +152,38 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType + AddServerSideApplyMarkers(f) + return f, nil } +// AddServerSideApplyMarkers adds server-side apply comment markers to indicate +// that scalar maps and sets can be merged granularly, not replace atomically. +func AddServerSideApplyMarkers(f *Field) { + switch f.Schema.Type { //nolint:exhaustive + case schema.TypeMap: + // A map should always have an element of type Schema. + if es, ok := f.Schema.Elem.(*schema.Schema); ok { + switch es.Type { //nolint:exhaustive + // We assume scalar types can be granular maps. + case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: + f.Comment.ServerSideApplyOptions.MapType = ptr.To[markers.MapType](markers.MapTypeGranular) + } + } + case schema.TypeSet: + if es, ok := f.Schema.Elem.(*schema.Schema); ok { + switch es.Type { //nolint:exhaustive + // We assume scalar types can be granular maps. + case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: + f.Comment.ServerSideApplyOptions.ListType = ptr.To[markers.ListType](markers.ListTypeSet) + } + } + } + // TODO(negz): Can we reliably add SSA markers for lists of objects? Do we + // have cases where we're turning a Terraform map of maps into a list of + // objects with a well-known key that we could merge on? +} + // NewSensitiveField returns a constructed sensitive Field object. func NewSensitiveField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, snakeFieldName string, tfPath, xpPath, names []string, asBlocksMode bool) (*Field, bool, error) { //nolint:gocyclo f, err := NewField(g, cfg, r, sch, snakeFieldName, tfPath, xpPath, names, asBlocksMode) diff --git a/pkg/types/markers/options.go b/pkg/types/markers/options.go index fb5e06ae..c64b5773 100644 --- a/pkg/types/markers/options.go +++ b/pkg/types/markers/options.go @@ -9,11 +9,13 @@ type Options struct { UpjetOptions CrossplaneOptions KubebuilderOptions + ServerSideApplyOptions } // String returns a string representation of this Options object. func (o Options) String() string { return o.UpjetOptions.String() + o.CrossplaneOptions.String() + - o.KubebuilderOptions.String() + o.KubebuilderOptions.String() + + o.ServerSideApplyOptions.String() } diff --git a/pkg/types/markers/ssa.go b/pkg/types/markers/ssa.go new file mode 100644 index 00000000..c6891fc4 --- /dev/null +++ b/pkg/types/markers/ssa.go @@ -0,0 +1,87 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package markers + +import "fmt" + +// A ListType is a type of list. +type ListType string + +// Types of lists. +const ( + // ListTypeAtomic means the entire list is replaced during merge. At any + // point in time, a single manager owns the list. + ListTypeAtomic ListType = "atomic" + + // ListTypeSet can be granularly merged, and different managers can own + // different elements in the list. The list can include only scalar + // elements. + ListTypeSet ListType = "set" + + // ListTypeSet can be granularly merged, and different managers can own + // different elements in the list. The list can include only nested types + // (i.e. objects). + ListTypeMap ListType = "map" +) + +// A MapType is a type of map. +type MapType string + +// Types of maps. +const ( + // MapTypeAtomic means that the map can only be entirely replaced by a + // single manager. + MapTypeAtomic MapType = "atomic" + + // MapTypeGranular means that the map supports separate managers updating + // individual fields. + MapTypeGranular MapType = "granular" +) + +// A StructType is a type of struct. +type StructType string + +// Struct types. +const ( + // StructTypeAtomic means that the struct can only be entirely replaced by a + // single manager. + StructTypeAtomic StructType = "atomic" + + // StructTypeGranular means that the struct supports separate managers + // updating individual fields. + StructTypeGranular StructType = "granular" +) + +// ServerSideApplyOptions represents the server-side apply merge options that +// upjet needs to control. +// https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy +type ServerSideApplyOptions struct { + ListType *ListType + ListMapKey []string + MapType *MapType + StructType *StructType +} + +func (o ServerSideApplyOptions) String() string { + m := "" + + if o.ListType != nil { + m += fmt.Sprintf("+listType:%s\n", *o.ListType) + } + + for _, k := range o.ListMapKey { + m += fmt.Sprintf("+listMapKey:%s\n", k) + } + + if o.MapType != nil { + m += fmt.Sprintf("+mapType:%s\n", *o.MapType) + } + + if o.StructType != nil { + m += fmt.Sprintf("+structType:%s\n", *o.StructType) + } + + return m +} diff --git a/pkg/types/markers/ssa_test.go b/pkg/types/markers/ssa_test.go new file mode 100644 index 00000000..062d808c --- /dev/null +++ b/pkg/types/markers/ssa_test.go @@ -0,0 +1,49 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package markers + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "k8s.io/utils/ptr" +) + +func TestServerSideApplyOptions(t *testing.T) { + cases := map[string]struct { + o ServerSideApplyOptions + want string + }{ + "MapType": { + o: ServerSideApplyOptions{ + MapType: ptr.To[MapType](MapTypeAtomic), + }, + want: "+mapType:atomic\n", + }, + "StructType": { + o: ServerSideApplyOptions{ + StructType: ptr.To[StructType](StructTypeAtomic), + }, + want: "+structType:atomic\n", + }, + "ListType": { + o: ServerSideApplyOptions{ + ListType: ptr.To[ListType](ListTypeMap), + ListMapKey: []string{"name", "coolness"}, + }, + want: "+listType:map\n+listMapKey:name\n+listMapKey:coolness\n", + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := tc.o.String() + + if diff := cmp.Diff(tc.want, got); diff != "" { + t.Errorf("o.String(): -want, +got: %s", diff) + } + }) + } +} From 5150500b82b67cf882591ef7ed751a07fb52db4a Mon Sep 17 00:00:00 2001 From: Alper Rifat Ulucinar Date: Fri, 1 Dec 2023 03:01:30 +0300 Subject: [PATCH 2/2] Fix kubebuilder topological markers: use "=" instead of ":" between tokens - Do not add topological markers for sensitive fields Signed-off-by: Alper Rifat Ulucinar --- pkg/types/field.go | 6 ++++-- pkg/types/markers/ssa.go | 10 +++++----- pkg/types/markers/ssa_test.go | 6 +++--- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/types/field.go b/pkg/types/field.go index d2ed2325..97d6e9c9 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -152,7 +152,9 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType - AddServerSideApplyMarkers(f) + if !sch.Sensitive { + AddServerSideApplyMarkers(f) + } return f, nil } @@ -173,7 +175,7 @@ func AddServerSideApplyMarkers(f *Field) { case schema.TypeSet: if es, ok := f.Schema.Elem.(*schema.Schema); ok { switch es.Type { //nolint:exhaustive - // We assume scalar types can be granular maps. + // We assume scalar types can be granular sets. case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: f.Comment.ServerSideApplyOptions.ListType = ptr.To[markers.ListType](markers.ListTypeSet) } diff --git a/pkg/types/markers/ssa.go b/pkg/types/markers/ssa.go index c6891fc4..6a26c875 100644 --- a/pkg/types/markers/ssa.go +++ b/pkg/types/markers/ssa.go @@ -20,7 +20,7 @@ const ( // elements. ListTypeSet ListType = "set" - // ListTypeSet can be granularly merged, and different managers can own + // ListTypeMap can be granularly merged, and different managers can own // different elements in the list. The list can include only nested types // (i.e. objects). ListTypeMap ListType = "map" @@ -68,19 +68,19 @@ func (o ServerSideApplyOptions) String() string { m := "" if o.ListType != nil { - m += fmt.Sprintf("+listType:%s\n", *o.ListType) + m += fmt.Sprintf("+listType=%s\n", *o.ListType) } for _, k := range o.ListMapKey { - m += fmt.Sprintf("+listMapKey:%s\n", k) + m += fmt.Sprintf("+listMapKey=%s\n", k) } if o.MapType != nil { - m += fmt.Sprintf("+mapType:%s\n", *o.MapType) + m += fmt.Sprintf("+mapType=%s\n", *o.MapType) } if o.StructType != nil { - m += fmt.Sprintf("+structType:%s\n", *o.StructType) + m += fmt.Sprintf("+structType=%s\n", *o.StructType) } return m diff --git a/pkg/types/markers/ssa_test.go b/pkg/types/markers/ssa_test.go index 062d808c..dbfa5513 100644 --- a/pkg/types/markers/ssa_test.go +++ b/pkg/types/markers/ssa_test.go @@ -20,20 +20,20 @@ func TestServerSideApplyOptions(t *testing.T) { o: ServerSideApplyOptions{ MapType: ptr.To[MapType](MapTypeAtomic), }, - want: "+mapType:atomic\n", + want: "+mapType=atomic\n", }, "StructType": { o: ServerSideApplyOptions{ StructType: ptr.To[StructType](StructTypeAtomic), }, - want: "+structType:atomic\n", + want: "+structType=atomic\n", }, "ListType": { o: ServerSideApplyOptions{ ListType: ptr.To[ListType](ListTypeMap), ListMapKey: []string{"name", "coolness"}, }, - want: "+listType:map\n+listMapKey:name\n+listMapKey:coolness\n", + want: "+listType=map\n+listMapKey=name\n+listMapKey=coolness\n", }, }