Merge pull request #3404 from dgageot/init-valid-manifests

Better check for valid Kubernetes manifests
GoogleContainerTools · Dec 19, 2019 · fd3351b · fd3351b
2 parents 4ebd35e + 7b69513
commit fd3351b
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 78 deletions.
diff --git a/pkg/skaffold/initializer/init.go b/pkg/skaffold/initializer/init.go
@@ -674,11 +674,9 @@ func checkConfigFile(filePath string, force bool, potentialConfigs *[]string) (b
 		return true, nil
 	}
 
-	if IsSupportedKubernetesFileExtension(filePath) {
-		if filepath.Base(filePath) != "package.json" { // Not a valid k8s manifest
-			*potentialConfigs = append(*potentialConfigs, filePath)
-			return true, nil
-		}
+	if kubectl.IsKubernetesManifest(filePath) {
+		*potentialConfigs = append(*potentialConfigs, filePath)
+		return true, nil
 	}
 
 	return false, nil

diff --git a/pkg/skaffold/initializer/init_test.go b/pkg/skaffold/initializer/init_test.go
@@ -126,6 +126,8 @@ func TestPrintAnalyzeJSONNoJib(t *testing.T) {
 
 func TestWalk(t *testing.T) {
 	emptyFile := ""
+	validK8sManifest := "apiVersion: v1\nkind: Service\nmetadata:\n  name: test\n"
+
 	tests := []struct {
 		description         string
 		filesWithContents   map[string]string
@@ -139,8 +141,9 @@ func TestWalk(t *testing.T) {
 		{
 			description: "should return correct k8 configs and build files (backwards compatibility)",
 			filesWithContents: map[string]string{
-				"config/test.yaml":    emptyFile,
-				"k8pod.yml":           emptyFile,
+				"config/test.yaml":    validK8sManifest,
+				"config/invalid.yaml": emptyFile,
+				"k8pod.yml":           validK8sManifest,
 				"README":              emptyFile,
 				"deploy/Dockerfile":   emptyFile,
 				"gradle/build.gradle": emptyFile,
@@ -161,8 +164,9 @@ func TestWalk(t *testing.T) {
 		{
 			description: "should return correct k8 configs and build files",
 			filesWithContents: map[string]string{
-				"config/test.yaml":    emptyFile,
-				"k8pod.yml":           emptyFile,
+				"config/test.yaml":    validK8sManifest,
+				"config/invalid.yaml": emptyFile,
+				"k8pod.yml":           validK8sManifest,
 				"README":              emptyFile,
 				"deploy/Dockerfile":   emptyFile,
 				"gradle/build.gradle": emptyFile,
@@ -189,8 +193,8 @@ func TestWalk(t *testing.T) {
 		{
 			description: "skip validating nested jib configs",
 			filesWithContents: map[string]string{
-				"config/test.yaml":               emptyFile,
-				"k8pod.yml":                      emptyFile,
+				"config/test.yaml":               validK8sManifest,
+				"k8pod.yml":                      validK8sManifest,
 				"gradle/build.gradle":            emptyFile,
 				"gradle/subproject/build.gradle": emptyFile,
 				"maven/asubproject/pom.xml":      emptyFile,
@@ -213,9 +217,9 @@ func TestWalk(t *testing.T) {
 			filesWithContents: map[string]string{
 				"build.gradle":                 emptyFile,
 				"ignored-builder/build.gradle": emptyFile,
-				"not-ignored-config/test.yaml": emptyFile,
+				"not-ignored-config/test.yaml": validK8sManifest,
 				"Dockerfile":                   emptyFile,
-				"k8pod.yml":                    emptyFile,
+				"k8pod.yml":                    validK8sManifest,
 				"pom.xml":                      emptyFile,
 			},
 			force:         false,
@@ -234,8 +238,8 @@ func TestWalk(t *testing.T) {
 		{
 			description: "should skip hidden dir",
 			filesWithContents: map[string]string{
-				".hidden/test.yaml":  emptyFile,
-				"k8pod.yml":          emptyFile,
+				".hidden/test.yaml":  validK8sManifest,
+				"k8pod.yml":          validK8sManifest,
 				"README":             emptyFile,
 				".hidden/Dockerfile": emptyFile,
 				"Dockerfile":         emptyFile,
@@ -257,8 +261,8 @@ func TestWalk(t *testing.T) {
 kind: Config
 deploy:
   kustomize: {}`,
-				"config/test.yaml":  emptyFile,
-				"k8pod.yml":         emptyFile,
+				"config/test.yaml":  validK8sManifest,
+				"k8pod.yml":         validK8sManifest,
 				"README":            emptyFile,
 				"deploy/Dockerfile": emptyFile,
 				"Dockerfile":        emptyFile,
@@ -278,8 +282,8 @@ deploy:
 		{
 			description: "should error when skaffold.config present and force = false",
 			filesWithContents: map[string]string{
-				"config/test.yaml":  emptyFile,
-				"k8pod.yml":         emptyFile,
+				"config/test.yaml":  validK8sManifest,
+				"k8pod.yml":         validK8sManifest,
 				"README":            emptyFile,
 				"deploy/Dockerfile": emptyFile,
 				"Dockerfile":        emptyFile,
@@ -297,8 +301,8 @@ deploy:
 		{
 			description: "should error when skaffold.config present with jib config",
 			filesWithContents: map[string]string{
-				"config/test.yaml": emptyFile,
-				"k8pod.yml":        emptyFile,
+				"config/test.yaml": validK8sManifest,
+				"k8pod.yml":        validK8sManifest,
 				"README":           emptyFile,
 				"pom.xml":          emptyFile,
 				"skaffold.yaml": `apiVersion: skaffold/v1beta6

diff --git a/pkg/skaffold/initializer/kubectl/kubectl.go b/pkg/skaffold/initializer/kubectl/kubectl.go
@@ -27,11 +27,9 @@ import (
 	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
-// ValidSuffixes are the supported file formats for Kubernetes manifests
-var ValidSuffixes = []string{".yml", ".yaml", ".json"}
-
 var requiredFields = []string{"apiVersion", "kind", "metadata"}
 
 // Kubectl holds parameters to run kubectl.
@@ -62,6 +60,16 @@ func New(potentialConfigs []string) (*Kubectl, error) {
 	}, nil
 }
 
+// IsKubernetesManifest is for determining if a file is a valid Kubernetes manifest
+func IsKubernetesManifest(file string) bool {
+	if !util.IsSupportedKubernetesFormat(file) {
+		return false
+	}
+
+	_, err := parseImagesFromKubernetesYaml(file)
+	return err == nil
+}
+
 // GenerateDeployConfig implements the Initializer interface and generates
 // skaffold kubectl deployment config.
 func (k *Kubectl) GenerateDeployConfig() latest.DeployConfig {

diff --git a/pkg/skaffold/initializer/kubectl/kubectl_test.go b/pkg/skaffold/initializer/kubectl/kubectl_test.go
@@ -123,3 +123,64 @@ spec:
 		})
 	}
 }
+
+func TestIsKubernetesManifest(t *testing.T) {
+	tests := []struct {
+		description string
+		filename    string
+		content     string
+		expected    bool
+	}{
+		{
+			description: "valid k8 yaml filename format",
+			filename:    "test1.yaml",
+			content:     "apiVersion: v1\nkind: Service\nmetadata:\n  name: test\n",
+			expected:    true,
+		},
+		{
+			description: "valid k8 json filename format",
+			filename:    "test1.json",
+			content:     `{"apiVersion":"v1","kind":"Service","metadata":{"name": "test"}}`,
+			expected:    true,
+		},
+		{
+			description: "valid k8 yaml filename format",
+			filename:    "test1.yml",
+			content:     "apiVersion: v1\nkind: Service\nmetadata:\n  name: test\n",
+			expected:    true,
+		},
+		{
+			description: "invalid k8 yaml",
+			filename:    "test1.yaml",
+			content:     "key: value",
+			expected:    false,
+		},
+		{
+			description: "invalid k8 json",
+			filename:    "test1.json",
+			content:     `{}`,
+			expected:    false,
+		},
+		{
+			description: "invalid k8s yml",
+			filename:    "test1.yml",
+			content:     "key: value",
+			expected:    false,
+		},
+		{
+			description: "invalid file",
+			filename:    "some.config",
+			content:     "",
+			expected:    false,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.NewTempDir().Write(test.filename, test.content).Chdir()
+
+			supported := IsKubernetesManifest(test.filename)
+
+			t.CheckDeepEqual(test.expected, supported)
+		})
+	}
+}
diff --git a/pkg/skaffold/initializer/util.go b/pkg/skaffold/initializer/util.go
@@ -16,24 +16,7 @@ limitations under the License.
 
 package initializer
 
-import (
-	"strings"
-
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/initializer/kubectl"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
-)
-
-// IsSupportedKubernetesFileExtension is for determining if a file under a glob pattern
-// is deployable file format. It makes no attempt to check whether or not the file
-// is actually deployable or has the correct contents.
-func IsSupportedKubernetesFileExtension(n string) bool {
-	for _, s := range kubectl.ValidSuffixes {
-		if strings.HasSuffix(n, s) {
-			return true
-		}
-	}
-	return false
-}
+import "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
 
 // IsSkaffoldConfig is for determining if a file is skaffold config file.
 func IsSkaffoldConfig(file string) bool {

diff --git a/pkg/skaffold/initializer/util_test.go b/pkg/skaffold/initializer/util_test.go
@@ -22,42 +22,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-func TestIsSupportedKubernetesFileExtension(t *testing.T) {
-	tests := []struct {
-		description string
-		filename    string
-		expected    bool
-	}{
-		{
-			description: "valid k8 yaml filename format",
-			filename:    "test1.yaml",
-			expected:    true,
-		},
-		{
-			description: "valid k8 json filename format",
-			filename:    "test1.json",
-			expected:    true,
-		},
-		{
-			description: "valid k8 yaml filename format",
-			filename:    "test1.yml",
-			expected:    true,
-		},
-		{
-			description: "invalid file",
-			filename:    "some.config",
-			expected:    false,
-		},
-	}
-	for _, test := range tests {
-		testutil.Run(t, test.description, func(t *testutil.T) {
-			supported := IsSupportedKubernetesFileExtension(test.filename)
-
-			t.CheckDeepEqual(test.expected, supported)
-		})
-	}
-}
-
 func TestIsSkaffoldConfig(t *testing.T) {
 	tests := []struct {
 		description string