Issues with server side apply and Services

[shawkins]

What happened?

After creating a Service with server side apply, modifications to that service may prevent additional attempts at server side apply.

What did you expect to happen?

Ideally the additional server side apply's would be successful at asserting the desired state.

How can we reproduce it (as minimally and precisely as possible)?

kubectl apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - protocol: TCP
      port: 80
      targetPort: 9376
EOF

Change the protocol via a mechanism other than server side apply

kubectl patch service my-service --patch '{"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}}'

Then reapply:

kubectl apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - protocol: TCP
      port: 80
      targetPort: 9376
EOF

The error will be

The Service "my-service" is invalid: 
* spec.ports[0].name: Required value
* spec.ports[1].name: Required value

Implying that the merge key is being ignored and that apply is attempting to add another port, rather than modify the existing one.

Anything else we need to know?

The final step works as expected if using client side apply.

Kubernetes version

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.0", GitCommit:"09f825e2ac8ddedf8bbc6bc82ffc5520560788a0", GitTreeState:"clean", BuildDate:"2022-04-29T17:13:31Z", GoVersion:"go1.17.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.3", GitCommit:"c92036820499fedefec0f847e2054d824aea6cd1", GitTreeState:"clean", BuildDate:"2021-10-27T18:35:25Z", GoVersion:"go1.16.9", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

N/A - just minikube

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[aojea]

I think we have some issue to track all of these problems with service ports and SSA, @thockin you are more familiar than me with this, do you remember which?

[thockin]

A few things going on here:

1. When you mix SSA and non-SSA you can confuse the system.

2. Unfortunately, there's a known issue (for which we have not yet found an acceptable and compatible fix) with service ports which use the same port number. The client-side merge logic thinks that port (number) is the unique key, but it's not - it should have been port+protocol. You are tickling that specifically.

3. Given those, it seems that SSA is trying to merge the now-UDP port 80 with TCP port 80, in which case the error is correct - the ports need names.

Let's look a little more closely:

$ k delete svc my-service
Error from server (NotFound): services "my-service" not found

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - protocol: TCP
      port: 80
      targetPort: 9376
EOF
service/my-service serverside-applied

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "port": 80,
    "protocol": "TCP",
    "targetPort": 9376
  }
]

$ k get svc my-service -o json --show-managed-fields | jq .metadata.managedFields
[
  {
    "apiVersion": "v1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:ports": {
          "k:{\"port\":80,\"protocol\":\"TCP\"}": {
            ".": {},
            "f:port": {},
            "f:protocol": {},
            "f:targetPort": {}
          }
        },
        "f:selector": {}
      }
    },
    "manager": "kubectl",
    "operation": "Apply",
    "time": "2023-06-17T18:34:32Z"
  }
]

You can see that SSA knows one key of TCP 80.

$ k -v 9 patch service my-service --patch '{"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}'
I0617 11:40:47.188599   94077 loader.go:395] Config loaded from file:  /home/thockin/.kube/config
I0617 11:40:47.193193   94077 round_trippers.go:466] curl -v -XGET  -H "Accept: application/json" -H "User-Agent: kubectl/v1.28.0 (linux/amd64) kubernetes/7637fee" -H "Authorization: Bearer <masked>" 'https://34.27.8.27/api/v1/namespaces/default/services/my-service'
I0617 11:40:47.262844   94077 round_trippers.go:510] HTTP Trace: Dial to tcp:34.27.8.27:443 succeed
I0617 11:40:47.413121   94077 round_trippers.go:553] GET https://34.27.8.27/api/v1/namespaces/default/services/my-service 200 OK in 219 milliseconds
I0617 11:40:47.413145   94077 round_trippers.go:570] HTTP Statistics: DNSLookup 0 ms Dial 69 ms TLSHandshake 78 ms ServerProcessing 71 ms Duration 219 ms
I0617 11:40:47.413156   94077 round_trippers.go:577] Response Headers:
I0617 11:40:47.413168   94077 round_trippers.go:580]     Audit-Id: a6038884-1daa-4355-8d75-f5e8ee3fe764
I0617 11:40:47.413177   94077 round_trippers.go:580]     Cache-Control: no-cache, private
I0617 11:40:47.413184   94077 round_trippers.go:580]     Content-Type: application/json
I0617 11:40:47.413192   94077 round_trippers.go:580]     X-Kubernetes-Pf-Flowschema-Uid: bea7e864-f7f8-4e1b-88f4-47f04190b9d6
I0617 11:40:47.413200   94077 round_trippers.go:580]     X-Kubernetes-Pf-Prioritylevel-Uid: d2fc4a2f-cfbd-42a1-9956-3fbb6bd8475c
I0617 11:40:47.413208   94077 round_trippers.go:580]     Content-Length: 816
I0617 11:40:47.413216   94077 round_trippers.go:580]     Date: Sat, 17 Jun 2023 18:40:47 GMT
I0617 11:40:47.413409   94077 request.go:1188] Response Body: {"kind":"Service","apiVersion":"v1","metadata":{"name":"my-service","namespace":"default","uid":"ddcca2f0-8b8f-41b4-9e6b-d790df026f70","resourceVersion":"27525629","creationTimestamp":"2023-06-17T18:34:32Z","managedFields":[{"manager":"kubectl","operation":"Apply","apiVersion":"v1","time":"2023-06-17T18:34:32Z","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:ports":{"k:{\"port\":80,\"protocol\":\"TCP\"}":{".":{},"f:port":{},"f:protocol":{},"f:targetPort":{}}},"f:selector":{}}}}]},"spec":{"ports":[{"protocol":"TCP","port":80,"targetPort":9376}],"selector":{"app.kubernetes.io/name":"MyApp"},"clusterIP":"10.0.208.121","clusterIPs":["10.0.208.121"],"type":"ClusterIP","sessionAffinity":"None","ipFamilies":["IPv4"],"ipFamilyPolicy":"SingleStack","internalTrafficPolicy":"Cluster"},"status":{"loadBalancer":{}}}
I0617 11:40:47.413779   94077 request.go:1188] Request Body: {"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}
I0617 11:40:47.413833   94077 round_trippers.go:466] curl -v -XPATCH  -H "Accept: application/json" -H "Content-Type: application/strategic-merge-patch+json" -H "User-Agent: kubectl/v1.28.0 (linux/amd64) kubernetes/7637fee" -H "Authorization: Bearer <masked>" 'https://34.27.8.27/api/v1/namespaces/default/services/my-service?fieldManager=kubectl-patch'
I0617 11:40:47.631616   94077 round_trippers.go:553] PATCH https://34.27.8.27/api/v1/namespaces/default/services/my-service?fieldManager=kubectl-patch 200 OK in 217 milliseconds
I0617 11:40:47.631659   94077 round_trippers.go:570] HTTP Statistics: GetConnection 0 ms ServerProcessing 217 ms Duration 217 ms
I0617 11:40:47.631682   94077 round_trippers.go:577] Response Headers:
I0617 11:40:47.631704   94077 round_trippers.go:580]     Date: Sat, 17 Jun 2023 18:40:47 GMT
I0617 11:40:47.631725   94077 round_trippers.go:580]     Audit-Id: ba43c464-3a47-4269-a68c-bfe1f1cce7ed
I0617 11:40:47.631745   94077 round_trippers.go:580]     Cache-Control: no-cache, private
I0617 11:40:47.631765   94077 round_trippers.go:580]     Content-Type: application/json
I0617 11:40:47.631784   94077 round_trippers.go:580]     X-Kubernetes-Pf-Flowschema-Uid: bea7e864-f7f8-4e1b-88f4-47f04190b9d6
I0617 11:40:47.631804   94077 round_trippers.go:580]     X-Kubernetes-Pf-Prioritylevel-Uid: d2fc4a2f-cfbd-42a1-9956-3fbb6bd8475c
I0617 11:40:47.631824   94077 round_trippers.go:580]     Content-Length: 961
I0617 11:40:47.632644   94077 request.go:1188] Response Body: {"kind":"Service","apiVersion":"v1","metadata":{"name":"my-service","namespace":"default","uid":"ddcca2f0-8b8f-41b4-9e6b-d790df026f70","resourceVersion":"27526422","creationTimestamp":"2023-06-17T18:34:32Z","managedFields":[{"manager":"kubectl","operation":"Apply","apiVersion":"v1","time":"2023-06-17T18:34:32Z","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:selector":{}}}},{"manager":"kubectl-patch","operation":"Update","apiVersion":"v1","time":"2023-06-17T18:40:47Z","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{"f:ports":{"k:{\"port\":80,\"protocol\":\"UDP\"}":{".":{},"f:port":{},"f:protocol":{},"f:targetPort":{}}}}}}]},"spec":{"ports":[{"protocol":"UDP","port":80,"targetPort":9376}],"selector":{"app.kubernetes.io/name":"MyApp"},"clusterIP":"10.0.208.121","clusterIPs":["10.0.208.121"],"type":"ClusterIP","sessionAffinity":"None","ipFamilies":["IPv4"],"ipFamilyPolicy":"SingleStack","internalTrafficPolicy":"Cluster"},"status":{"loadBalancer":{}}}
service/my-service patched

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "port": 80,
    "protocol": "UDP",
    "targetPort": 9376
  }
]

