Use server-side apply patch to update the resolved cross-resource references #623

ulucinar · 2023-12-07T16:08:15Z

Description of your changes

This PR proposes to use server-side apply patch operations in managed.APISimpleReferenceResolver when updating the resolved cross-resource references. This will enable us to address the issue reported in crossplane-contrib/provider-upjet-aws#975 by specifying the SSA merge strategy for the object list spec.forProvider.vpcConfig and with the root cause described in detail here.

An example patch document computed while updating the resolved VPC ID for the following manifests:

apiVersion: ec2.aws.upbound.io/v1beta1
kind: VPC
metadata:
  name: sample-vpc-alper-1
  labels:
    app: test
spec:
  initProvider:
    cidrBlock: 172.16.0.0/16
    tags:
      a: b
  forProvider:
    region: us-west-1
    tags:
      Name: DemoVpc

---

apiVersion: ec2.aws.upbound.io/v1beta1
kind: Subnet
metadata:
  name: sample-subnet-alper-1
spec:
  forProvider:
    region: us-west-1
    availabilityZone: us-west-1b
    cidrBlock: 172.16.10.0/24
    vpcIdSelector:
      matchLabels:
        app: test

is:

{
  "apiVersion": "ec2.aws.upbound.io/v1beta1",
  "kind": "Subnet",
  "spec": {
    "forProvider": {
      "vpcId": "vpc-08f590a352d767c2f",
      "vpcIdRef": {
        "name": "sample-vpc-alper-1"
      }
    }
  }
}

And the resulting managed field object is:

{
    "apiVersion": "ec2.aws.upbound.io/v1beta1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:forProvider": {
          "f:vpcId": {},
          "f:vpcIdRef": {
            "f:name": {}
          }
        }
      }
    },
    "manager": "managed.crossplane.io/api-simple-reference-resolver",
    "operation": "Apply",
    "time": "2023-12-07T14:51:26Z"
  }

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested via a custom build of upbound/provider-aws with the above manifests and also with the configuration packages 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.

turkenh · 2023-12-08T07:46:44Z

pkg/reconciler/managed/api.go

+	defer existing.GetObjectKind().SetGroupVersionKind(existing.GetObjectKind().GroupVersionKind())
+	existing.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{})


Sorry, I couldn't get the reasoning behind these two lines. What happens if we didn't have both?

Hi @turkenh,
The deferred resetting in line 149 is currently not needed and we can remove it. I felt like the function is actually not expected to modify the object existing which is generally a pointer type and instead of deep copying and modifying the copy, I preferred the much lighter set/reset pattern. Currently, the caller does not use existing after it calls this function but if we attempt to do so assuming existing is unmodified, that assumption now holds because we reset the GVK to its original value.

In line 150, we set the GVK to its zero value because in the calculated patch document we need the type meta as in:

