Skip to content

Commit a78cd08

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#89965 from liggitt/automated-cherry-pick-of-#89963-upstream-release-1.18
Automated cherry pick of kubernetes#89963: Drop round-trip annotations in HPA conversion
2 parents c08ae87 + 0d2cdc3 commit a78cd08

File tree

12 files changed

+451
-81
lines changed

12 files changed

+451
-81
lines changed

‎pkg/apis/autoscaling/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ go_library(
1010
srcs = [
1111
"annotations.go",
1212
"doc.go",
13+
"helpers.go",
1314
"register.go",
1415
"types.go",
1516
"zz_generated.deepcopy.go",

‎pkg/apis/autoscaling/helpers.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
Copyright 2020 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package autoscaling
18+
19+
// DropRoundTripHorizontalPodAutoscalerAnnotations removes any annotations used to serialize round-tripped fields from later API versions,
20+
// and returns false if no changes were made and the original input object was returned.
21+
// It should always be called when converting internal -> external versions, prior
22+
// to setting any of the custom annotations:
23+
//
24+
// annotations, copiedAnnotations := DropRoundTripHorizontalPodAutoscalerAnnotations(externalObj.Annotations)
25+
// externalObj.Annotations = annotations
26+
//
27+
// if internal.SomeField != nil {
28+
// if !copiedAnnotations {
29+
// externalObj.Annotations = DeepCopyStringMap(externalObj.Annotations)
30+
// copiedAnnotations = true
31+
// }
32+
// externalObj.Annotations[...] = json.Marshal(...)
33+
// }
34+
func DropRoundTripHorizontalPodAutoscalerAnnotations(in map[string]string) (out map[string]string, copied bool) {
35+
_, hasMetricsSpecs := in[MetricSpecsAnnotation]
36+
_, hasBehaviorSpecs := in[BehaviorSpecsAnnotation]
37+
_, hasMetricsStatuses := in[MetricStatusesAnnotation]
38+
_, hasConditions := in[HorizontalPodAutoscalerConditionsAnnotation]
39+
if hasMetricsSpecs || hasBehaviorSpecs || hasMetricsStatuses || hasConditions {
40+
out = DeepCopyStringMap(in)
41+
delete(out, MetricSpecsAnnotation)
42+
delete(out, BehaviorSpecsAnnotation)
43+
delete(out, MetricStatusesAnnotation)
44+
delete(out, HorizontalPodAutoscalerConditionsAnnotation)
45+
return out, true
46+
}
47+
return in, false
48+
}
49+
50+
// DeepCopyStringMap returns a copy of the input map.
51+
// If input is nil, an empty map is returned.
52+
func DeepCopyStringMap(in map[string]string) map[string]string {
53+
out := make(map[string]string, len(in))
54+
for k, v := range in {
55+
out[k] = v
56+
}
57+
return out
58+
}

‎pkg/apis/autoscaling/v1/BUILD

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,13 @@ go_test(
3030
embed = [":go_default_library"],
3131
deps = [
3232
"//pkg/api/legacyscheme:go_default_library",
33+
"//pkg/apis/autoscaling:go_default_library",
3334
"//pkg/apis/autoscaling/install:go_default_library",
3435
"//pkg/apis/core/install:go_default_library",
3536
"//staging/src/k8s.io/api/autoscaling/v1:go_default_library",
37+
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
3638
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
39+
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
3740
"//vendor/k8s.io/utils/pointer:go_default_library",
3841
],
3942
)

‎pkg/apis/autoscaling/v1/conversion.go

Lines changed: 54 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,10 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
260260
return err
261261
}
262262

263+
// clear any pre-existing round-trip annotations to make sure the only ones set are ones we produced during conversion
264+
annotations, copiedAnnotations := autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
265+
out.Annotations = annotations
266+
263267
otherMetrics := make([]autoscalingv1.MetricSpec, 0, len(in.Spec.Metrics))
264268
for _, metric := range in.Spec.Metrics {
265269
if metric.Type == autoscaling.ResourceMetricSourceType && metric.Resource != nil && metric.Resource.Name == core.ResourceCPU && metric.Resource.Target.AverageUtilization != nil {
@@ -289,19 +293,16 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
289293
}
290294
}
291295

292-
if len(otherMetrics) > 0 || len(in.Status.CurrentMetrics) > 0 || len(currentConditions) > 0 || in.Spec.Behavior != nil {
293-
old := out.Annotations
294-
out.Annotations = make(map[string]string, len(old)+4)
295-
for k, v := range old {
296-
out.Annotations[k] = v
297-
}
298-
}
299-
300296
if len(otherMetrics) > 0 {
301297
otherMetricsEnc, err := json.Marshal(otherMetrics)
302298
if err != nil {
303299
return err
304300
}
301+
// copy before mutating
302+
if !copiedAnnotations {
303+
copiedAnnotations = true
304+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
305+
}
305306
out.Annotations[autoscaling.MetricSpecsAnnotation] = string(otherMetricsEnc)
306307
}
307308

@@ -310,14 +311,25 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
310311
if err != nil {
311312
return err
312313
}
314+
// copy before mutating
315+
if !copiedAnnotations {
316+
copiedAnnotations = true
317+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
318+
}
313319
out.Annotations[autoscaling.MetricStatusesAnnotation] = string(currentMetricsEnc)
314320
}
315321

