Merge pull request #73608 from juanvallejo/jvallejo/improve-discovery-restmapper-errors

return original error, unless error is *meta.NoKindMatchError

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder.go

@@ -708,12 +708,12 @@ func (b *Builder) mappingFor(resourceOrKindArg string) (*meta.RESTMapping, error
 		// if we error out here, it is because we could not match a resource or a kind
 		// for the given argument. To maintain consistency with previous behavior,
 		// announce that a resource type could not be found.
-		// if the error is a URL error, then we had trouble doing discovery, so we should return the original
-		// error since it may help a user diagnose what is actually wrong
-		if _, ok := err.(*url.Error); ok {
-			return nil, err
+		// if the error is _not_ a *meta.NoKindMatchError, then we had trouble doing discovery,
+		// so we should return the original error since it may help a user diagnose what is actually wrong
+		if meta.IsNoMatchError(err) {
+			return nil, fmt.Errorf("the server doesn't have a resource type %q", groupResource.Resource)
 		}
-		return nil, fmt.Errorf("the server doesn't have a resource type %q", groupResource.Resource)
+		return nil, err
 	}
 
 	return mapping, nil

staging/src/k8s.io/cli-runtime/pkg/genericclioptions/resource/builder_test.go

@@ -282,6 +282,30 @@ func newDefaultBuilderWith(fakeClientFn FakeClientFunc) *Builder {
 		WithScheme(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...)
 }
 
+type errorRestMapper struct {
+	meta.RESTMapper
+	err error
+}
+
+func (l *errorRestMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) {
+	return nil, l.err
+}
+
+func newDefaultBuilderWithMapperError(fakeClientFn FakeClientFunc, err error) *Builder {
+	return NewFakeBuilder(
+		fakeClientFn,
+		func() (meta.RESTMapper, error) {
+			return &errorRestMapper{
+				RESTMapper: testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme),
+				err:        err,
+			}, nil
+		},
+		func() (restmapper.CategoryExpander, error) {
+			return FakeCategoryExpander, nil
+		}).
+		WithScheme(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...)
+}
+
 func TestPathBuilderAndVersionedObjectNotDefaulted(t *testing.T) {
 	b := newDefaultBuilder().
 		FilenameParam(false, &FilenameOptions{Recursive: false, Filenames: []string{"../../../artifacts/kitten-rc.yaml"}})
@@ -634,6 +658,49 @@ func TestResourceByName(t *testing.T) {
 	}
 }
 
+func TestRestMappingErrors(t *testing.T) {
+	pods, _ := testData()
+	b := newDefaultBuilderWith(fakeClientWith("", t, map[string]string{
+		"/namespaces/test/pods/foo": runtime.EncodeOrDie(corev1Codec, &pods.Items[0]),
+	})).NamespaceParam("test")
+
+	if b.Do().Err() == nil {
+		t.Errorf("unexpected non-error")
+	}
+
+	test := &testVisitor{}
+	singleItemImplied := false
+
+	b.ResourceTypeOrNameArgs(true, "foo", "bar")
+
+	// ensure that requesting a resource we _know_ not to exist results in an expected *meta.NoKindMatchError
+	err := b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle)
+	if err != nil {
+		if !strings.Contains(err.Error(), "server doesn't have a resource type \"foo\"") {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	}
+
+	expectedErr := fmt.Errorf("expected error")
+	b = newDefaultBuilderWithMapperError(fakeClientWith("", t, map[string]string{
+		"/namespaces/test/pods/foo": runtime.EncodeOrDie(corev1Codec, &pods.Items[0]),
+	}), expectedErr).NamespaceParam("test")
+
+	if b.Do().Err() == nil {
+		t.Errorf("unexpected non-error")
+	}
+
+	// ensure we request a resource we know not to exist. This way, we
+	// end up taking the codepath we want to test in the resource builder
+	b.ResourceTypeOrNameArgs(true, "foo", "bar")
+
+	// ensure that receiving an error for any reason other than a non-existent resource is returned as-is
+	err = b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle)
+	if err != nil && !strings.Contains(err.Error(), expectedErr.Error()) {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
 func TestMultipleResourceByTheSameName(t *testing.T) {
 	pods, svcs := testData()
 	b := newDefaultBuilderWith(fakeClientWith("", t, map[string]string{