{ "apiVersion": "ec2.aws.upbound.io/v1beta1", "kind": "Subnet", ...

I will leave a comment explaining this.

I see, thanks!
Another idea could be setting those two fields over the generated patch to make it more obvious.

turkenh · 2023-12-08T07:55:47Z

pkg/reconciler/managed/api.go

@@ -151,7 +181,11 @@ func (a *APISimpleReferenceResolver) ResolveReferences(ctx context.Context, mg r
 		return nil
 	}

-	return errors.Wrap(a.client.Update(ctx, mg), errUpdateManaged)
+	patch, err := prepareJSONMerge(existing, mg)


The reason why can simply go with a JSON merge patch here is because we don't have a case where a resolved reference is dropped/removed by resolver later. Is this correct?

Yes, correct. The idea is that we mark the target object list which contains the reference fields with the SSA merge strategy markers. If we use JSON merge patch operation instead of the SSA patch operation, then for example for object lists, the API server cannot properly merge the objects in the list and updates the object as a whole. When both controllers (e.g., the managed reconciler & the XR reconciler) use SSA, then when we specify an SSA merge strategy of map (together with the list map keys) for the object list, then the API server has a chance to properly merge the list object. For example, when a composition just patches the label selectors for a cross-resource reference, the API server can properly merge only the intended label selector field and not overwrite the reference target fields (e.g., spec.forProvider.vpcConfig[0].subnetIds in the case of crossplane-contrib/provider-upjet-aws#975) assuming the managed reconciler also uses SSA.

turkenh

Looking good, thanks @ulucinar 🙌

turkenh · 2023-12-08T08:43:55Z

pkg/reconciler/managed/api.go

+	defer existing.GetObjectKind().SetGroupVersionKind(existing.GetObjectKind().GroupVersionKind())
+	existing.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{})


I see, thanks!
Another idea could be setting those two fields over the generated patch to make it more obvious.

…erences in managed.APISimpleReferenceResolver. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar · 2023-12-11T12:45:22Z

I've re-validated this PR using the following packages against upbound/configuration-aws-eks:

index.docker.io/ulucinar/configuration-aws-eks:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/configuration-aws-network:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/provider-aws-ec2:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/provider-aws-eks:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/provider-aws-iam:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/provider-family-aws:v0.46.0-5e03e3f09730791cd3bc035b00e0b9e349bdfe32
index.docker.io/ulucinar/crossplane:v1.15.0-d80762842f59fc4c8f2fe2222f51bce42f6e8f45 -> Containing SSA-related changes for the XR reconciler while applying the composed resources.

With the SSA changes in the XR reconciler (so that it also uses SSA patch operations for applying the composed resources), the relevant field owners are now as follows:

{
    "apiVersion": "eks.aws.upbound.io/v1beta1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:metadata": {
        "f:annotations": {
          "f:crossplane.io/composition-resource-name": {}
        },
        "f:generateName": {},
        "f:labels": {
          "f:crossplane.io/claim-name": {},
          "f:crossplane.io/claim-namespace": {},
          "f:crossplane.io/composite": {}
        },
        "f:ownerReferences": {
          "k:{\"uid\":\"a3e0b3ca-cd2b-4e34-a1dc-da53c11f2000\"}": {}
        }
      },
      "f:spec": {
        "f:deletionPolicy": {},
        "f:forProvider": {
          "f:region": {},
          "f:roleArnSelector": {
            "f:matchControllerRef": {},
            "f:matchLabels": {
              "f:role": {}
            }
          },
          "f:version": {},
          "f:vpcConfig": {
            "k:{\"index\":\"0\"}": {
              ".": {},
              "f:endpointPrivateAccess": {},
              "f:endpointPublicAccess": {},
              "f:subnetIdSelector": {
                "f:matchLabels": {
                  "f:access": {},
                  "f:networks.aws.platform.upbound.io/network-id": {}
                }
              }
            }
          }
        },
        "f:providerConfigRef": {
          "f:name": {}
        }
      }
    },
    "manager": "apiextensions.crossplane.io/composed",
    "operation": "Apply",
    "time": "2023-12-11T12:26:53Z"
  },
  {
    "apiVersion": "eks.aws.upbound.io/v1beta1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:forProvider": {
          "f:roleArn": {},
          "f:roleArnRef": {
            "f:name": {}
          },
          "f:vpcConfig": {
            "k:{\"index\":\"0\"}": {
              ".": {},
              "f:endpointPrivateAccess": {},
              "f:endpointPublicAccess": {},
              "f:index": {},
              "f:subnetIdRefs": {},
              "f:subnetIdSelector": {
                "f:matchLabels": {
                  "f:access": {},
                  "f:networks.aws.platform.upbound.io/network-id": {}
                }
              },
              "f:subnetIds": {
                "v:\"subnet-0c8586b9c01ff4ff8\"": {},
                "v:\"subnet-0e907805a42eead8d\"": {}
              }
            }
          }
        }
      }
    },
    "manager": "managed.crossplane.io/api-simple-reference-resolver",
    "operation": "Apply",
    "time": "2023-12-11T12:27:06Z"
  }

, where field manager apiextensions.crossplane.io/composed is the XR reconciler.

The alternation behavior reported in crossplane-contrib/provider-upjet-aws#975 on spec.forProvider.vpcConfig[0].subnetIds is also no longer observed as the following watch output (started before the object exists and watched until after the XR is ready) shows:

["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]
["subnet-0c8586b9c01ff4ff8","subnet-0e907805a42eead8d"]

ulucinar requested review from a team as code owners December 7, 2023 16:08

ulucinar requested review from bobh66 and jbw976 December 7, 2023 16:08

turkenh reviewed Dec 8, 2023

View reviewed changes

turkenh approved these changes Dec 8, 2023

View reviewed changes

Use server-side apply patch to update the resolved cross-resource ref…

f1be5e6

…erences in managed.APISimpleReferenceResolver. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar force-pushed the ssa-simple-api-resolver branch from e9321bc to f1be5e6 Compare December 8, 2023 12:05

turkenh merged commit 8d14356 into crossplane:master Dec 11, 2023
8 checks passed

ulucinar mentioned this pull request Dec 27, 2023

Generate reference fields for the spec.initProvider of all resources crossplane-contrib/provider-upjet-aws#1046

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use server-side apply patch to update the resolved cross-resource references #623

Use server-side apply patch to update the resolved cross-resource references #623

ulucinar commented Dec 7, 2023

turkenh Dec 8, 2023

ulucinar Dec 8, 2023

turkenh Dec 8, 2023

turkenh Dec 8, 2023

ulucinar Dec 8, 2023 •

edited

Loading

turkenh left a comment

turkenh Dec 8, 2023

ulucinar commented Dec 11, 2023 •

edited

Loading

		defer existing.GetObjectKind().SetGroupVersionKind(existing.GetObjectKind().GroupVersionKind())
		existing.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{})

Use server-side apply patch to update the resolved cross-resource references #623

Use server-side apply patch to update the resolved cross-resource references #623

Conversation

ulucinar commented Dec 7, 2023

Description of your changes

How has this code been tested

turkenh Dec 8, 2023

Choose a reason for hiding this comment

ulucinar Dec 8, 2023

Choose a reason for hiding this comment

turkenh Dec 8, 2023

Choose a reason for hiding this comment

turkenh Dec 8, 2023

Choose a reason for hiding this comment

ulucinar Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

turkenh left a comment

Choose a reason for hiding this comment

turkenh Dec 8, 2023

Choose a reason for hiding this comment

ulucinar commented Dec 11, 2023 • edited Loading

ulucinar Dec 8, 2023 •

edited

Loading

ulucinar commented Dec 11, 2023 •

edited

Loading