Reset managedFields corrupted by admission controllers #98074

kwiesmueller · 2021-01-14T19:25:37Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
When a mutating admission controller changes managedFields in a way that causes them to be invalid, this change will reset them to their state before the admission chain modified them.

Which issue(s) this PR fixes:

Fixes #96688

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The apiserver now resets managedFields that got corrupted by a mutating admission controller.

/sig api-machinery
/wg api-expression
/cc @apelisse

kwiesmueller · 2021-01-14T19:26:44Z

For a reason I still don't understand the webhook tests fail for me locally with unable to sync kubernetes service: Endpoints "kubernetes" is invalid: subsets[0].addresses[0].ip: Invalid value: "127.0.0.1": may not be in the loopback range (127.0.0.0/8).
Let's hope it works in CI.

fedebongio · 2021-01-14T21:19:50Z

/triage accepted

kwiesmueller · 2021-01-15T00:06:07Z

/retest

kwiesmueller · 2021-01-15T01:01:09Z

I don't really understand what I could have done to cause this specific error.
/retest

apelisse · 2021-01-15T04:04:18Z

Conformance test fail specifically on admission webhooks, that might be related ;-)
bazel-test fails on endpoint, I haven't looked but I'm sure you've changed something there.

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/admission.go

apelisse · 2021-01-16T00:51:50Z

A few questions based on our discussion:

Why is the result of mutating webhooks not validated? I bet it is, why is this specific field not properly validated?
Since this is around the whole admission rather than per plugin, should we send an invalid fields set by a plugin to the following plugins?
What happens if we don't validate the whole "FieldSet", how do we treat these errors in general?

kwiesmueller · 2021-02-01T19:02:28Z

/retest

apelisse · 2021-02-04T01:15:55Z

Rather than making this into a "admission wrapper", would it make sense to apply this right after admission instead in the DefaultUpdatedObjectInfo function, e.g. https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L596-L604?

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go

apelisse

I still need to look at the changes to the internal decoding functions.

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/admission.go

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go

apelisse · 2021-03-01T20:21:15Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/admission_test.go

+				{
+					FieldsType: "FieldsV1",
+				},
+			},


Maybe make a test that shows that corrupting the "FieldsV1" isn't good

Actually this test is no longer testing what it did before.
All tests fail for the same reason, something is missing from the managedFields entry.
The manager name is not even covered by the reset anymore but still gets reset.

The test just makes sure a reset happens if anything is invalid.
The details are tested in the decoding tests. I'll simplify this one.

Changed the test quite a bit and it should actually do what it's supposed to do now.

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/admission_test.go

apelisse · 2021-03-01T20:54:46Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go

+// DecodeManagedFields converts ManagedFields from the wire format (api format)
+// to the format used by sigs.k8s.io/structured-merge-diff
+func DecodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (Managed, error) {
+	return internal.DecodeManagedFields(encodedManagedFields)


I haven't look at internal yet, but can we get rid of the indirection here? I'd rather avoid having too many "DecodeManagedFields" functions if possible :-)

I looked at it for a bit, but I didn't want to move too much outside the internal package if not needed.
There are a few internal types and decoding functions right now, but in theory we could move everything I think.
We would be splitting up decoding and encoding though, or move encoding as well. But I'm not sure if I want to also expose encoding.
Have a look and tell me what you think, I can do it either way.

apelisse · 2021-03-01T21:02:04Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go

@@ -219,7 +224,7 @@ func (f *FieldManager) Apply(liveObj, appliedObj runtime.Object, manager string,
 	}

 	// Decode the managed fields in the live object, since it isn't allowed in the patch.
-	managed, err := internal.DecodeObjectManagedFields(accessor.GetManagedFields())
+	managed, err := DecodeManagedFields(accessor.GetManagedFields())


Yes, but somewhat unrelated, shouldn't we do something useful if the managedfields is invalid here?

Should we replace emptyManagedFieldsOnErr method with a decodeManagedFieldsOrEmpty, which could be used here?

Shouldn't we keep returning the error here if decoding fails (it is apply)?
What would we want to happen if live is corrupted?
It could only happen if somebody updates it and then applies would fail in the future. I'd say we then should prevent invalid managedFields from getting persisted, what this PR tries to at least make harder.

Right, if the managed fields are corrupted in the apiserver, then someone can't apply anymore. The right thing to do is to restart fresh I would say.

apelisse · 2021-03-01T21:02:38Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go


+func emptyManagedFieldsOnErr(managed Managed, err error) (Managed, error) {


This always returns nil error, does it need to return that?

Yeah, I made it this way for ease of use so it can just wrap the function call for providing the same return values.
So no additional error check or anything is required.

apelisse · 2021-03-01T23:44:17Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/managedfields.go

 // to the format used by sigs.k8s.io/structured-merge-diff
-func decodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (managed managedStruct, err error) {
+func DecodeManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (ManagedInterface, error) {


That's good, thanks!

apelisse · 2021-03-01T23:44:48Z

Beside the new validation that I think you have to revert, mostly nits, thanks!

apelisse

/lgtm

Thanks Kevin!

lavalamp · 2021-03-07T03:07:02Z

/approve

k8s-ci-robot · 2021-03-07T03:07:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwiesmueller, lavalamp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~test/integration/apiserver/OWNERS~~ [lavalamp]

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

k8s-ci-robot requested a review from apelisse January 14, 2021 19:25

k8s-ci-robot added area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 14, 2021

kwiesmueller force-pushed the admission-reset-managedFields branch from 6cde0f1 to eb4d0af Compare January 14, 2021 21:05

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 14, 2021

apelisse reviewed Jan 15, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/admission.go Outdated Show resolved Hide resolved

kwiesmueller force-pushed the admission-reset-managedFields branch from 4f17899 to 9eb9385 Compare February 1, 2021 17:17

kwiesmueller force-pushed the admission-reset-managedFields branch 4 times, most recently from 1094f21 to 9f45cd3 Compare February 2, 2021 15:14

Kevin Wiesmueller added 12 commits March 1, 2021 19:59

fix test

711f2da

fix admission controller

429a96d

add managedFields admission

f86b59a

export and cleanup managedFields decoding

589ca1b

consolidate managedFields validation

da610d6

use existing validation code and decoding in fieldManager admission

fc1841d

add warning check to managedFields integration test

ba2f610

prevent fieldManager admission from wrapping nil

22dfa6a

update licenses and bazel

1a8e2bf

fix test dependencies

295e47f

fix validation tests

ff5a0cc

harden managedFields decoding

470ad03

kwiesmueller force-pushed the admission-reset-managedFields branch from 0ee0fc6 to 470ad03 Compare March 1, 2021 19:07

kwiesmueller commented Mar 1, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/fieldmanager.go Show resolved Hide resolved

only use managedFields decoding for admission check

98d4981

apelisse reviewed Mar 1, 2021

View reviewed changes

Jefftree mentioned this pull request Mar 2, 2021

Promote SSA to GA #99652

Merged

Kevin Wiesmueller added 3 commits March 4, 2021 21:17

remove apiVersion check from managedFields validation

ce0657c

cleanup managedFields admission and test

2d1ba0c

fix validation test

94149ef

apelisse reviewed Mar 5, 2021

View reviewed changes

k8s-ci-robot assigned apelisse Mar 5, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2021

k8s-ci-robot merged commit 4cf8823 into kubernetes:master Mar 7, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset managedFields corrupted by admission controllers #98074

Reset managedFields corrupted by admission controllers #98074

kwiesmueller commented Jan 14, 2021

kwiesmueller commented Jan 14, 2021

fedebongio commented Jan 14, 2021

kwiesmueller commented Jan 15, 2021

kwiesmueller commented Jan 15, 2021

apelisse commented Jan 15, 2021 •

edited

Loading

apelisse commented Jan 16, 2021

kwiesmueller commented Feb 1, 2021

apelisse commented Feb 4, 2021

apelisse left a comment

apelisse Mar 1, 2021

kwiesmueller Mar 4, 2021

kwiesmueller Mar 4, 2021

apelisse Mar 1, 2021

kwiesmueller Mar 4, 2021

apelisse Mar 1, 2021

kwiesmueller Mar 4, 2021

apelisse Mar 4, 2021

apelisse Mar 1, 2021

kwiesmueller Mar 4, 2021

apelisse Mar 1, 2021

apelisse commented Mar 1, 2021

apelisse left a comment

lavalamp commented Mar 7, 2021

k8s-ci-robot commented Mar 7, 2021


		func emptyManagedFieldsOnErr(managed Managed, err error) (Managed, error) {

Reset managedFields corrupted by admission controllers #98074

Reset managedFields corrupted by admission controllers #98074

Conversation

kwiesmueller commented Jan 14, 2021

kwiesmueller commented Jan 14, 2021

fedebongio commented Jan 14, 2021

kwiesmueller commented Jan 15, 2021

kwiesmueller commented Jan 15, 2021

apelisse commented Jan 15, 2021 • edited Loading

apelisse commented Jan 16, 2021

kwiesmueller commented Feb 1, 2021

apelisse commented Feb 4, 2021

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Mar 1, 2021

apelisse left a comment

Choose a reason for hiding this comment

lavalamp commented Mar 7, 2021

k8s-ci-robot commented Mar 7, 2021

apelisse commented Jan 15, 2021 •

edited

Loading