Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Strip .meta.managedFields for kubectl edit command #91946

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Strip .meta.managedFields for kubectl edit command #91946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 2 additions & 6 deletions staging/src/k8s.io/kubectl/pkg/cmd/edit/edit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,8 @@ func NewCmdEdit(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra
Long: editLong,
Example: editExample,
Run: func(cmd *cobra.Command, args []string) {
if err := o.Complete(f, args, cmd); err != nil {
cmdutil.CheckErr(err)
}
if err := o.Run(); err != nil {
cmdutil.CheckErr(err)
}
cmdutil.CheckErr(o.Complete(f, args, cmd))
cmdutil.CheckErr(o.Run())
},
}

Expand Down
11 changes: 11 additions & 0 deletions staging/src/k8s.io/kubectl/pkg/cmd/util/editor/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ go_library(
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
Expand Down Expand Up @@ -41,6 +42,16 @@ go_test(
"editor_test.go",
],
embed = [":go_default_library"],
deps = [
"//staging/src/k8s.io/api/core/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/types:go_default_library",
"//staging/src/k8s.io/cli-runtime/pkg/genericclioptions:go_default_library",
"//staging/src/k8s.io/cli-runtime/pkg/resource:go_default_library",
],
)

filegroup(
Expand Down
61 changes: 61 additions & 0 deletions staging/src/k8s.io/kubectl/pkg/cmd/util/editor/editoptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -72,6 +73,8 @@ type EditOptions struct {
ApplyAnnotation bool
ChangeCause string

managedFields map[types.UID][]metav1.ManagedFieldsEntry

genericclioptions.IOStreams

Recorder genericclioptions.Recorder
Expand Down Expand Up @@ -262,6 +265,10 @@ func (o *EditOptions) Run() error {
}

if !containsError {
if err := o.extractManagedFields(originalObj); err != nil {
return preservedFile(err, results.file, o.ErrOut)
}

if err := o.editPrinterOptions.PrintObj(originalObj, w); err != nil {
return preservedFile(err, results.file, o.ErrOut)
}
Expand All @@ -279,6 +286,7 @@ func (o *EditOptions) Run() error {
if err != nil {
return preservedFile(err, results.file, o.ErrOut)
}

// If we're retrying the loop because of an error, and no change was made in the file, short-circuit
if containsError && bytes.Equal(cmdutil.StripComments(editedDiff), cmdutil.StripComments(edited)) {
return preservedFile(fmt.Errorf("%s", "Edit cancelled, no valid changes were saved."), file, o.ErrOut)
Expand Down Expand Up @@ -334,10 +342,19 @@ func (o *EditOptions) Run() error {
results.header.reasons = append(results.header.reasons, editReason{head: fmt.Sprintf("The edited file had a syntax error: %v", err)})
continue
}

// not a syntax error as it turns out...
containsError = false
updatedVisitor := resource.InfoListVisitor(updatedInfos)

// we need to add back managedFields to both updated and original object
if err := o.restoreManagedFields(updatedInfos); err != nil {
return preservedFile(err, file, o.ErrOut)
}
if err := o.restoreManagedFields(infos); err != nil {
return preservedFile(err, file, o.ErrOut)
}

// need to make sure the original namespace wasn't changed while editing
if err := updatedVisitor.Visit(resource.RequireNamespace(o.CmdNamespace)); err != nil {
return preservedFile(err, file, o.ErrOut)
Expand Down Expand Up @@ -437,6 +454,49 @@ func (o *EditOptions) Run() error {
}
}

func (o *EditOptions) extractManagedFields(obj runtime.Object) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test for extract / clear / restore would be good?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

o.managedFields = make(map[types.UID][]metav1.ManagedFieldsEntry)
if meta.IsListType(obj) {
err := meta.EachListItem(obj, func(obj runtime.Object) error {
uid, mf, err := clearManagedFields(obj)
if err != nil {
return err
}
o.managedFields[uid] = mf
return nil
})
return err
}
uid, mf, err := clearManagedFields(obj)
if err != nil {
return err
}
o.managedFields[uid] = mf
return nil
}

func clearManagedFields(obj runtime.Object) (types.UID, []metav1.ManagedFieldsEntry, error) {
metaObjs, err := meta.Accessor(obj)
if err != nil {
return "", nil, err
}
mf := metaObjs.GetManagedFields()
metaObjs.SetManagedFields(nil)
return metaObjs.GetUID(), mf, nil
}

func (o *EditOptions) restoreManagedFields(infos []*resource.Info) error {
for _, info := range infos {
metaObjs, err := meta.Accessor(info.Object)
if err != nil {
return err
}
mf := o.managedFields[metaObjs.GetUID()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend detecting if the user tried to add something here and failing with a message if so, e.g. "you can't use kubectl edit to edit the managed field list".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's a mergepatch.PreconditionFunc which I wanted to update and I forgot, updated.

metaObjs.SetManagedFields(mf)
}
return nil
}

func (o *EditOptions) visitToApplyEditPatch(originalInfos []*resource.Info, patchVisitor resource.Visitor) error {
err := patchVisitor.Visit(func(info *resource.Info, incomingErr error) error {
editObjUID, err := meta.NewAccessor().UID(info.Object)
Expand Down Expand Up @@ -590,6 +650,7 @@ func (o *EditOptions) visitToPatch(originalInfos []*resource.Info, patchVisitor
mergepatch.RequireKeyUnchanged("apiVersion"),
mergepatch.RequireKeyUnchanged("kind"),
mergepatch.RequireMetadataKeyUnchanged("name"),
mergepatch.RequireKeyUnchanged("managedFields"),
}

// Create the versioned struct from the type defined in the mapping
Expand Down
228 changes: 228 additions & 0 deletions staging/src/k8s.io/kubectl/pkg/cmd/util/editor/editoptions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,17 @@ limitations under the License.
package editor

import (
"reflect"
"testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/cli-runtime/pkg/resource"
)

func TestHashOnLineBreak(t *testing.T) {
Expand Down Expand Up @@ -49,3 +59,221 @@ func TestHashOnLineBreak(t *testing.T) {
}
}
}

func TestManagedFieldsExtractAndRestore(t *testing.T) {
tests := map[string]struct {
object runtime.Object
managedFields map[types.UID][]metav1.ManagedFieldsEntry
}{
"single object, empty managedFields": {
object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{UID: types.UID("12345")}},
managedFields: map[types.UID][]metav1.ManagedFieldsEntry{
types.UID("12345"): nil,
},
},
"multiple objects, empty managedFields": {
object: &unstructured.UnstructuredList{
Object: map[string]interface{}{
"kind": "List",
"apiVersion": "v1",
"metadata": map[string]interface{}{},
},
Items: []unstructured.Unstructured{
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "12345",
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "98765",
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
},
},
managedFields: map[types.UID][]metav1.ManagedFieldsEntry{
types.UID("12345"): nil,
types.UID("98765"): nil,
},
},
"single object, all managedFields": {
object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{
UID: types.UID("12345"),
ManagedFields: []metav1.ManagedFieldsEntry{
{
Manager: "test",
Operation: metav1.ManagedFieldsOperationApply,
},
},
}},
managedFields: map[types.UID][]metav1.ManagedFieldsEntry{
types.UID("12345"): {
{
Manager: "test",
Operation: metav1.ManagedFieldsOperationApply,
},
},
},
},
"multiple objects, all managedFields": {
object: &unstructured.UnstructuredList{
Object: map[string]interface{}{
"kind": "List",
"apiVersion": "v1",
"metadata": map[string]interface{}{},
},
Items: []unstructured.Unstructured{
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "12345",
"managedFields": []interface{}{
map[string]interface{}{
"manager": "test",
"operation": "Apply",
},
},
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "98765",
"managedFields": []interface{}{
map[string]interface{}{
"manager": "test",
"operation": "Update",
},
},
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
},
},
managedFields: map[types.UID][]metav1.ManagedFieldsEntry{
types.UID("12345"): {
{
Manager: "test",
Operation: metav1.ManagedFieldsOperationApply,
},
},
types.UID("98765"): {
{
Manager: "test",
Operation: metav1.ManagedFieldsOperationUpdate,
},
},
},
},
"multiple objects, some managedFields": {
object: &unstructured.UnstructuredList{
Object: map[string]interface{}{
"kind": "List",
"apiVersion": "v1",
"metadata": map[string]interface{}{},
},
Items: []unstructured.Unstructured{
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "12345",
"managedFields": []interface{}{
map[string]interface{}{
"manager": "test",
"operation": "Apply",
},
},
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
{
Object: map[string]interface{}{
"apiVersion": "v1",
"kind": "Pod",
"metadata": map[string]interface{}{
"uid": "98765",
},
"spec": map[string]interface{}{},
"status": map[string]interface{}{},
},
},
},
},
managedFields: map[types.UID][]metav1.ManagedFieldsEntry{
types.UID("12345"): {
{
Manager: "test",
Operation: metav1.ManagedFieldsOperationApply,
},
},
types.UID("98765"): nil,
},
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
// operate on a copy, so we can compare the original and the modified object
objCopy := test.object.DeepCopyObject()
var infos []*resource.Info
o := NewEditOptions(NormalEditMode, genericclioptions.NewTestIOStreamsDiscard())
err := o.extractManagedFields(objCopy)
if err != nil {
t.Errorf("unexpected extraction error %v", err)
}
if meta.IsListType(objCopy) {
infos = []*resource.Info{}
meta.EachListItem(objCopy, func(obj runtime.Object) error {
metaObjs, _ := meta.Accessor(obj)
if metaObjs.GetManagedFields() != nil {
t.Errorf("unexpected managedFileds after extraction")
}
infos = append(infos, &resource.Info{Object: obj})
return nil
})
} else {
metaObjs, _ := meta.Accessor(objCopy)
if metaObjs.GetManagedFields() != nil {
t.Errorf("unexpected managedFileds after extraction")
}
infos = []*resource.Info{{Object: objCopy}}
}

err = o.restoreManagedFields(infos)
if err != nil {
t.Errorf("unexpected restore error %v", err)
}
if !reflect.DeepEqual(test.object, objCopy) {
t.Errorf("mismatched object after extract and restore managedFields: %#v", objCopy)
}
if test.managedFields != nil && !reflect.DeepEqual(test.managedFields, o.managedFields) {
t.Errorf("mismatched managedFields %#v vs %#v", test.managedFields, o.managedFields)
}
})
}
}