Server Side Apply has poor interaction with some fields zero values

[howardjohn]

When using client.Apply patch type, some APIs run into issues. In particular, Service.spec.ports.targetPort, although there are likely others.

Consider this example:

func TestApplyCR(t *testing.T) {
	k, err := kube.NewDefaultClient()
	if err != nil {
		t.Fatal(err)
	}
	c, err := client.New(k.RESTConfig(), client.Options{})
	if err != nil {
		t.Fatal(err)
	}
	for i := 0; i < 5; i++ {
		s := &corev1.Service{
			TypeMeta: metav1.TypeMeta{
				Kind:       "Service",
				APIVersion: "v1",
			},
			ObjectMeta: metav1.ObjectMeta{
				Name: "c-r",
				Namespace: "default",
			},
			Spec: corev1.ServiceSpec{
				Ports: []corev1.ServicePort{{
					Name:        "foo",
					Port:        80,
				}},
			},
		}
		if err := c.Patch(context.Background(), s, client.Apply, client.FieldOwner("test")); err != nil {
			t.Fatal(err)
		}
		time.Sleep(time.Second)
		cur := &corev1.Service{}
		c.Get(context.Background(), client.ObjectKeyFromObject(s), cur)
		t.Log(cur.ResourceVersion)
	}
}

The expectation is this creates the service on the first iteration, and is a NOP on future iterations.
In reality, its changing each time:

=== RUN   TestApplyCR
    deployment_test.go:77: 1272637
    deployment_test.go:77: 1272639
    deployment_test.go:77: 1272666
    deployment_test.go:77: 1272668
    deployment_test.go:77: 1272693

In a real controller, this change then is picked up by the watch, and triggers another Reconcile, which loops forever.

Its plausible to workaround this by doing complex diffing on the client side, but this breaks the UX goal of Ensure presented in #347, where a client can just declare they configuration they want and let the server figure it out.

The reason for this is due to the Service API: TargetPort intstr.IntOrString json:"targetPort,omitempty" protobuf:"bytes,4,opt,name=targetPort"``
TargetPort is never empty because its not a pointer, so it is always marshalled as 0.

This can be seen in the following test:

func TestMarshal(t *testing.T) {
	j, _ := json.MarshalIndent(&corev1.Service{
		Spec: corev1.ServiceSpec{
			Ports: []corev1.ServicePort{{
				Name:        "foo",
				Port:        80,
			}},
		},
	}, "", "  ")
	t.Log(string(j))
}

which outputs:

            "ports": [
              {
                "name": "foo",
                "port": 80,
                "targetPort": 0
              }
            ]

The new client-go native typed Apply support does not have this issue, as seen by the test below:

func TestApply(t *testing.T) {
	k, err := kube.NewDefaultClient()
	if err != nil {
		t.Fatal(err)
	}
	for i := 0; i < 5; i++ {
		res, err := k.CoreV1().Services("default").Apply(context.Background(), applycorev1.
			Service("test", "default").
			WithSpec(applycorev1.ServiceSpec().WithPorts(applycorev1.ServicePort().WithPort(80))),
			metav1.ApplyOptions{FieldManager: "test"})
		if err != nil {
			t.Fatal(err)
		}
		t.Log(res.ResourceVersion)
	}
}

The reason for this is the Apply code has a mirror of the API where each field is a pointer, thus omitempty actually leads to it not being set (TargetPort *intstr.IntOrString json:"targetPort,omitempty"`).

The request is to improve this experience. This could be done in a number of ways including...:

• Accepting the apply variants of objects
• Documenting how to safely use SSA - whether that is using the client-go library directly, or something else
• Document that these types of fields should be explicitly set to their default values. This is feasible for this particular instance but I am not sure how universal it is.
• ? probably other ways as well

PS: TypeMeta is required to explicitly be set which seems like something that can be inferred from the object type automatically.

[coderanger]

Yes, that is correct. This is why the new typed builder syntax exists. Also, it's not really related to controller-runtime. Your two options for Apply are Unstructureds or Typed Builders.

[howardjohn]

It would be great to document that as there are no examples in the library or docs on use Apply and the only examples that I can find are from an issue (#347) where the normal types (not unstructured or Typed Builders) are used.

[howardjohn]

Also as far as I can tell Typed Builders cannot be used directly with controller-runtime client, we would need to drop down directly into the client-go library which breaks the abstraction provided by controller-runtime?

[coderanger]

We're still working on improved Apply support. The typed builders are relatively new and have minimal support in the ecosystem (we only recently merged the ability to generate them in the first place). My standing recommendation is still templates+go:embed+Unstructured though. It's much easier to manage long term.

I'm going to close this out as this is not a bug we can address (nor can it be fixed in general).

/close

[k8s-ci-robot]

@coderanger: Closing this issue.

In response to this:

We're still working on improved Apply support. The typed builders are relatively new and have minimal support in the ecosystem (we only recently merged the ability to generate them in the first place). My standing recommendation is still templates+go:embed+Unstructured though. It's much easier to manage long term.

I'm going to close this out as this is not a bug we can address (nor can it be fixed in general).

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aw185176]

I think there is still something going on here... Using envtest binaries for k8s 1.20.2 I get the errors above, but Kind @ 1.20.7 I do not. Looking at the commit on k/k master, it doesn't appear to be in any 1.20.x tags at all.