protect admission against empty keys #5900

dbenque · 2023-06-28T09:39:17Z

Which component this PR applies to?

vertical-pod-autoscaler/admission-controller

What type of PR is this?

/kind bug

What this PR does / why we need it:

Using external recommender that was coming with a bug, we ended up with a VPA object that was presenting a recommendation for a resource that has an empty name:

 resources:
          limits:
            cpu: "1"
            memory: 3Gi
          requests:
            "": "0"              <== empty key
            cpu: "1"
            ephemeral-storage: 10Gi
            memory: 3Gi

In this PR we are proposing to sanitise the recommendation by removing any resource key that would be empty so that the admission is not pushing "garbage" to the pods.

Possible extension: instead of purging empty key, we could have a list of allowed resourceName given as input parameter of the admission. This would also allow us to purge unknown resource or the one coming with typo.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

If a recommendation comes with a resource key that is empty, this resource will be ignored in the admission-controller.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

The provisioning state reflects the status of the last provisioning action, which means the instance can enter a failed state after it's running. Protect against unnecessary scaledowns by checking the power state to avoid scaling down running VMs

- this taint leads to unexpected behavior - users expect CA to consider the taint when autoscaling Signed-off-by: vadasambar <[email protected]>

- happens when CA tries to check if the unmanaged fargate node is a part of ASG (it isn't) - and keeps on logging error Signed-off-by: vadasambar <[email protected]>

Signed-off-by: vadasambar <[email protected]>

…are more appropriate

Previous "CropNodes" function of ScaleDownBudgetProcessor had an assumption that atomically-scaled node groups should be classified as "empty" or "drain" as a whole, however Cluster Autoscaler may classify some of the nodes from a single group as "empty" and other as "drain".

update agnhost image to pull from registry.k8s.io

jbartosik

I asked for some changes. I think we should allow only cpu and memory

Also I'd add a short release note

...oscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go

jbartosik · 2023-06-28T13:16:41Z

vertical-pod-autoscaler/pkg/utils/test/test_utils.go

@@ -89,6 +90,16 @@ func Resources(cpu, mem string) apiv1.ResourceList {
 	return result
 }

+// AddResource add a resource to the given resource list
+func AddResource(rl apiv1.ResourceList, resourceName apiv1.ResourceName, value string) apiv1.ResourceList {


There's only one place we're using this function. Please move it to vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go and unexport it (rename to addResource)

jbartosik · 2023-06-28T13:20:28Z

vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go

@@ -55,6 +57,15 @@ func (b *recommendationBuilder) WithTarget(cpu, memory string) RecommendationBui
 	return &c
 }

+func (b *recommendationBuilder) WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder {


Suggested change

func (b *recommendationBuilder) WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder {

func (b *recommendationBuilder) WithTargetResource(resource apiv1.ResourceName, value string) RecommendationBuilder {

jbartosik · 2023-06-28T13:23:13Z

vertical-pod-autoscaler/pkg/utils/test/test_vpa.go

@@ -131,6 +133,12 @@ func (b *verticalPodAutoscalerBuilder) WithTarget(cpu, memory string) VerticalPo
 	return &c
 }

+func (b *verticalPodAutoscalerBuilder) WithResourceInTarget(resource core.ResourceName, value string) VerticalPodAutoscalerBuilder {


Let's call it the same as the function in vertical-pod-autoscaler/pkg/utils/test/test_recommendation.go

Also please add a TODO, it looks like verticalPodAutoscalerBuilder (here) and recommendationBuilder (in test_recommendation.go) could use some deduplication

Generated by runing: ``` go mod tidy go mod vendor ```

Update VPA vendor

…r-cleanup Replace `BuildTestContainer` with use of builder

Allow using an externally created secret instead of using the one the Helm chart creates

fix: Broken links to testgrid dashboard

…ial-backoff feat(hetzner): use less requests while waiting for server create

Signed-off-by: AhmedGrati <[email protected]>

…proposal-5700 [addon-resizer] docs: add KEP to add nanny configuration automatic reload.

update RBAC to only use verbs that exist for the resources

…re-s390x-arch CA - Git ignore s390x arch binaries

…mp-pod-scaleup Add support to filter out pods being deleted.

…recommendation/recommendation_provider_test.go Co-authored-by: Joachim <[email protected]>

k8s-ci-robot · 2023-08-28T10:10:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dbenque
Once this PR has been reviewed and has the lgtm label, please assign mwielgus for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dbenque · 2023-08-28T10:35:40Z

Closing this PR due to a bad rebase that makes the history not clear.

Taking all the relevant changes to a new clean branch:
#6062

jbartosik · 2023-09-04T06:30:15Z

...oscaler/pkg/admission-controller/resource/pod/recommendation/recommendation_provider_test.go

@@ -311,8 +311,7 @@ func TestUpdateResourceRequests(t *testing.T) {
 					return
 				}

-				_, foundEmpty := resources[0].Requests[""]
-				assert.Equal(t, foundEmpty, false, "empty resourceKey have not been purged")
+				assert.Contains(t, resources, "", "expected empty resource to be removed")


I know this is closed but isn't this check kind of opposite of the old one?

domenicbozzuto and others added 16 commits May 17, 2023 16:02

feat(*): add more metrics

fab8ec7

fix: don't clean CriticalAddonsOnly taint from template nodes

7217870

- this taint leads to unexpected behavior - users expect CA to consider the taint when autoscaling Signed-off-by: vadasambar <[email protected]>

fix: CA on fargate causing log flood

adde651

- happens when CA tries to check if the unmanaged fargate node is a part of ASG (it isn't) - and keeps on logging error Signed-off-by: vadasambar <[email protected]>

test: fix node names

ec01059

Signed-off-by: vadasambar <[email protected]>

Move two util functions from actuator to delete_in_batch, where they …

1847875

…are more appropriate

Add support for atomic scale-down in node group options

61b7958

Extract cropNodesToBudgets function out of actuator file

292a517

Support atomic scale-down option for node groups

7e3e15b

Respond to readability-related comments from the review

1ecb843

Don't pass NodeGroup as a parameter to functions running asynchronously

257e66c

Add unit test for group_deletion_scheduler

5448bd2

Use single AtomicScaling option for scale up and scale down

05e1fe1

Address next set of comments

374cf61

update agnhost image to pull from registry.k8s.io

76a7d21

protect admission against empty keys

08c8ba0

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/vertical-pod-autoscaler labels Jun 28, 2023

k8s-ci-robot requested review from jbartosik and kgolab June 28, 2023 09:39

kawych and others added 3 commits June 28, 2023 10:04

Remove unneeded node groups regardless of scale down being in cooldown.

72c0a7e

Merge pull request kubernetes#5895 from kwiesmueller/e2e-todo-1

03b45e2

update agnhost image to pull from registry.k8s.io

jbartosik requested changes Jun 28, 2023

View reviewed changes

jbartosik and others added 4 commits June 28, 2023 16:01

Update VPA vendor

0e7d327

Generated by runing: ``` go mod tidy go mod vendor ```

Replace BuildTestContainer with use of builder

db507cd

Merge pull request kubernetes#5904 from jbartosik/update-vpa-vendor

5a23f69

Update VPA vendor

Merge pull request kubernetes#5896 from kwiesmueller/test-util-builde…

8f83f7e

…r-cleanup Replace `BuildTestContainer` with use of builder

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2023

Quote temp folder name parameter to avoid errors

d3a1f4f

k8s-ci-robot and others added 12 commits August 22, 2023 15:45

Merge pull request kubernetes#6016 from mtougeron/flexible-secret-name

5e20c44

Allow using an externally created secret instead of using the one the Helm chart creates

Merge pull request kubernetes#6036 from khareyash05/docs

199570e

fix: Broken links to testgrid dashboard

Merge pull request kubernetes#6039 from hetznercloud/hetzner-exponent…

533719c

…ial-backoff feat(hetzner): use less requests while waiting for server create

kep: add config file format and structure notes

6a207c8

Signed-off-by: AhmedGrati <[email protected]>

Merge pull request kubernetes#6015 from AhmedGrati/addon-resizer-kep-…

0874fe6

…proposal-5700 [addon-resizer] docs: add KEP to add nanny configuration automatic reload.

Merge pull request kubernetes#5927 from MaxRink/rbac

57d07df

update RBAC to only use verbs that exist for the resources

CA - Git ignore s390x arch binaries

5c666d3

Add support to filter out pods being deleted.

1b3bbdd

Merge pull request kubernetes#6059 from gjtempleton/ca-update-gitigno…

a370dc3

…re-s390x-arch CA - Git ignore s390x arch binaries

Merge pull request kubernetes#6056 from jayantjain93/deletion-timesta…

fe2e4c0

…mp-pod-scaleup Add support to filter out pods being deleted.

protect admission against empty keys

6b81c88

Update vertical-pod-autoscaler/pkg/admission-controller/resource/pod/…

dd8c42b

…recommendation/recommendation_provider_test.go Co-authored-by: Joachim <[email protected]>

dbenque requested a review from jbartosik August 28, 2023 09:28

Code review

7220ffa

k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider area/provider/azure Issues or PRs related to azure provider area/provider/gce area/provider/hetzner Issues or PRs related to Hetzner provider area/balancer area/cluster-autoscaler area/helm-charts labels Aug 28, 2023

dbenque mentioned this pull request Aug 28, 2023

David.benque/prevent empty resource propagation upstream 2 #6062

Merged

dbenque closed this Aug 28, 2023

jbartosik reviewed Sep 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protect admission against empty keys #5900

protect admission against empty keys #5900

dbenque commented Jun 28, 2023 •

edited

Loading

jbartosik left a comment

jbartosik Jun 28, 2023

dbenque Aug 28, 2023

jbartosik Jun 28, 2023

dbenque Aug 28, 2023

jbartosik Jun 28, 2023

dbenque Aug 28, 2023

k8s-ci-robot commented Aug 28, 2023

dbenque commented Aug 28, 2023

jbartosik Sep 4, 2023

	func (b *recommendationBuilder) WithResource(resource apiv1.ResourceName, value string) RecommendationBuilder {
	func (b *recommendationBuilder) WithTargetResource(resource apiv1.ResourceName, value string) RecommendationBuilder {

protect admission against empty keys #5900

protect admission against empty keys #5900

Conversation

dbenque commented Jun 28, 2023 • edited Loading

Which component this PR applies to?

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jbartosik left a comment

Choose a reason for hiding this comment

jbartosik Jun 28, 2023

Choose a reason for hiding this comment

dbenque Aug 28, 2023

Choose a reason for hiding this comment

jbartosik Jun 28, 2023

Choose a reason for hiding this comment

dbenque Aug 28, 2023

Choose a reason for hiding this comment

jbartosik Jun 28, 2023

Choose a reason for hiding this comment

dbenque Aug 28, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 28, 2023

dbenque commented Aug 28, 2023

jbartosik Sep 4, 2023

Choose a reason for hiding this comment

dbenque commented Jun 28, 2023 •

edited

Loading