$ k get svc my-service -o json --show-managed-fields | jq .metadata.managedFields
[
  {
    "apiVersion": "v1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:selector": {}
      }
    },
    "manager": "kubectl",
    "operation": "Apply",
    "time": "2023-06-17T18:34:32Z"
  },
  {
    "apiVersion": "v1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:ports": {
          "k:{\"port\":80,\"protocol\":\"UDP\"}": {
            ".": {},
            "f:port": {},
            "f:protocol": {},
            "f:targetPort": {}
          }
        }
      }
    },
    "manager": "kubectl-patch",
    "operation": "Update",
    "time": "2023-06-17T18:40:47Z"
  }
]

You can see not that the SSA managed fields info has been updated to UDP 80. So when you send a new SSA with TCP 80, it's a different key and tries to merge them. That produces two ports without names, which is an error.

@apelisse correct me if I got that wrong.

So what could have been better? Frankly, it's a little surprising that apply tries to merge those. The underlying reason is to allow multiple co-owners of things which don't need to be aware of each other. In the case of service ports that seems pretty unlikely (but not impossible - I can imagine uses for it). Still, this is another place where list-map semantics may not be right, and atomic may be better matched to how it is actually used.

@shawkins If you want to assert complete state, you should probably use replace instead of apply.

[shawkins]

I had made the wrong assumption that the client side apply behavior was correct. Ideally then providing both name and port should make the entry unique, but the following:

kubectl apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF

Won't work either. After the protocol modification re-applying gives you:

The Service "my-service" is invalid: spec.ports[1].name: Duplicate value: "my-port"

It still appears to be creating another port entry.

Here as well the same scenario works with client side apply, or with a strategic merge patch (with or without a port name).

@shawkins If you want to assert complete state, you should probably use replace instead of apply.

Ideally what we're looking to do is have the JOSDK https://github.com/java-operator-sdk/java-operator-sdk use server side apply to apply the desired state of resources. It's not necessarily that we're trying to assert the complete spec, just the part of it that we're managing. If there are corner cases that require

[thockin]

Trying to recreate your test, I get:

$ k delete svc my-service
Error from server (NotFound): services "my-service" not found

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - protocol: TCP
      port: 80
      targetPort: 9376
EOF
service/my-service serverside-applied

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "port": 80,
    "protocol": "TCP",
    "targetPort": 9376
  }
]

$ k patch service my-service --patch '{"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}'
service/my-service patched

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "port": 80,
    "protocol": "UDP",
    "targetPort": 9376
  }
]

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF
The Service "my-service" is invalid: spec.ports[0].name: Required value

