Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config.Resource. ServerSideApplyMergeStrategies to configure the SSA Merge Strategies #308

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

ulucinar
Copy link
Collaborator

@ulucinar ulucinar commented Dec 5, 2023

Description of your changes

This is a follow-up PR for #301.

We would like to have the API server properly track the field owners of the objects in the lists of the upjet-generated APIs when the server-side apply (SSA) patch operations are used. We would like it to also be able to properly merge objects in these lists with SSA patches. This will help us in addressing issues similar to crossplane-contrib/provider-upjet-aws#975 when we specify a non-atomic SSA merge strategy for a list. Please see this comment for more context.

This PR extends upjet's resource configuration framework so that it's now possible to configure a map's or object list's SSA merge strategy and to specify the list map keys for object lists with the map merge strategy. Please note that the map merge strategy is not the best merge strategy for all object lists. For certain object lists, it's more desirable to replace the whole object instead of merging values specified by different clients, when merging the lists. This PR allows the configuration of the SSA merge strategy as one of the allowed values of map, set or atomic for object lists, or granular or atomic for maps.

This PR allows injection of a naming (index) field into the object schemas of a list being configured with a merge strategy of map. This injected field will be available in the generated CRDs and thus visible to the API server (so that it can properly track field ownership for the list's objects and merge the lists and the objects as intended) but it's removed when the objects are being passed down to the Terraform layer (as these injected fields are not known to the underlying Terraform provider).

The list map keys specified in the OpenAPI SSA extensions for an associative list are not nullable, they must either be required or defaulted. While configuring an injected field for the SSA merge strategy configuration, one can also specify a default value for an injected field, which will result in a +kubebuilder:default marker to be added to the generated Go struct field. This is useful when configuring the SSA merge strategy for object lists with a max size constraint of 1. However, object lists with a size greater than 1 will not be able to utilize this configuration option and will need to delegate the defaulting or the specification of a value for the injected field to some other entity such as:

  • The user of the API, if we are fine with marking the injected field as required. Please note that this PR currently does not support this. The injected fields are generated as optional CRD parameters that needs some means of defaulting. In the future, we may decide to add a new configuration option for those injected fields that will mark them as required in the generated CRDs as associative lists are common in Kubernetes APIs. But please note that we are not authoring the APIs of interest manually and they are being generated from the schemas of the underlying Terraform providers. But still, I believe for certain object list APIs, it could make sense to require the user to name the objects they specify in those lists, deviating from the underlying Terraform resource API.
  • A defaulting webhook. Please note that for the API server to be able to properly track the field ownership and to properly merge lists, the index set (which contains the injected field in the context of this discussion) must uniquely identify the items in the list. This implies two things: Index sets of two list objects must differ if the objects differ and the index sets of two equal objects must be equal to each other. The equality condition in the second clause does not consider the injected field(s) because the semantics of the object API does not involve these injected fields. Thus, while it would be lexically correct to have two distinct object list items {"host": "a.b.c", interfaces: ["x", "y"], injected:"0"} and {"host": "a.b.c", interfaces: ["x", "y"], injected:"1"}, semantically these two objects are equal because they are the same host and they designate the same set of network interfaces to be attached to this host. In order to implement SSA field manager tracking and list merging properly, the injected index fields (injected in the example JSON objects) for these two objects should be the same. Please note that these examples deliberately have the non-scalar field interfaces that are used to uniquely identify the list objects, so that we cannot designate the SSA list map keys to be {host, interfaces}. For another object schema like {"host": "a.b.c", "port": 443}, where all the identifying fields are scalars, we may not need to inject any new indexing fields as the already existing scalar fields could constitute the index set if they are all not nullable (required or defaulted).

An example configuration looks like as follows:

p.AddResourceConfigurator("aws_eks_cluster", func(r *config.Resource) {
  r.ServerSideApplyMergeStrategies["vpc_config"] = config.MergeStrategy{
			  ListMergeStrategy: config.ListMergeStrategy{
				  MergeStrategy: config.ListTypeMap,
				  ListMapKeys: config.ListMapKeys{
					  InjectedKey: config.InjectedKey{
						  Key:          "index",
						  DefaultValue: `"0"`,
					  },
				  },
			  },
		  }
}

Because the spec.forProvider.vpcConfig field of the Cluster.eks resource is an object list of max length 1 (actually an embedded object), this configuration injects an indexing field and defaults its value to "0". Please note that vpcConfig list item schema does not have an existing index set available to it and this is the reason we configure field injection to support SSA patch operations on this list.

Upjet checks the supplied SSA merge strategy configuration and if interrupts the code generation pipelines of the supplied configuration is not compatible with the underlying resource schema, for example, when a map strategy configuration is supplied for an object list.

This PR moves the merge strategy related types from the markers package to the config package to prevent the import cycle markers -> config -> markers as the markers package already uses the config.Reference. Making the merge strategies configurable via upjet's configuration framework, it makes more sense to move the merge strategy related types to the config package.

We may also consider automatically injecting SSA merge strategy markers on top of the configuration framework being introduced with this PR. This would allow us to override/remove any merge strategy configuration that's done by upjet on the provider side.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

This PR has been tested with custom builds of upbound/provider-aws and the configurations upbound/configuration-aws-eks and upbound/configuration-aws-network in the context of crossplane-contrib/provider-upjet-aws#975. The AWS family provider packages used in these tests are index.docker.io/ulucinar/provider-aws-{ec2, iam, eks}:v0.45.0-c914fe18a92011992e32630ca4d0dba4a8f36242 and the configuration packages are index.docker.io/ulucinar/configuration-aws-{network, eks}:v0.45.0-c914fe18a92011992e32630ca4d0dba4a8f36242. We also need changes in the crossplane-runtime (subject of a separate PR) so that the MR reconciler uses SSA patch operations while setting the resolved references (in the context of these tests, the spec.forProvider.vpcConfig[0].subnetIds field) and changes in the XR reconciler so that the XR reconciler uses SSA patch operations while applying the composed resources from the compositions of the configuration package upbound/configuration-aws-eks.

@ulucinar ulucinar force-pushed the ssa-object-lists branch 3 times, most recently from 186dfd7 to ef45303 Compare December 6, 2023 13:07
…igure

the server-side apply merge strategies for object lists & maps.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar changed the title Add config.Resource.ServerSideApplyListMapKeys to configure the list map keys Add config.Resource. ServerSideApplyMergeStrategies to configure the SSA Merge Strategies Dec 20, 2023
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ulucinar LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants