kubectl apply --server-side --dry-run=server can duplicate field in an atomic list

[apelisse]

Apparently, the bug happens in 1.26.3 but not in 1.27.1 so maybe it's been fixed somewhere.

Here are the steps to reproduce:

cat > testhpa.yaml <<EOF
apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
  name: test-hpa
  namespace: default
spec:
  maxReplicas: 5
  metrics:
  - external:
      metric:
        name: foo
      target:
        averageValue: "10"
        type: AverageValue
    type: External
  minReplicas: 1
  scaleTargetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: test-depl
EOF

kubectl apply --server-side=true -f testhpa.yaml
kubectl apply --dry-run=server --server-side=true -o yaml -f testhpa.yaml

Note that the metrics list is atomic, so the list should be entirely replaced every-time, not merged/appended.
https://github.com/kubernetes/kubernetes/blob/v1.27.2/staging/src/k8s.io/api/autoscaling/v2/types.go#L67-L77

/wg api-expression
/triage accepted
/priority important-soon

[apelisse]

I tried to add a printf of the json'd merged object right before leaving the ssa code, and the field isn't duplicated. So something else must be trying to merge somehow!?
https://github.com/kubernetes/kubernetes/blob/v1.26.3/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go#L262

[apelisse]

I also checked that it's not kubectl doing anything crazy, the response from the server includes the duplicated field.

[apelisse]

Checked both before and after admission, object seems to be properly merged.

[apelisse]

Ah, something not mentioned in the bug, applying without dry-run doesn't trigger the same behavior.

[liggitt]

storage version changed to v2 in 1.27 (#114358)

HPA conversion is really weird (taking v2 fields and putting them in annotations in v1, then shipping annotations back into appended items when converting back to v2)... I'd suspect something in the storage → conversion path

[apelisse]

Right, the exact conversions that we do are different in dry-run vs non-dry-run because we have to storage version. I'm still investigating the conversion path to see if anything seems shady and I've made progress on that.

[apelisse]

OK, this is the trace that leads to the internal HorizontalPodAutoscaler object to have the metrics field duplicated is this one:

k8s.io/kubernetes/pkg/apis/autoscaling/v1.Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(0x70000000100d8e7?, 0xc0020908c0, {0x64e47e0, 0xc0058ba9e0})
	pkg/apis/autoscaling/v1/conversion.go:460 +0x769
k8s.io/kubernetes/pkg/apis/autoscaling/v1.RegisterConversions.func38({0x5a9a580?, 0xc00f6e0900?}, {0x5a58660?, 0xc0020908c0?}, {0x64e47e0?, 0xc0058ba9e0?})
	pkg/apis/autoscaling/v1/zz_generated.conversion.go:229 +0x65
k8s.io/apimachinery/pkg/conversion.(*Converter).Convert(0xc000226b10, {0x5a9a580, 0xc00f6e0900}, {0x5a58660, 0xc0020908c0}, 0xc0058ba9d0)
	vendor/k8s.io/apimachinery/pkg/conversion/converter.go:210 +0x418
k8s.io/apimachinery/pkg/runtime.(*Scheme).Convert(0xc00036e3f0, {0x5a9a580, 0xc00f6e0900}, {0x5a58660, 0xc0020908c0}, {0x56619c0?, 0xc001cd24c0})
	vendor/k8s.io/apimachinery/pkg/runtime/scheme.go:420 +0x1b6
k8s.io/apimachinery/pkg/runtime/serializer/versioning.(*codec).Decode(0xc001ccc1e0, {0xc0068cfa00?, 0x1f1?, 0x200?}, 0x64c9420?, {0x64e8b88, 0xc0020908c0})
	vendor/k8s.io/apimachinery/pkg/runtime/serializer/versioning/versioning.go:178 +0x647
k8s.io/apiserver/pkg/registry/generic/registry.(*DryRunnableStorage).copyInto(0xc001cf4238, {0x64e8b88, 0xc0023f3180}, {0x64e8b88, 0xc0020908c0})
	vendor/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go:107 +0xa6
k8s.io/apiserver/pkg/registry/generic/registry.(*DryRunnableStorage).GuaranteedUpdate(0xc001cf4238, {0x65027b8?, 0xc0063009c0?}, {0xc00f511cb0, 0x2a}, {0x64e8b88, 0xc0020908c0}, 0xd?, 0x5b02eb6?, 0xc00a1ba840, ...)
	vendor/k8s.io/apiserver/pkg/registry/generic/registry/dryrun.go:91 +0x169
k8s.io/apiserver/pkg/registry/generic/registry.(*Store).Update(0xc001cf4140, {0x65027b8?, 0xc0063009c0}, {0xc00b574c41, 0x8}, {0x64e51e0?, 0xc006300b10}, 0xc00462c6e0, 0xc00f6f1200, 0x1, ...)
	vendor/k8s.io/apiserver/pkg/registry/generic/registry/store.go:543 +0x508
k8s.io/apiserver/pkg/endpoints/handlers.(*patcher).patchResource.func2()
	vendor/k8s.io/apiserver/pkg/endpoints/handlers/patch.go:665 +0xa7
k8s.io/apiserver/pkg/endpoints/handlers.(*patcher).patchResource.func3()
	vendor/k8s.io/apiserver/pkg/endpoints/handlers/patch.go:671 +0x38
k8s.io/apiserver/pkg/endpoints/handlers/finisher.finishRequest.func1()
	vendor/k8s.io/apiserver/pkg/endpoints/handlers/finisher/finisher.go:117 +0x8f
created by k8s.io/apiserver/pkg/endpoints/handlers/finisher.finishRequest
	vendor/k8s.io/apiserver/pkg/endpoints/handlers/finisher/finisher.go:92 +0xde

So definitely in the path to dry-run.

The following test reproduces the error, as it fails with two identical metrics.

in := &autoscalingv1.HorizontalPodAutoscaler{
	ObjectMeta: metav1.ObjectMeta{
		Annotations: map[string]string{
			"autoscaling.alpha.kubernetes.io/metrics": "[{\"type\":\"External\",\"external\":{\"metricName\":\"foo\",\"targetAverageValue\":\"10\"}}]",
		},
	},
	Spec: autoscalingv1.HorizontalPodAutoscalerSpec{
		MinReplicas: utilpointer.Int32Ptr(1),
		MaxReplicas: 5,
	},
}
out := &autoscaling.HorizontalPodAutoscaler{
	Spec: autoscaling.HorizontalPodAutoscalerSpec{
		Metrics: []autoscaling.MetricSpec{
			{
				Type: autoscaling.ExternalMetricSourceType,
				External: &autoscaling.ExternalMetricSource{
					Target: autoscaling.MetricTarget{
						Type:         autoscaling.AverageValueMetricType,
						AverageValue: resource.NewMilliQuantity(10, resource.DecimalSI),
					},
				},
			},
		},
	},
}
expectOut := &autoscaling.HorizontalPodAutoscaler{
	Spec: autoscaling.HorizontalPodAutoscalerSpec{
		MinReplicas: utilpointer.Int32Ptr(1),
		MaxReplicas: 5,
		Metrics: []autoscaling.MetricSpec{
			{
				Type: autoscaling.ExternalMetricSourceType,
				External: &autoscaling.ExternalMetricSource{
					Target: autoscaling.MetricTarget{
						Type:         autoscaling.AverageValueMetricType,
						AverageValue: resource.NewMilliQuantity(10, resource.DecimalSI),
					},
				},
			},
		},
	},
}
if err := Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(in, out, nil); err != nil {
	t.Errorf("Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler() error = %v", err)
}
assert.Equal(t, expectOut, out)

Problem is probably that 1. the "out" object isn't cleared before copyInto for dry-run. 2. conversion doesn't detect the duplication/remove previous entry from out. I'll check to see how the non-dry-run version clears the out object.

[ethan-gallant]

Did anyone find a workaround for this bug? Currently trying to compare equality between two objects after a dry-run to populate default fields.