This is a correct error (unfortunately) - adding a second port requires naming the first. We don't treat "" as a valid name (maybe we could but it would require some deep thinking).

If I make the port+proto be the same:

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "port": 80,
    "protocol": "UDP",
    "targetPort": 9376
  }
]

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: UDP
      port: 80
      targetPort: 9376
EOF
service/my-service serverside-applied

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "name": "my-port",
    "port": 80,
    "protocol": "UDP",
    "targetPort": 9376
  }
]

So I am not sure where the duplicate is coming in.

Ideally what we're looking to do is have the JOSDK https://github.com/java-operator-sdk/java-operator-sdk use server side apply to apply the desired state of resources.

And that involves modifying service ports? That's a little surprising to me. Unfortunately the port naming semantic is a little wonky when going from 1 to 2, in that name becomes required. I've had it on my low-prio backlog for a long time to investigate allowing one unnamed port and the REST require names, but it's not done.

Help me understand exactly what things you think you need to co-own in Service?

[shawkins]

So I am not sure where the duplicate is coming in.

Sorry I wasn't descriptive enough with the next reproducer, I'm saying start with the port name and number, then modify and try to re-apply:

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF

$ k patch service my-service --patch '{"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}'

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF

My understanding from what you were saying is that having both the port number and the name should be sufficient for server side apply to understand it is modifying the existing port. But it instead fails.

And that involves modifying service ports? That's a little surprising to me.

No this isn't a real world scenario (yet). What we'd like to understand for the JOSDK, and for the kubernetes fabric8 client, are what situations won't work well with server side apply. It will be surprising for users if their resource starts to reject modifications from a controller with a 422, when it used to work because effectively a replace was being performed.

Based upon what I'm seeing here I'd like to more broadly understand how server side apply handles anything where a merge key should be used. Just checked that it's also a problem with deployment container env variables - if an existing value is modified by something else, then running a server side apply to change the value back will fail.

[thockin]

When you do the 2nd apply, you are adding a new port, but using the same name as the UDP one.

$ k delete svc my-service
Error from server (NotFound): services "my-service" not found

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF
service/my-service serverside-applied

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "name": "my-port",
    "port": 80,
    "protocol": "TCP",
    "targetPort": 9376
  }
]

$ k patch service my-service --patch '{"spec": {"ports": [{"protocol": "UDP", "port": 80}]}}'
service/my-service patched

$ k get svc my-service -o json | jq '.spec.ports'
[
  {
    "name": "my-port",
    "port": 80,
    "protocol": "UDP",
    "targetPort": 9376
  }
]

$ k apply --server-side -f - <<EOF
apiVersion: v1
kind: Service
metadata:
  name: my-service
spec:
  selector:
    app.kubernetes.io/name: MyApp
  ports:
    - name: my-port
      protocol: TCP
      port: 80
      targetPort: 9376
EOF
The Service "my-service" is invalid: spec.ports[1].name: Duplicate value: "my-port"

My understanding from what you were saying is that having both the port number and the name should be sufficient for server side apply to understand it is modifying the existing port.

No, the port number and protocol is the unique key. Not name. Name also needs to be unique, but is allowed to be omitted in the case of exactly one port. I know that's a little finicky, but the distinction is important. "" is not a valid port name.

It will be surprising for users if their resource starts to reject modifications from a controller with a 422, when it used to work because effectively a replace was being performed

