From f2b48a90dfeb73c673a700c945bdd75cefed5880 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 15 Mar 2018 10:06:51 -0700 Subject: [PATCH] diff: Fix broken `Local()` logic Local and Live functions where doing and returning the same thing, giving empty results by default. Fix the local function by copying the objects before fetching the live version. --- hack/make-rules/test-cmd-util.sh | 38 ++++++++++++++++ pkg/kubectl/cmd/diff.go | 76 +++++++++++++++++++++++++------- 2 files changed, 98 insertions(+), 16 deletions(-) diff --git a/hack/make-rules/test-cmd-util.sh b/hack/make-rules/test-cmd-util.sh index acea42c3f6e..bba8d4656cc 100755 --- a/hack/make-rules/test-cmd-util.sh +++ b/hack/make-rules/test-cmd-util.sh @@ -1185,6 +1185,39 @@ run_kubectl_apply_deployments_tests() { set +o errexit } +# Runs tests for kubectl alpha diff +run_kubectl_diff_tests() { + set -o nounset + set -o errexit + + create_and_use_new_namespace + kube::log::status "Testing kubectl alpha diff" + + kubectl apply -f hack/testdata/pod.yaml + + # Ensure that selfLink has been added, and shown in the diff + output_message=$(kubectl alpha diff -f hack/testdata/pod.yaml) + kube::test::if_has_string "${output_message}" 'selfLink' + output_message=$(kubectl alpha diff LOCAL LIVE -f hack/testdata/pod.yaml) + kube::test::if_has_string "${output_message}" 'selfLink' + output_message=$(kubectl alpha diff LOCAL MERGED -f hack/testdata/pod.yaml) + kube::test::if_has_string "${output_message}" 'selfLink' + + output_message=$(kubectl alpha diff MERGED MERGED -f hack/testdata/pod.yaml) + kube::test::if_empty_string "${output_message}" + output_message=$(kubectl alpha diff LIVE LIVE -f hack/testdata/pod.yaml) + kube::test::if_empty_string "${output_message}" + output_message=$(kubectl alpha diff LAST LAST -f hack/testdata/pod.yaml) + kube::test::if_empty_string "${output_message}" + output_message=$(kubectl alpha diff LOCAL LOCAL -f hack/testdata/pod.yaml) + kube::test::if_empty_string "${output_message}" + + kubectl delete -f hack/testdata/pod.yaml + + set +o nounset + set +o errexit +} + # Runs tests for --save-config tests. run_save_config_tests() { set -o nounset @@ -4983,6 +5016,11 @@ runTests() { record_command run_kubectl_apply_deployments_tests fi + ################ + # Kubectl diff # + ################ + record_command run_kubectl_diff_tests + ############### # Kubectl get # ############### diff --git a/pkg/kubectl/cmd/diff.go b/pkg/kubectl/cmd/diff.go index 7f42034b22e..9105d4b953c 100644 --- a/pkg/kubectl/cmd/diff.go +++ b/pkg/kubectl/cmd/diff.go @@ -26,9 +26,11 @@ import ( "github.com/ghodss/yaml" "github.com/spf13/cobra" - "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/client-go/dynamic" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/kubectl/apply/parse" "k8s.io/kubernetes/pkg/kubectl/apply/strategy" @@ -263,6 +265,7 @@ type Object interface { // InfoObject is an implementation of the Object interface. It gets all // the information from the Info object. type InfoObject struct { + Remote runtime.Unstructured Info *resource.Info Encoder runtime.Encoder Parser *parse.Factory @@ -288,14 +291,10 @@ func (obj InfoObject) Local() (map[string]interface{}, error) { } func (obj InfoObject) Live() (map[string]interface{}, error) { - if obj.Info.Object == nil { + if obj.Remote == nil { return nil, nil // Object doesn't exist on cluster. } - data, err := runtime.Encode(obj.Encoder, obj.Info.Object) - if err != nil { - return nil, err - } - return obj.toMap(data) + return obj.Remote.UnstructuredContent(), nil } func (obj InfoObject) Merged() (map[string]interface{}, error) { @@ -327,10 +326,10 @@ func (obj InfoObject) Merged() (map[string]interface{}, error) { } func (obj InfoObject) Last() (map[string]interface{}, error) { - if obj.Info.Object == nil { + if obj.Remote == nil { return nil, nil // No object is live, return empty } - accessor, err := meta.Accessor(obj.Info.Object) + accessor, err := meta.Accessor(obj.Remote) if err != nil { return nil, err } @@ -390,6 +389,50 @@ func (d *Differ) TearDown() { d.To.Dir.Delete() // Ignore error } +type Downloader struct { + mapper meta.RESTMapper + dclient dynamic.Interface + ns string +} + +func NewDownloader(f cmdutil.Factory) (*Downloader, error) { + var err error + var d Downloader + + d.mapper, err = f.RESTMapper() + if err != nil { + return nil, err + } + d.dclient, err = f.DynamicClient() + if err != nil { + return nil, err + } + d.ns, _, _ = f.DefaultNamespace() + + return &d, nil +} + +func (d *Downloader) Download(info *resource.Info) (*unstructured.Unstructured, error) { + gvk := info.Object.GetObjectKind().GroupVersionKind() + mapping, err := d.mapper.RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + return nil, err + } + + var resource dynamic.ResourceInterface + switch mapping.Scope.Name() { + case meta.RESTScopeNameNamespace: + if info.Namespace == "" { + info.Namespace = d.ns + } + resource = d.dclient.Resource(mapping.Resource).Namespace(info.Namespace) + case meta.RESTScopeNameRoot: + resource = d.dclient.Resource(mapping.Resource) + } + + return resource.Get(info.Name, metav1.GetOptions{}) +} + // RunDiff uses the factory to parse file arguments, find the version to // diff, and find each Info object for each files, and runs against the // differ. @@ -417,25 +460,26 @@ func RunDiff(f cmdutil.Factory, diff *DiffProgram, options *DiffOptions, from, t Unstructured(). NamespaceParam(cmdNamespace).DefaultNamespace(). FilenameParam(enforceNamespace, &options.FilenameOptions). + Local(). Flatten(). Do() if err := r.Err(); err != nil { return err } + dl, err := NewDownloader(f) + if err != nil { + return err + } + err = r.Visit(func(info *resource.Info, err error) error { if err != nil { return err } - if err := info.Get(); err != nil { - if !errors.IsNotFound(err) { - return cmdutil.AddSourceToErr(fmt.Sprintf("retrieving current configuration of:\n%s\nfrom server for:", info.String()), info.Source, err) - } - info.Object = nil - } - + remote, _ := dl.Download(info) obj := InfoObject{ + Remote: remote, Info: info, Parser: parser, Encoder: cmdutil.InternalVersionJSONEncoder(),