diff --git a/pkg/config/common.go b/pkg/config/common.go index ee291e39..eb5c0b8d 100644 --- a/pkg/config/common.go +++ b/pkg/config/common.go @@ -77,17 +77,18 @@ func DefaultResource(name string, terraformSchema *schema.Resource, terraformReg } r := &Resource{ - Name: name, - TerraformResource: terraformSchema, - MetaResource: terraformRegistry, - ShortGroup: group, - Kind: kind, - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: make(map[string]*SchemaElementOption), + Name: name, + TerraformResource: terraformSchema, + MetaResource: terraformRegistry, + ShortGroup: group, + Kind: kind, + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: make(References), + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: make(SchemaElementOptions), + ServerSideApplyMergeStrategies: make(ServerSideApplyMergeStrategies), } for _, f := range opts { f(r) diff --git a/pkg/config/common_test.go b/pkg/config/common_test.go index 3ad62cab..4b8a7207 100644 --- a/pkg/config/common_test.go +++ b/pkg/config/common_test.go @@ -33,15 +33,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_ec2_instance", }, want: &Resource{ - Name: "aws_ec2_instance", - ShortGroup: "ec2", - Kind: "Instance", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_ec2_instance", + ShortGroup: "ec2", + Kind: "Instance", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "TwoSectionsName": { @@ -50,15 +51,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_instance", }, want: &Resource{ - Name: "aws_instance", - ShortGroup: "aws", - Kind: "Instance", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_instance", + ShortGroup: "aws", + Kind: "Instance", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithPrefixAcronym": { @@ -67,15 +69,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_sql_server", }, want: &Resource{ - Name: "aws_db_sql_server", - ShortGroup: "db", - Kind: "SQLServer", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_sql_server", + ShortGroup: "db", + Kind: "SQLServer", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithSuffixAcronym": { @@ -84,15 +87,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_server_id", }, want: &Resource{ - Name: "aws_db_server_id", - ShortGroup: "db", - Kind: "ServerID", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_server_id", + ShortGroup: "db", + Kind: "ServerID", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, "NameWithMultipleAcronyms": { @@ -101,15 +105,16 @@ func TestDefaultResource(t *testing.T) { name: "aws_db_sql_server_id", }, want: &Resource{ - Name: "aws_db_sql_server_id", - ShortGroup: "db", - Kind: "SQLServerID", - Version: "v1alpha1", - ExternalName: NameAsIdentifier, - References: map[string]Reference{}, - Sensitive: NopSensitive, - UseAsync: true, - SchemaElementOptions: SchemaElementOptions{}, + Name: "aws_db_sql_server_id", + ShortGroup: "db", + Kind: "SQLServerID", + Version: "v1alpha1", + ExternalName: NameAsIdentifier, + References: map[string]Reference{}, + Sensitive: NopSensitive, + UseAsync: true, + SchemaElementOptions: SchemaElementOptions{}, + ServerSideApplyMergeStrategies: ServerSideApplyMergeStrategies{}, }, }, } diff --git a/pkg/config/resource.go b/pkg/config/resource.go index 876ec6f5..1ad1e777 100644 --- a/pkg/config/resource.go +++ b/pkg/config/resource.go @@ -24,6 +24,54 @@ import ( "github.com/crossplane/upjet/pkg/registry" ) +// 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" + + // 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" +) + +// 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" +) + // SetIdentifierArgumentsFn sets the name of the resource in Terraform attributes map, // i.e. Main HCL file. type SetIdentifierArgumentsFn func(base map[string]any, externalName string) @@ -266,6 +314,60 @@ func setExternalTagsWithPaved(externalTags map[string]string, paved *fieldpath.P return pavedByte, nil } +type InjectedKey struct { + Key string + DefaultValue string +} + +// ListMapKeys is the list map keys when the server-side apply merge strategy +// islistType=map. +type ListMapKeys struct { + // InjectedKey can be used to inject the specified index key + // into the generated CRD schema for the list object when + // the SSA merge strategy for the parent list is `map`. + // If a non-zero `InjectedKey` is specified, then a field of type string with + // the specified name is injected into the Terraform schema and used as + // a list map key together with any other existing keys specified in `Keys`. + InjectedKey InjectedKey + // Keys is the set of list map keys to be used while SSA merges list items. + // If InjectedKey is non-zero, then it's automatically put into Keys and + // you must not specify the InjectedKey in Keys explicitly. + Keys []string +} + +// ListMergeStrategy configures the corresponding field as list +// and configures its server-side apply merge strategy. +type ListMergeStrategy struct { + // ListMapKeys is the list map keys when the SSA merge strategy is + // `listType=map`. The keys specified here must be a set of scalar Terraform + // argument names to be used as the list map keys for the object list. + ListMapKeys ListMapKeys + // MergeStrategy is the SSA merge strategy for an object list. Valid values + // are: `atomic`, `set` and `map` + MergeStrategy ListType +} + +// MergeStrategy configures the server-side apply merge strategy for the +// corresponding field. One and only one of the pointer members can be set +// and the specified merge strategy configuration must match the field's +// type, e.g., you cannot set MapMergeStrategy for a field of type list. +type MergeStrategy struct { + ListMergeStrategy ListMergeStrategy + MapMergeStrategy MapType + StructMergeStrategy StructType +} + +// ServerSideApplyMergeStrategies configures the server-side apply merge strategy +// for the field at the specified path as the map key. The key is +// a Terraform configuration argument path such as a.b.c, without any +// index notation (i.e., array/map components do not need indices). +// It's an error to set a configuration option which does not match +// the object type at the specified path or to leave the corresponding +// configuration entry empty. For example, if the field at path a.b.c is +// a list, then ListMergeStrategy must be set and it should be the only +// configuration entry set. +type ServerSideApplyMergeStrategies map[string]MergeStrategy + // Resource is the set of information that you can override at different steps // of the code generation pipeline. type Resource struct { @@ -338,6 +440,12 @@ type Resource struct { // Terraform InstanceDiff is computed during reconciliation. TerraformCustomDiff CustomDiff + // ServerSideApplyMergeStrategies configures the server-side apply merge + // strategy for the fields at the given map keys. The map key is + // a Terraform configuration argument path such as a.b.c, without any + // index notation (i.e., array/map components do not need indices). + ServerSideApplyMergeStrategies ServerSideApplyMergeStrategies + // useNoForkClient indicates that a no-fork external client should // be generated instead of the Terraform CLI-forking client. useNoForkClient bool diff --git a/pkg/controller/external_nofork.go b/pkg/controller/external_nofork.go index 3bcd674b..52a9413a 100644 --- a/pkg/controller/external_nofork.go +++ b/pkg/controller/external_nofork.go @@ -227,7 +227,7 @@ func (c *NoForkConnector) applyStateFuncToParam(sc *schema.Schema, param any) an return param } -func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (managed.ExternalClient, error) { +func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (managed.ExternalClient, error) { //nolint:gocyclo c.metricRecorder.ObserveReconcileDelay(mg.GetObjectKind().GroupVersionKind(), mg.GetName()) logger := c.logger.WithValues("uid", mg.GetUID(), "name", mg.GetName(), "gvk", mg.GetObjectKind().GroupVersionKind().String()) logger.Debug("Connecting to the service provider") @@ -279,6 +279,17 @@ func (c *NoForkConnector) Connect(ctx context.Context, mg xpresource.Managed) (m } s.RawPlan = tfStateCtyValue s.RawConfig = rawConfig + + timeouts := getTimeoutParameters(c.config) + if len(timeouts) > 0 { + if s == nil { + s = &tf.InstanceState{} + } + if s.Meta == nil { + s.Meta = make(map[string]interface{}) + } + s.Meta[schema.TimeoutKey] = timeouts + } opTracker.SetTfState(s) } @@ -405,7 +416,7 @@ func getTimeoutParameters(config *config.Resource) map[string]any { //nolint:goc func (n *noForkExternal) getResourceDataDiff(tr resource.Terraformed, ctx context.Context, s *tf.InstanceState, resourceExists bool) (*tf.InstanceDiff, error) { //nolint:gocyclo resourceConfig := tf.NewResourceConfigRaw(n.params) - instanceDiff, err := schema.InternalMap(n.config.TerraformResource.Schema).Diff(ctx, s, resourceConfig, nil, n.ts.Meta, false) + instanceDiff, err := schema.InternalMap(n.config.TerraformResource.Schema).Diff(ctx, s, resourceConfig, n.config.TerraformResource.CustomizeDiff, n.ts.Meta, false) if err != nil { return nil, errors.Wrap(err, "failed to get *terraform.InstanceDiff") } @@ -515,7 +526,7 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma resource.SetUpToDateCondition(mg, noDiff) } // check for an external-name change - if nameChanged, err := n.setExternalName(mg, newState); err != nil { + if nameChanged, err := n.setExternalName(mg, stateValueMap); err != nil { return managed.ExternalObservation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during observe") } else { specUpdateRequired = specUpdateRequired || nameChanged @@ -532,15 +543,14 @@ func (n *noForkExternal) Observe(ctx context.Context, mg xpresource.Managed) (ma // sets the external-name on the MR. Returns `true` // if the external-name of the MR has changed. -func (n *noForkExternal) setExternalName(mg xpresource.Managed, newState *tf.InstanceState) (bool, error) { - if newState.ID == "" { +func (n *noForkExternal) setExternalName(mg xpresource.Managed, stateValueMap map[string]interface{}) (bool, error) { + id, ok := stateValueMap["id"] + if !ok || id.(string) == "" { return false, nil } - newName, err := n.config.ExternalName.GetExternalNameFn(map[string]any{ - "id": newState.ID, - }) + newName, err := n.config.ExternalName.GetExternalNameFn(stateValueMap) if err != nil { - return false, errors.Wrapf(err, "failed to get the external-name from ID: %s", newState.ID) + return false, errors.Wrapf(err, "failed to compute the external-name from the state map of the resource with the ID %s", id) } oldName := meta.GetExternalName(mg) // we have to make sure the newly set external-name is recorded @@ -553,8 +563,36 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man start := time.Now() newState, diag := n.resourceSchema.Apply(ctx, n.opTracker.GetTfState(), n.instanceDiff, n.ts.Meta) metrics.ExternalAPITime.WithLabelValues("create").Observe(time.Since(start).Seconds()) - // diag := n.resourceSchema.CreateWithoutTimeout(ctx, n.resourceData, n.ts.Meta) if diag != nil && diag.HasError() { + // we need to store the Terraform state from the downstream create call if + // one is available even if the diagnostics has reported errors. + // The downstream create call comprises multiple external API calls such as + // the external resource create call, expected state assertion calls + // (external resource state reads) and external resource state refresh + // calls, etc. Any of these steps can fail and if the initial + // external resource create call succeeds, then the TF plugin SDK makes the + // state (together with the TF ID associated with the external resource) + // available reporting any encountered issues in the returned diagnostics. + // If we don't record the returned state from the successful create call, + // then we may hit issues for resources whose Crossplane identifiers cannot + // be computed solely from spec parameters and provider configs, i.e., + // those that contain a random part generated by the CSP. Please see: + // https://github.com/upbound/provider-aws/issues/1010, or + // https://github.com/upbound/provider-aws/issues/1018, which both involve + // MRs with config.IdentifierFromProvider external-name configurations. + // NOTE: The safe (and thus the proper) thing to do in this situation from + // the Crossplane provider's perspective is to set the MR's + // `crossplane.io/external-create-failed` annotation because the provider + // does not know the exact state the external resource is in and a manual + // intervention may be required. But at the time we are introducing this + // fix, we believe associating the external-resource with the MR will just + // provide a better UX although the external resource may not be in the + // expected/desired state yet. We are also planning for improvements on the + // crossplane-runtime's managed reconciler to better support upjet's async + // operations in this regard. + if !n.opTracker.HasState() { // we do not expect a previous state here but just being defensive + n.opTracker.SetTfState(newState) + } return managed.ExternalCreation{}, errors.Errorf("failed to create the resource: %v", diag) } @@ -563,12 +601,12 @@ func (n *noForkExternal) Create(ctx context.Context, mg xpresource.Managed) (man } n.opTracker.SetTfState(newState) - if _, err := n.setExternalName(mg, newState); err != nil { - return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") - } stateValueMap, err := n.fromInstanceStateToJSONMap(newState) if err != nil { - return managed.ExternalCreation{}, err + return managed.ExternalCreation{}, errors.Wrap(err, "failed to convert instance state to map") + } + if _, err := n.setExternalName(mg, stateValueMap); err != nil { + return managed.ExternalCreation{}, errors.Wrapf(err, "failed to set the external-name of the managed resource during create") } err = mg.(resource.Terraformed).SetObservation(stateValueMap) if err != nil { diff --git a/pkg/resource/sensitive.go b/pkg/resource/sensitive.go index 229ddd97..54959e9e 100644 --- a/pkg/resource/sensitive.go +++ b/pkg/resource/sensitive.go @@ -107,6 +107,9 @@ func GetSensitiveAttributes(from map[string]any, mapping map[string]string) (map for tf := range mapping { fieldPaths, err := paved.ExpandWildcards(tf) if err != nil { + if fieldpath.IsNotFound(err) { + continue + } return nil, errors.Wrap(err, errCannotExpandWildcards) } diff --git a/pkg/terraform/files.go b/pkg/terraform/files.go index 78adac16..26a8fe1c 100644 --- a/pkg/terraform/files.go +++ b/pkg/terraform/files.go @@ -130,9 +130,9 @@ type FileProducer struct { features *feature.Flags } -// WriteMainTF writes the content main configuration file that has the desired -// state configuration for Terraform. -func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { +// BuildMainTF produces the contents of the mainTF file as a map. This format is conducive to +// inspection for tests. WriteMainTF calls this function an serializes the result to a file as JSON. +func (fp *FileProducer) BuildMainTF() map[string]any { // If the resource is in a deletion process, we need to remove the deletion // protection. lifecycle := map[string]any{ @@ -153,7 +153,7 @@ func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { // Note(turkenh): To use third party providers, we need to configure // provider name in required_providers. providerSource := strings.Split(fp.Setup.Requirement.Source, "/") - m := map[string]any{ + return map[string]any{ "terraform": map[string]any{ "required_providers": map[string]any{ providerSource[len(providerSource)-1]: map[string]string{ @@ -171,6 +171,12 @@ func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { }, }, } +} + +// WriteMainTF writes the content main configuration file that has the desired +// state configuration for Terraform. +func (fp *FileProducer) WriteMainTF() (ProviderHandle, error) { + m := fp.BuildMainTF() rawMainTF, err := json.JSParser.Marshal(m) if err != nil { return InvalidProviderHandle, errors.Wrap(err, "cannot marshal main hcl object") diff --git a/pkg/types/builder.go b/pkg/types/builder.go index b4dbc3ef..6b9aa4a8 100644 --- a/pkg/types/builder.go +++ b/pkg/types/builder.go @@ -27,6 +27,9 @@ const ( // ref: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules celEscapeSequence = "__%s__" + // description for an injected list map key field in the context of the + // server-side apply object list merging + descriptionInjectedKey = "This is an injected field with a default value for being able to merge items of the parent object list." ) var ( @@ -67,6 +70,10 @@ func NewBuilder(pkg *types.Package) *Builder { // Build returns parameters and observation types built out of Terraform schema. func (g *Builder) Build(cfg *config.Resource) (Generated, error) { + if err := injectServerSideApplyListMergeKeys(cfg); err != nil { + return Generated{}, errors.Wrapf(err, "cannot inject server-side apply merge keys for resource %q", cfg.Name) + } + fp, ap, ip, err := g.buildResource(cfg.TerraformResource, cfg, nil, nil, false, cfg.Kind) return Generated{ Types: g.genTypes, @@ -75,7 +82,46 @@ func (g *Builder) Build(cfg *config.Resource) (Generated, error) { InitProviderType: ip, AtProviderType: ap, ValidationRules: g.validationRules, - }, errors.Wrapf(err, "cannot build the Types") + }, errors.Wrapf(err, "cannot build the Types for resource %q", cfg.Name) +} + +func injectServerSideApplyListMergeKeys(cfg *config.Resource) error { //nolint:gocyclo // Easier to follow the logic in a single function + for f, s := range cfg.ServerSideApplyMergeStrategies { + if s.ListMergeStrategy.MergeStrategy != config.ListTypeMap { + continue + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key == "" && len(s.ListMergeStrategy.ListMapKeys.Keys) == 0 { + return errors.Errorf("list map keys configuration for the object list %q is empty", f) + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key == "" { + continue + } + sch := config.GetSchema(cfg.TerraformResource, f) + if sch == nil { + return errors.Errorf("cannot find the Terraform schema for the argument at the path %q", f) + } + if sch.Type != schema.TypeList && sch.Type != schema.TypeSet { + return errors.Errorf("fieldpath %q is not a Terraform list or set", f) + } + el, ok := sch.Elem.(*schema.Resource) + if !ok { + return errors.Errorf("fieldpath %q is a Terraform list or set but its element type is not a Terraform *schema.Resource", f) + } + for k := range el.Schema { + if k == s.ListMergeStrategy.ListMapKeys.InjectedKey.Key { + return errors.Errorf("element schema for the object list %q already contains the argument key %q", f, k) + } + } + el.Schema[s.ListMergeStrategy.ListMapKeys.InjectedKey.Key] = &schema.Schema{ + Type: schema.TypeString, + Required: true, + Description: descriptionInjectedKey, + } + if s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue != "" { + el.Schema[s.ListMergeStrategy.ListMapKeys.InjectedKey.Key].Default = s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue + } + } + return nil } func (g *Builder) buildResource(res *schema.Resource, cfg *config.Resource, tfPath []string, xpPath []string, asBlocksMode bool, names ...string) (*types.Named, *types.Named, *types.Named, error) { //nolint:gocyclo @@ -228,7 +274,7 @@ func (g *Builder) buildSchema(f *Field, cfg *config.Resource, names []string, r if paramType.Underlying().String() != emptyStruct { field := types.NewField(token.NoPos, g.Package, f.Name.Camel, types.NewSlice(paramType), false) r.addParameterField(f, field) - r.addInitField(f, field, g.Package) + r.addInitField(f, field, g, nil) } default: if paramType == nil { @@ -354,7 +400,7 @@ func (r *resource) addParameterField(f *Field, field *types.Var) { r.paramFields = append(r.paramFields, field) } -func (r *resource) addInitField(f *Field, field *types.Var, pkg *types.Package) { +func (r *resource) addInitField(f *Field, field *types.Var, g *Builder, typeNames *types.TypeName) { // If the field is not an init field, we don't add it. if !f.isInit() { return @@ -364,10 +410,14 @@ func (r *resource) addInitField(f *Field, field *types.Var, pkg *types.Package) // If the field is a nested type, we need to add it as the init type. if f.InitType != nil { - field = types.NewField(token.NoPos, pkg, f.Name.Camel, f.InitType, false) + field = types.NewField(token.NoPos, g.Package, f.Name.Camel, f.InitType, false) } r.initFields = append(r.initFields, field) + + if f.Reference != nil { + r.addReferenceFields(g, typeNames, f, true) + } } func (r *resource) addObservationField(f *Field, field *types.Var) { @@ -383,10 +433,15 @@ func (r *resource) addObservationField(f *Field, field *types.Var) { r.obsTags = append(r.obsTags, fmt.Sprintf(`json:"%s" tf:"%s"`, f.JSONTag, f.TFTag)) } -func (r *resource) addReferenceFields(g *Builder, paramName *types.TypeName, field *Field) { +func (r *resource) addReferenceFields(g *Builder, paramName *types.TypeName, field *Field, isInit bool) { refFields, refTags := g.generateReferenceFields(paramName, field) - r.paramTags = append(r.paramTags, refTags...) - r.paramFields = append(r.paramFields, refFields...) + if isInit { + r.initTags = append(r.initTags, refTags...) + r.initFields = append(r.initFields, refFields...) + } else { + r.paramTags = append(r.paramTags, refTags...) + r.paramFields = append(r.paramFields, refFields...) + } } // generateTypeName generates a unique name for the type if its original name diff --git a/pkg/types/builder_test.go b/pkg/types/builder_test.go index 0322f2e9..2c559f7b 100644 --- a/pkg/types/builder_test.go +++ b/pkg/types/builder_test.go @@ -314,6 +314,7 @@ func TestBuild(t *testing.T) { "Invalid_Sensitive_Fields": { args: args{ cfg: &config.Resource{ + Name: "test_resource", TerraformResource: &schema.Resource{ Schema: map[string]*schema.Schema{ "key_1": { @@ -325,7 +326,7 @@ func TestBuild(t *testing.T) { }, }, want: want{ - err: errors.Wrapf(fmt.Errorf(`got type %q for field %q, only types "string", "*string", []string, []*string, "map[string]string" and "map[string]*string" supported as sensitive`, "*float64", "Key1"), "cannot build the Types"), + err: errors.Wrapf(fmt.Errorf(`got type %q for field %q, only types "string", "*string", []string, []*string, "map[string]string" and "map[string]*string" supported as sensitive`, "*float64", "Key1"), `cannot build the Types for resource "test_resource"`), }, }, "References": { @@ -361,6 +362,7 @@ func TestBuild(t *testing.T) { "Invalid_Schema_Type": { args: args{ cfg: &config.Resource{ + Name: "test_resource", TerraformResource: &schema.Resource{ Schema: map[string]*schema.Schema{ "name": { @@ -372,7 +374,7 @@ func TestBuild(t *testing.T) { }, }, want: want{ - err: errors.Wrapf(errors.Wrapf(errors.Errorf("invalid schema type %s", "TypeInvalid"), "cannot infer type from schema of field %s", "name"), "cannot build the Types"), + err: errors.Wrapf(errors.Wrapf(errors.Errorf("invalid schema type %s", "TypeInvalid"), "cannot infer type from schema of field %s", "name"), `cannot build the Types for resource "test_resource"`), }, }, "Validation_Rules_With_Keywords": { diff --git a/pkg/types/field.go b/pkg/types/field.go index 4a130eb7..2e6fc2dc 100644 --- a/pkg/types/field.go +++ b/pkg/types/field.go @@ -22,6 +22,12 @@ import ( "github.com/crossplane/upjet/pkg/types/name" ) +const ( + errFmtInvalidSSAConfiguration = "invalid server-side apply merge strategy configuration: Field schema for %q is of type %q and the specified configuration must only set %q" + errFmtUnsupportedSSAField = "cannot configure the server-side apply merge strategy for %q: Configuration can only be specified for lists, sets or maps" + errFmtMissingListMapKeys = "server-side apply merge strategy configuration for %q belongs to a list of type map but list map keys configuration is missing" +) + var parentheses = regexp.MustCompile(`\(([^)]+)\)`) // Field represents a field that is built from the Terraform schema. @@ -39,6 +45,9 @@ type Field struct { TransformedName string SelectorName string Identifier bool + // Injected is set if this Field is an injected field to the Terraform + // schema as an object list map key for server-side apply merges. + Injected bool } // getDocString tries to extract the documentation string for the specified @@ -152,7 +161,97 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, f.FieldType = fieldType f.InitType = initType - return f, nil + AddServerSideApplyMarkers(f) + return f, errors.Wrapf(AddServerSideApplyMarkersFromConfig(f, cfg), "cannot add the server-side apply merge strategy markers for the field") +} + +// 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) { + // for sensitive fields, we generate secret or secret key references + if f.Schema.Sensitive { + return + } + + 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[config.MapType](config.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 sets. + case schema.TypeString, schema.TypeBool, schema.TypeInt, schema.TypeFloat: + f.Comment.ServerSideApplyOptions.ListType = ptr.To[config.ListType](config.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? +} + +func setInjectedField(fp, k string, f *Field, s config.MergeStrategy) bool { + if fp != fmt.Sprintf("%s.%s", k, s.ListMergeStrategy.ListMapKeys.InjectedKey.Key) { + return false + } + + if s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue != "" { + f.Comment.KubebuilderOptions.Default = ptr.To[string](s.ListMergeStrategy.ListMapKeys.InjectedKey.DefaultValue) + } + f.TFTag = "-" // prevent serialization into Terraform configuration + f.Injected = true + return true +} + +func AddServerSideApplyMarkersFromConfig(f *Field, cfg *config.Resource) error { //nolint:gocyclo // Easier to follow the logic in a single function + // for sensitive fields, we generate secret or secret key references + if f.Schema.Sensitive { + return nil + } + fp := strings.ReplaceAll(strings.Join(f.TerraformPaths, "."), ".*.", ".") + fp = strings.TrimSuffix(fp, ".*") + for k, s := range cfg.ServerSideApplyMergeStrategies { + if setInjectedField(fp, k, f, s) || k != fp { + continue + } + switch f.Schema.Type { //nolint:exhaustive + case schema.TypeList, schema.TypeSet: + if s.ListMergeStrategy.MergeStrategy == "" || s.MapMergeStrategy != "" || s.StructMergeStrategy != "" { + return errors.Errorf(errFmtInvalidSSAConfiguration, k, "list", "ListMergeStrategy") + } + f.Comment.ServerSideApplyOptions.ListType = ptr.To[config.ListType](s.ListMergeStrategy.MergeStrategy) + if s.ListMergeStrategy.MergeStrategy != config.ListTypeMap { + continue + } + f.Comment.ServerSideApplyOptions.ListMapKey = make([]string, 0, len(s.ListMergeStrategy.ListMapKeys.Keys)+1) + f.Comment.ServerSideApplyOptions.ListMapKey = append(f.Comment.ServerSideApplyOptions.ListMapKey, s.ListMergeStrategy.ListMapKeys.Keys...) + if s.ListMergeStrategy.ListMapKeys.InjectedKey.Key != "" { + f.Comment.ServerSideApplyOptions.ListMapKey = append(f.Comment.ServerSideApplyOptions.ListMapKey, s.ListMergeStrategy.ListMapKeys.InjectedKey.Key) + } + if len(f.Comment.ServerSideApplyOptions.ListMapKey) == 0 { + return errors.Errorf(errFmtMissingListMapKeys, k) + } + case schema.TypeMap: + if s.MapMergeStrategy == "" || s.ListMergeStrategy.MergeStrategy != "" || s.StructMergeStrategy != "" { + return errors.Errorf(errFmtInvalidSSAConfiguration, k, "map", "MapMergeStrategy") + } + f.Comment.ServerSideApplyOptions.MapType = ptr.To[config.MapType](s.MapMergeStrategy) // better to have a copy of the strategy + default: + // currently the generated APIs do not contain embedded objects, embedded + // objects are represented as lists of max size 1. However, this may + // change in the future, i.e., we may decide to generate HCL lists of max + // size 1 as embedded objects. + return errors.Errorf(errFmtUnsupportedSSAField, k) + } + } + return nil } // NewSensitiveField returns a constructed sensitive Field object. @@ -235,9 +334,15 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // parameter field if it's not an observation (only) field, i.e. parameter. // // We do this only if tf tag is not set to "-" because otherwise it won't - // be populated from the tfstate. We typically set tf tag to "-" for - // sensitive fields which were replaced with secretKeyRefs. - if f.TFTag != "-" && !addToObservation { + // be populated from the tfstate. Injected fields are included in the + // observation because an associative-list in the spec should also be + // an associative-list in the observation (status). + // We also make sure that this field has not already been added to the + // observation type via an explicit resource configuration. + // We typically set tf tag to "-" for sensitive fields which were replaced + // with secretKeyRefs, or for injected fields into the CRD schema, + // which do not exist in the Terraform schema. + if (f.TFTag != "-" || f.Injected) && !addToObservation { r.addObservationField(f, field) } @@ -246,11 +351,11 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add f.TFTag = strings.TrimSuffix(f.TFTag, ",omitempty") } r.addParameterField(f, field) - r.addInitField(f, field, g.Package) + r.addInitField(f, field, g, typeNames.InitTypeName) } if f.Reference != nil { - r.addReferenceFields(g, typeNames.ParameterTypeName, f) + r.addReferenceFields(g, typeNames.ParameterTypeName, f, false) } // Note(lsviben): All fields are optional because observation fields are @@ -281,7 +386,8 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // isInit returns true if the field should be added to initProvider. // We don't add Identifiers, references or fields which tag is set to -// "-". +// "-" unless they are injected object list map keys for server-side apply +// merges. // // Identifiers as they should not be ignorable or part of init due // the fact being created for one identifier and then updated for another @@ -295,7 +401,7 @@ func (f *Field) AddToResource(g *Builder, r *resource, typeNames *TypeNames, add // an earlier step, so they cannot be included as well. Plus probably they // should also not change for Create and Update steps. func (f *Field) isInit() bool { - return !f.Identifier && f.Reference == nil && f.TFTag != "-" + return !f.Identifier && (f.TFTag != "-" || f.Injected) } func getDescription(s string) string { diff --git a/pkg/types/markers/kubebuilder.go b/pkg/types/markers/kubebuilder.go index 6b1c6e15..e1b386a2 100644 --- a/pkg/types/markers/kubebuilder.go +++ b/pkg/types/markers/kubebuilder.go @@ -12,6 +12,7 @@ type KubebuilderOptions struct { Required *bool Minimum *int Maximum *int + Default *string } func (o KubebuilderOptions) String() string { @@ -30,6 +31,9 @@ func (o KubebuilderOptions) String() string { if o.Maximum != nil { m += fmt.Sprintf("+kubebuilder:validation:Maximum=%d\n", *o.Maximum) } + if o.Default != nil { + m += fmt.Sprintf("+kubebuilder:default:=%s\n", *o.Default) + } return m } 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..bdd0d02a --- /dev/null +++ b/pkg/types/markers/ssa.go @@ -0,0 +1,43 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package markers + +import ( + "fmt" + + "github.com/crossplane/upjet/pkg/config" +) + +// 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 *config.ListType + ListMapKey []string + MapType *config.MapType + StructType *config.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..92c17eb8 --- /dev/null +++ b/pkg/types/markers/ssa_test.go @@ -0,0 +1,51 @@ +// 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" + + "github.com/crossplane/upjet/pkg/config" +) + +func TestServerSideApplyOptions(t *testing.T) { + cases := map[string]struct { + o ServerSideApplyOptions + want string + }{ + "MapType": { + o: ServerSideApplyOptions{ + MapType: ptr.To[config.MapType](config.MapTypeAtomic), + }, + want: "+mapType=atomic\n", + }, + "StructType": { + o: ServerSideApplyOptions{ + StructType: ptr.To[config.StructType](config.StructTypeAtomic), + }, + want: "+structType=atomic\n", + }, + "ListType": { + o: ServerSideApplyOptions{ + ListType: ptr.To[config.ListType](config.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) + } + }) + } +}