Understood. I think the case you have devised is both confusing (it seems like one port being managed, but it's 2) and powerful (it demonstrates how SSA allows multiple controllers to act independently, even when operating on the same list field).

Just checked that it's also a problem with deployment container env variables

Man, you're hitting all of the awkward cases :)

env is problematic for a different reason - it purports to be a map, but it's really not, and duplicates can be semantically meaningful. We're discussing these issues literally the last 2 weeks, and trying to figure out how to resolve them. These are among the oldest fields in the whole system, and predate SSA, strategic-merge patch, and even plain PATCH support. Because a lot of the validation is hand-written, it missed cases that probably should have been disallowed, but we can't go back and fix those because that might break users :(

[shawkins]

No, the port number and protocol is the unique key. Not name. Name also needs to be unique, but is allowed to be omitted in the case of exactly one port. I know that's a little finicky, but the distinction is important. "" is not a valid port name.

Ok, sorry I see you did previously say protocol+port, and then I swapped in name instead. I see for service ports the merging behavior has an open discussion #105610 and previous issues.

env is problematic for a different reason - it purports to be a map, but it's really not, and duplicates can be semantically meaningful.

Actually I missed that forcing conflicts is good enough to resolve that, so that seems fine.

So it seems like the confusion will be with fields that are similar to ports, such as topologySpreadConstraints (with multiple +listMapKey's):

kubectl apply --server-side -f - <<EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: pause-deployment
  labels:
    app: pause
spec:
  replicas: 0
  selector:
    matchLabels:
      app: pause
  template:
    metadata:
      labels:
        app: pause
    spec:
      topologySpreadConstraints:
      - maxSkew: 1
        topologyKey: zone
        whenUnsatisfiable: DoNotSchedule
        labelSelector:
          matchLabels:
            foo: bar
      containers:
      - name: pause
        image: registry.k8s.io/pause:3.1
EOF

Then modify to whenUnsatisfiable: ScheduleAnyway

If you try to do the server side apply again, you'll end up with 2 topologySpreadConstraints:

      - labelSelector:
          matchLabels:
            foo: bar
        maxSkew: 1
        topologyKey: zone
        whenUnsatisfiable: ScheduleAnyway
      - labelSelector:
          matchLabels:
            foo: bar
        maxSkew: 1
        topologyKey: zone
        whenUnsatisfiable: DoNotSchedule

But do the same thing with a strategic merge and you'll end up with just a single topologySpreadConstraint reverted back to the original. If I'm understanding you correctly server side apply is working as desired. That is if the patching entry does not have the same complex key, then add a new entry? It does seem like to be server side apply friendly these complex keys situations should be able to consider a simple key (name) instead.

Sorry to tack another thing on, but is it correct to say that you should not rely on defaults with server side apply as no management is considered over the default values?

[thockin]

We have a doc somewhere, but I just can't find it this mornign, wherein we started accumulating fields that suffer this. When I find it, I will post the list to #118261

topologySpreadConstraints is on the list, which totals about 10 fields (so far).

If I'm understanding you correctly server side apply is working as desired

Well, it's CORRECT but not helpful given that we have these problematic fields. We are working on how to best address them - almost certainly that will be some changes to SSA.

is it correct to say that you should not rely on defaults with server side apply as no management is considered over the default values?

Not sure what you mean here - clarify for me?

[shawkins]

Not sure what you mean here - clarify for me?

Let's say I create a deployment via server side apply and it doesn't specify an imagePullPolicy for one of the containers - I'm just relying on the default of IfNotPresent

    ...
    spec:
      topologySpreadConstraints:
      containers:
      - name: pause
        image: registry.k8s.io/pause:3.1

Then something else modifies the imagePullPolicy to Always. If I simply re-apply the original server side apply without the imagePullPolicy - that will not revert back the Always setting. Meaning effective defaults should be considered unmanaged, and you must include them explicitly to be managed.

I'm asking this to clearly set expectations for fabric8 users so that they don't implicitly expect the same behavior as replace.

[thockin]

@apelisse Can you answer this last part? It's very subtle :)

$ cat /tmp/d.yaml 
apiVersion: apps/v1
kind: Deployment
metadata:
  name: test-deploy
spec:
  replicas: 0
  selector:
    matchLabels:
      app: myapp
  template:
    metadata:
      labels:
        app: myapp
    spec:
      containers:
      - name: c
        image: thockin/iamyouare

$ k apply --server-side -f /tmp/d.yaml
deployment.apps/test-deploy serverside-applied

$ k get deploy -o json --show-managed-fields test-deploy | jq .metadata.managedFields
[
  {
    "apiVersion": "apps/v1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:spec": {
        "f:replicas": {},
        "f:selector": {},
        "f:template": {
          "f:metadata": {
            "f:labels": {
              "f:app": {}
            }
          },
          "f:spec": {
            "f:containers": {
              "k:{\"name\":\"c\"}": {
                ".": {},
                "f:image": {},
                "f:name": {}
              }
            }
          }
        }
      }
    },
    "manager": "kubectl",
    "operation": "Apply",
    "time": "2023-06-19T18:27:01Z"
  },
  {
    "apiVersion": "apps/v1",
    "fieldsType": "FieldsV1",
    "fieldsV1": {
      "f:metadata": {
        "f:annotations": {
          ".": {},
          "f:deployment.kubernetes.io/revision": {}
        }
      },
      "f:status": {
        "f:conditions": {
          ".": {},
          "k:{\"type\":\"Available\"}": {
            ".": {},
            "f:lastTransitionTime": {},
            "f:lastUpdateTime": {},
            "f:message": {},
            "f:reason": {},
            "f:status": {},
            "f:type": {}
          },
          "k:{\"type\":\"Progressing\"}": {
            ".": {},
            "f:lastTransitionTime": {},
            "f:lastUpdateTime": {},
            "f:message": {},
            "f:reason": {},
            "f:status": {},
            "f:type": {}
          }
        },
        "f:observedGeneration": {}
      }
    },
    "manager": "kube-controller-manager",
    "operation": "Update",
    "subresource": "status",
    "time": "2023-06-19T18:27:01Z"
  }
]

Defaulted fields are NOT considered in managed-fields, but I can't speak to why.

[apelisse]

We very specifically decided that defaults are not part of your "intent". I can see both sides of the argument (what if you wanted that value but just left it out because you know it will default to the right value). If you have a strong opinion about the field, include it in your manifest. For port/protocol types of key, we need to know the default to index the item in the list, so that's more of an exception.

[thockin]

Here's some notes on the SSA issue (which is not the same as this one): xref kubernetes-sigs/structured-merge-diff#234

For this issue, I am not sure if there's actually any action to be taken.

The apply action is not simply create-or-replace - it's a much more sophisticated resource co-ownership system. If you don't keep that in mind, it may not always do exactly what you expect it to do. Given that, I think the situation above (apply, patch, apply) is working as intended.

@shawkins does that feel wrong?

In hindsight, I wonder if "apply" was the right name for this. Perhaps we really wanted 2 operations - "set" (simple create-or-replace) and "merge" (what we call apply). I suspect that vast majority of issues reported with apply would be avoided because humans would use "set" almost all the time, and "merge" is much more clearly a variant of patch. I wonder if it is possible to steer towards a future where this is clearer.

[shawkins]

Here's some notes on the SSA issue (which is not the same as this one): xref kubernetes-sigs/structured-merge-diff#234

Thank you I'll review and probably link to that as well.

For this issue, I am not sure if there's actually any action to be taken.

There's not, I just need to make sure what the expected behavior and gotchas are.

@shawkins does that feel wrong?

Not necessarily. It's just going to take some effort to explain the key differences. I think we can boil this down to a few examples of important differences with replace (or the createOrReplace in fabric8) that users will need to keep in mind. We need to be also make sure that users understand that while it will mostly act like apply or a strategic merge patch if the resource exists - it isn't the same.

In hindsight, I wonder if "apply" was the right name for this. Perhaps we really wanted 2 operations - "set" (simple create-or-replace) and "merge" (what we call apply). I suspect that vast majority of issues reported with apply would be avoided because humans would use "set" almost all the time, and "merge" is much more clearly a variant of patch.

Yes and the merging aspect can be hard to understand - with the validation issues raised here and on kubernetes-sigs/structured-merge-diff#234 it does seem like there's room for a lot of mismatch between user intent and what the merging resolves to. It was surprising to me for example that port+protocol is a key, but name is not.

I wonder if it is possible to steer towards a future where this is clearer.

set is clear and merge with sufficient guidance is clear.

From the perspective of the java operator sdk I'm a little fuzzy on though on what should be done with situations where a server side apply is invalid, or you want to assert ownship over more than just what appears in your patch (manipulate the managedFields and issue PUTs as needed?).

[apelisse]

you want to assert ownship over more than just what appears in your patch

Can you give an example of that?

[thockin]

I'm a little fuzzy on though on what should be done with situations where a server side apply is invalid

We're working on painting ourselves out of that corner, to the extent that it should never be "invalid" but it might trigger a corner-case path.

[shawkins]

Can you give an example of that?

This is coming from the perspective or current java operator sdk handling. The current state of a resource is provided to the reconciliation method, and the developer chooses what of that is meaning to preserve. Typically this has been things like labels, annotations, secret/configmap data. From there they throw away the rest of the existing state when they overlay their desired state.

In a more complex scenario using that stategy one could maintain "full control" over the main container of a deployment and let something else manage any other containers. If users want to do something similar with server side apply, I think we are going to say that they must give up control over anything that isn't explicitly set on the main container.

We're working on painting ourselves out of that corner, to the extent that it should never be "invalid" but it might trigger a corner-case path.

That would be great. Having to know ahead of time all of the possible ways something could alter a resource in such a way as to make it invalid for additional server side applys seems daunting. Another example, again related to validation, is mentioned in #118519 (comment) - if you have an ingress:

keycloak]$ kubectl apply --server-side --force-conflicts -f - <<EOF
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: minimal-ingress
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /
spec:
  ingressClassName: nginx-example
  defaultBackend:
    service:
      name: some-service
      port:
        number: 80
  rules:
  - http:
      paths:
      - path: /testpath
        pathType: Prefix
        backend:
          service:
            name: test
            port:
              number: 80