316322
if in.Spec.Behavior != nil {
323+
// TODO: this is marshaling an internal type. Fix this without breaking backwards compatibility.
317324
behaviorEnc, err := json.Marshal(in.Spec.Behavior)
318325
if err != nil {
319326
return err
320327
}
328+
// copy before mutating
329+
if !copiedAnnotations {
330+
copiedAnnotations = true
331+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
332+
}
321333
out.Annotations[autoscaling.BehaviorSpecsAnnotation] = string(behaviorEnc)
322334
}
323335

@@ -326,6 +338,11 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v1_HorizontalPodAutoscaler(i
326338
if err != nil {
327339
return err
328340
}
341+
// copy before mutating
342+
if !copiedAnnotations {
343+
copiedAnnotations = true
344+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
345+
}
329346
out.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation] = string(currentConditionsEnc)
330347
}
331348

@@ -339,47 +356,40 @@ func Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(i
339356

340357
if otherMetricsEnc, hasOtherMetrics := out.Annotations[autoscaling.MetricSpecsAnnotation]; hasOtherMetrics {
341358
var otherMetrics []autoscalingv1.MetricSpec
342-
if err := json.Unmarshal([]byte(otherMetricsEnc), &otherMetrics); err != nil {
343-
return err
344-
}
345-
346-
// the normal Spec conversion could have populated out.Spec.Metrics with a single element, so deal with that
347-
outMetrics := make([]autoscaling.MetricSpec, len(otherMetrics)+len(out.Spec.Metrics))
348-
for i, metric := range otherMetrics {
349-
if err := Convert_v1_MetricSpec_To_autoscaling_MetricSpec(&metric, &outMetrics[i], s); err != nil {
350-
return err
359+
if err := json.Unmarshal([]byte(otherMetricsEnc), &otherMetrics); err == nil {
360+
// the normal Spec conversion could have populated out.Spec.Metrics with a single element, so deal with that
361+
outMetrics := make([]autoscaling.MetricSpec, len(otherMetrics)+len(out.Spec.Metrics))
362+
for i, metric := range otherMetrics {
363+
if err := Convert_v1_MetricSpec_To_autoscaling_MetricSpec(&metric, &outMetrics[i], s); err != nil {
364+
return err
365+
}
351366
}
367+
if out.Spec.Metrics != nil {
368+
outMetrics[len(otherMetrics)] = out.Spec.Metrics[0]
369+
}
370+
out.Spec.Metrics = outMetrics
352371
}
353-
if out.Spec.Metrics != nil {
354-
outMetrics[len(otherMetrics)] = out.Spec.Metrics[0]
355-
}
356-
out.Spec.Metrics = outMetrics
357-
delete(out.Annotations, autoscaling.MetricSpecsAnnotation)
358372
}
359373

360374
if behaviorEnc, hasConstraints := out.Annotations[autoscaling.BehaviorSpecsAnnotation]; hasConstraints {
375+
// TODO: this is unmarshaling an internal type. Fix this without breaking backwards compatibility.
361376
var behavior autoscaling.HorizontalPodAutoscalerBehavior
362-
if err := json.Unmarshal([]byte(behaviorEnc), &behavior); err != nil {
363-
return err
377+
if err := json.Unmarshal([]byte(behaviorEnc), &behavior); err == nil && behavior != (autoscaling.HorizontalPodAutoscalerBehavior{}) {
378+
out.Spec.Behavior = &behavior
364379
}
365-
out.Spec.Behavior = &behavior
366-
delete(out.Annotations, autoscaling.BehaviorSpecsAnnotation)
367380
}
368381

369382
if currentMetricsEnc, hasCurrentMetrics := out.Annotations[autoscaling.MetricStatusesAnnotation]; hasCurrentMetrics {
370383
// ignore any existing status values -- the ones here have more information
371384
var currentMetrics []autoscalingv1.MetricStatus
372-
if err := json.Unmarshal([]byte(currentMetricsEnc), &currentMetrics); err != nil {
373-
return err
374-
}
375-
376-
out.Status.CurrentMetrics = make([]autoscaling.MetricStatus, len(currentMetrics))
377-
for i, currentMetric := range currentMetrics {
378-
if err := Convert_v1_MetricStatus_To_autoscaling_MetricStatus(&currentMetric, &out.Status.CurrentMetrics[i], s); err != nil {
379-
return err
385+
if err := json.Unmarshal([]byte(currentMetricsEnc), &currentMetrics); err == nil {
386+
out.Status.CurrentMetrics = make([]autoscaling.MetricStatus, len(currentMetrics))
387+
for i, currentMetric := range currentMetrics {
388+
if err := Convert_v1_MetricStatus_To_autoscaling_MetricStatus(&currentMetric, &out.Status.CurrentMetrics[i], s); err != nil {
389+
return err
390+
}
380391
}
381392
}
382-
delete(out.Annotations, autoscaling.MetricStatusesAnnotation)
383393
}
384394

385395
// autoscaling/v1 formerly had an implicit default applied in the controller. In v2beta1, we apply it explicitly.
@@ -403,19 +413,19 @@ func Convert_v1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutoscaler(i
403413

404414
if currentConditionsEnc, hasCurrentConditions := out.Annotations[autoscaling.HorizontalPodAutoscalerConditionsAnnotation]; hasCurrentConditions {
405415
var currentConditions []autoscalingv1.HorizontalPodAutoscalerCondition
406-
if err := json.Unmarshal([]byte(currentConditionsEnc), &currentConditions); err != nil {
407-
return err
408-
}
409-
410-
out.Status.Conditions = make([]autoscaling.HorizontalPodAutoscalerCondition, len(currentConditions))
411-
for i, currentCondition := range currentConditions {
412-
if err := Convert_v1_HorizontalPodAutoscalerCondition_To_autoscaling_HorizontalPodAutoscalerCondition(&currentCondition, &out.Status.Conditions[i], s); err != nil {
413-
return err
416+
if err := json.Unmarshal([]byte(currentConditionsEnc), &currentConditions); err == nil {
417+
out.Status.Conditions = make([]autoscaling.HorizontalPodAutoscalerCondition, len(currentConditions))
418+
for i, currentCondition := range currentConditions {
419+
if err := Convert_v1_HorizontalPodAutoscalerCondition_To_autoscaling_HorizontalPodAutoscalerCondition(&currentCondition, &out.Status.Conditions[i], s); err != nil {
420+
return err
421+
}
414422
}
415423
}
416-
delete(out.Annotations, autoscaling.HorizontalPodAutoscalerConditionsAnnotation)
417424
}
418425

426+
// drop round-tripping annotations after converting to internal
427+
out.Annotations, _ = autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
428+
419429
return nil
420430
}
421431

‎pkg/apis/autoscaling/v1/defaults_test.go

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@ import (
2121
"testing"
2222

2323
autoscalingv1 "k8s.io/api/autoscaling/v1"
24-
24+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2525
"k8s.io/apimachinery/pkg/runtime"
26+
"k8s.io/apimachinery/pkg/util/diff"
2627
"k8s.io/kubernetes/pkg/api/legacyscheme"
28+
"k8s.io/kubernetes/pkg/apis/autoscaling"
2729
_ "k8s.io/kubernetes/pkg/apis/autoscaling/install"
2830
. "k8s.io/kubernetes/pkg/apis/autoscaling/v1"
2931
_ "k8s.io/kubernetes/pkg/apis/core/install"
@@ -67,6 +69,71 @@ func TestSetDefaultHPA(t *testing.T) {
6769
}
6870
}
6971

72+
func TestHorizontalPodAutoscalerAnnotations(t *testing.T) {
73+
tests := []struct {
74+
hpa autoscalingv1.HorizontalPodAutoscaler
75+
test string
76+
}{
77+
{
78+
hpa: autoscalingv1.HorizontalPodAutoscaler{
79+
ObjectMeta: metav1.ObjectMeta{
80+
Annotations: map[string]string{
81+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "",
82+
autoscaling.MetricSpecsAnnotation: "",
83+
autoscaling.BehaviorSpecsAnnotation: "",
84+
autoscaling.MetricStatusesAnnotation: "",
85+
},
86+
},
87+
},
88+
test: "test empty value for Annotations",
89+
},
90+
{
91+
hpa: autoscalingv1.HorizontalPodAutoscaler{
92+
ObjectMeta: metav1.ObjectMeta{
93+
Annotations: map[string]string{
94+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "abc",
95+
autoscaling.MetricSpecsAnnotation: "abc",
96+
autoscaling.BehaviorSpecsAnnotation: "abc",
97+
autoscaling.MetricStatusesAnnotation: "abc",
98+
},
99+
},
100+
},
101+
test: "test random value for Annotations",
102+
},
103+
{
104+
hpa: autoscalingv1.HorizontalPodAutoscaler{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Annotations: map[string]string{
107+
autoscaling.HorizontalPodAutoscalerConditionsAnnotation: "[]",
108+
autoscaling.MetricSpecsAnnotation: "[]",
109+
autoscaling.BehaviorSpecsAnnotation: "[]",
110+
autoscaling.MetricStatusesAnnotation: "[]",
111+
},
112+
},
113+
},
114+
test: "test empty array value for Annotations",
115+
},
116+
}
117+
118+
for _, test := range tests {
119+
hpa := &test.hpa
120+
hpaBeforeMuatate := *hpa.DeepCopy()
121+
obj := roundTrip(t, runtime.Object(hpa))
122+
final_obj, ok := obj.(*autoscalingv1.HorizontalPodAutoscaler)
123+
if !ok {
124+
t.Fatalf("unexpected object: %v", obj)
125+
}
126+
if !reflect.DeepEqual(*hpa, hpaBeforeMuatate) {
127+
t.Errorf("diff: %v", diff.ObjectDiff(*hpa, hpaBeforeMuatate))
128+
t.Errorf("expected: %#v\n actual: %#v", *hpa, hpaBeforeMuatate)
129+
}
130+
131+
if len(final_obj.ObjectMeta.Annotations) != 0 {
132+
t.Fatalf("unexpected annotations: %v", final_obj.ObjectMeta.Annotations)
133+
}
134+
}
135+
}
136+
70137
func roundTrip(t *testing.T, obj runtime.Object) runtime.Object {
71138
data, err := runtime.Encode(legacyscheme.Codecs.LegacyCodec(SchemeGroupVersion), obj)
72139
if err != nil {

‎pkg/apis/autoscaling/v2beta1/BUILD

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ go_test(
3939
"//staging/src/k8s.io/api/autoscaling/v2beta1:go_default_library",
4040
"//staging/src/k8s.io/api/core/v1:go_default_library",
4141
"//staging/src/k8s.io/apimachinery/pkg/api/equality:go_default_library",
42+
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
4243
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
44+
"//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
4345
"//vendor/github.com/stretchr/testify/assert:go_default_library",
4446
"//vendor/k8s.io/utils/pointer:go_default_library",
4547
],

‎pkg/apis/autoscaling/v2beta1/conversion.go

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -264,18 +264,22 @@ func Convert_autoscaling_HorizontalPodAutoscaler_To_v2beta1_HorizontalPodAutosca
264264
if err := autoConvert_autoscaling_HorizontalPodAutoscaler_To_v2beta1_HorizontalPodAutoscaler(in, out, s); err != nil {
265265
return err
266266
}
267-
if in.Spec.Behavior != nil {
268-
old := out.Annotations
269-
out.Annotations = make(map[string]string, len(old)+1)
270-
for k, v := range old {
271-
out.Annotations[k] = v
272-
}
273267

268+
// clear any pre-existing round-trip annotations to make sure the only ones set are ones we produced during conversion
269+
annotations, copiedAnnotations := autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
270+
out.Annotations = annotations
271+
272+
if in.Spec.Behavior != nil {
273+
// TODO: this is marshaling an internal type. Fix this without breaking backwards compatibility with n-1 API servers.
274274
behaviorEnc, err := json.Marshal(in.Spec.Behavior)
275275
if err != nil {
276276
return err
277277
}
278-
// Even if the annotation for behavior exists, we will just overwrite it
278+
// copy before mutating
279+
if !copiedAnnotations {
280+
copiedAnnotations = true
281+
out.Annotations = autoscaling.DeepCopyStringMap(out.Annotations)
282+
}
279283
out.Annotations[autoscaling.BehaviorSpecsAnnotation] = string(behaviorEnc)
280284
}
281285

@@ -288,13 +292,17 @@ func Convert_v2beta1_HorizontalPodAutoscaler_To_autoscaling_HorizontalPodAutosca
288292
}
289293

290294
if behaviorEnc, hasBehaviors := out.Annotations[autoscaling.BehaviorSpecsAnnotation]; hasBehaviors {
295+
// TODO: this is unmarshaling an internal type. Fix this without breaking backwards compatibility with n-1 API servers.
291296
var behavior autoscaling.HorizontalPodAutoscalerBehavior
292-
if err := json.Unmarshal([]byte(behaviorEnc), &behavior); err != nil {
293-
return err
297+
if err := json.Unmarshal([]byte(behaviorEnc), &behavior); err == nil && behavior != (autoscaling.HorizontalPodAutoscalerBehavior{}) {
298+
// only move well-formed data from annotations to fields
299+
out.Spec.Behavior = &behavior
294300
}
295-
out.Spec.Behavior = &behavior
296-
delete(out.Annotations, autoscaling.BehaviorSpecsAnnotation)
297301
}
302+
303+
// drop round-tripping annotations after converting to internal
304+
out.Annotations, _ = autoscaling.DropRoundTripHorizontalPodAutoscalerAnnotations(out.Annotations)
305+
298306
return nil
299307
}
300308

0 commit comments

Comments
 (0)