EOF

Then edit the defaultBackend to use a port name instead, and re-apply the original server side apply you'll get an error:

The Ingress "minimal-ingress" is invalid: spec.defaultBackend: Invalid value: "": cannot set both port name & port number

Interestingly you don't get an error when the same thing is done with the port under the rules.

[apelisse]

Can you give an example of that?

This is coming from the perspective or current java operator sdk handling. The current state of a resource is provided to the reconciliation method, and the developer chooses what of that is meaning to preserve. Typically this has been things like labels, annotations, secret/configmap data. From there they throw away the rest of the existing state when they overlay their desired state.

I would say that's definitely not a use-case for server-side apply in general. With Server-Side Apply, you typically have an opinion about some fields that doesn't depend on the current state of the object itself. Anytime you need to "change" the object based on its current value, you should use a patch or an update (PUT).

It's ok to not use server-side apply everywhere, there are use-cases where it fits and some where it doesn't.

[shawkins]

It's ok to not use server-side apply everywhere, there are use-cases where it fits and some where it doesn't.

Indeed. We just need some guidance on what those situations are and how that could be presented to a developer working with fabric8 or the java operator sdk. Thank you both for following up on all of these issues / questions.

cc @csviri

[thockin]

WRT Ingress - Someone needs to make up their mind - is it number or name? SSA doesn't have a mutual-exclusion concept, so it's trying to "apply" the fields which are specified onto the existing state, so the error message is, again, correct. You've given conflicting information. We could manually make it so that setting one clears the other, but that's perilous for other reasons - e.g. "is that really what the user meat to do?"

[shawkins]

We could manually make it so that setting one clears the other, but that's perilous for other reasons - e.g. "is that really what the user meat to do?"

I accept that the error is correct, it just seems odd that the handling is inconsistent between the defaultBackend port and the rules port.

[thockin]

I agree - I'd have to go spelunking to figure out why, and I won't have time this week.

[neolit123]

/sig network

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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.