From c4937ae9ffaf29b1c39b34b33897c1d6c468f075 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Thu, 8 Mar 2018 16:20:37 +0000 Subject: [PATCH 1/3] Correct assumptions about test files A couple of tests assume that the test files are 1-1 with resources. This commit removes that assumption by comparing things to the curated ServiceMap() instead. It also puts one of the files in a subdirectory, to make sure things still work. --- cluster/kubernetes/resource/load_test.go | 5 ++--- cluster/kubernetes/testfiles/data.go | 8 ++++++-- daemon/daemon_test.go | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cluster/kubernetes/resource/load_test.go b/cluster/kubernetes/resource/load_test.go index ecaafefee..0a57a507f 100644 --- a/cluster/kubernetes/resource/load_test.go +++ b/cluster/kubernetes/resource/load_test.go @@ -136,8 +136,7 @@ func TestLoadSome(t *testing.T) { if err != nil { t.Error(err) } - // assume it's one per file for the minute - if len(objs) != len(testfiles.Files) { - t.Errorf("expected %d objects from %d files, got result:\n%#v", len(testfiles.Files), len(testfiles.Files), objs) + if len(objs) != len(testfiles.ServiceMap(dir)) { + t.Errorf("expected %d objects from %d files, got result:\n%#v", len(testfiles.ServiceMap(dir)), len(testfiles.Files), objs) } } diff --git a/cluster/kubernetes/testfiles/data.go b/cluster/kubernetes/testfiles/data.go index 2dc586e4e..913d268a0 100644 --- a/cluster/kubernetes/testfiles/data.go +++ b/cluster/kubernetes/testfiles/data.go @@ -30,6 +30,9 @@ func TempDir(t *testing.T) (string, func()) { func WriteTestFiles(dir string) error { for name, content := range Files { path := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil { + return err + } if err := ioutil.WriteFile(path, []byte(content), 0666); err != nil { return err } @@ -45,11 +48,12 @@ func ServiceMap(dir string) map[flux.ResourceID][]string { return map[flux.ResourceID][]string{ flux.MustParseResourceID("default:deployment/helloworld"): []string{filepath.Join(dir, "helloworld-deploy.yaml")}, flux.MustParseResourceID("default:deployment/locked-service"): []string{filepath.Join(dir, "locked-service-deploy.yaml")}, - flux.MustParseResourceID("default:deployment/test-service"): []string{filepath.Join(dir, "test-service-deploy.yaml")}, + flux.MustParseResourceID("default:deployment/test-service"): []string{filepath.Join(dir, "test/test-service-deploy.yaml")}, } } var Files = map[string]string{ + "garbage": "This should just be ignored, since it is not YAML", "helloworld-deploy.yaml": `apiVersion: extensions/v1beta1 kind: Deployment metadata: @@ -98,7 +102,7 @@ spec: ports: - containerPort: 80 `, - "test-service-deploy.yaml": `apiVersion: extensions/v1beta1 + "test/test-service-deploy.yaml": `apiVersion: extensions/v1beta1 kind: Deployment metadata: name: test-service diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go index 2243e7f6a..a987f0cc6 100644 --- a/daemon/daemon_test.go +++ b/daemon/daemon_test.go @@ -195,7 +195,7 @@ func TestDaemon_NotifyChange(t *testing.T) { t.Errorf("Sync was not called once, was called %d times", syncCalled) } else if syncDef == nil { t.Errorf("Sync was called with a nil syncDef") - } else if len(syncDef.Actions) != len(testfiles.Files) { + } else if len(syncDef.Actions) != len(testfiles.ServiceMap("unimportant")) { t.Errorf("Sync was not called with the %d actions, was called with: %d", len(testfiles.Files), len(syncDef.Actions)) } From b3cd0626a1c889e640f5d0b7b4fa62b96e27f744 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Thu, 8 Mar 2018 12:21:53 +0000 Subject: [PATCH 2/3] Ignore charts when loading manifests This commit makes the kubernetes manifest code skip over charts. This is a convenience so that people don't have to segregate their config and charts. --- cluster/kubernetes/resource/load.go | 23 +++++++++++ cluster/kubernetes/testfiles/data.go | 62 ++++++++++++++++++++++++++++ sync/sync_test.go | 7 ++-- 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/cluster/kubernetes/resource/load.go b/cluster/kubernetes/resource/load.go index 9e339f6f9..9fded341a 100644 --- a/cluster/kubernetes/resource/load.go +++ b/cluster/kubernetes/resource/load.go @@ -22,6 +22,11 @@ func Load(roots ...string) (map[string]resource.Resource, error) { if err != nil { return errors.Wrapf(err, "walking %q for yamels", path) } + + if info.IsDir() && looksLikeChart(path) { + return filepath.SkipDir + } + if !info.IsDir() && filepath.Ext(path) == ".yaml" || filepath.Ext(path) == ".yml" { bytes, err := ioutil.ReadFile(path) if err != nil { @@ -47,6 +52,24 @@ func Load(roots ...string) (map[string]resource.Resource, error) { return objs, nil } +// looksLikeChart returns `true` if the path `dir` (assumed to be a +// directory) looks like it contains a Helm chart, rather than +// manifest files. +func looksLikeChart(dir string) bool { + // These are the two mandatory parts of a chart. If they both + // exist, chances are it's a chart. See + // https://github.com/kubernetes/helm/blob/master/docs/charts.md#the-chart-file-structure + chartpath := filepath.Join(dir, "Chart.yaml") + valuespath := filepath.Join(dir, "values.yaml") + if _, err := os.Stat(chartpath); err != nil && os.IsNotExist(err) { + return false + } + if _, err := os.Stat(valuespath); err != nil && os.IsNotExist(err) { + return false + } + return true +} + // ParseMultidoc takes a dump of config (a multidoc YAML) and // constructs an object set from the resources represented therein. func ParseMultidoc(multidoc []byte, source string) (map[string]resource.Resource, error) { diff --git a/cluster/kubernetes/testfiles/data.go b/cluster/kubernetes/testfiles/data.go index 913d268a0..aec8c12ec 100644 --- a/cluster/kubernetes/testfiles/data.go +++ b/cluster/kubernetes/testfiles/data.go @@ -54,6 +54,7 @@ func ServiceMap(dir string) map[flux.ResourceID][]string { var Files = map[string]string{ "garbage": "This should just be ignored, since it is not YAML", + // Some genuine manifests "helloworld-deploy.yaml": `apiVersion: extensions/v1beta1 kind: Deployment metadata: @@ -122,6 +123,67 @@ spec: ports: - containerPort: 80 `, + + // A tricksy chart directory, which should be skipped entirely. Adapted from + // https://github.com/kubernetes/helm/tree/master/docs/examples + "charts/nginx/Chart.yaml": `--- +name: nginx +description: A basic NGINX HTTP server +version: 0.1.0 +kubeVersion: ">=1.2.0" +keywords: + - http + - nginx + - www + - web +home: https://github.com/kubernetes/helm +sources: + - https://hub.docker.com/_/nginx/ +maintainers: + - name: technosophos + email: mbutcher@deis.com +`, + "charts/nginx/values.yaml": `--- +# Declare name/value pairs to be passed into your templates. +replicaCount: 1 +restartPolicy: Never +index: >- +

Hello

+

This is a test

+image: + repository: nginx + tag: 1.11.0 + pullPolicy: IfNotPresent +`, + "charts/nginx/templates/deployment.yaml": `--- +apiVersion: extensions/v1beta1 +kind: Deployment +metadata: + name: {{ template "nginx.fullname" . }} + labels: + app: {{ template "nginx.name" . }} +spec: + replicas: {{ .Values.replicaCount }} + template: + metadata: +{{- if .Values.podAnnotations }} + # Allows custom annotations to be specified + annotations: +{{ toYaml .Values.podAnnotations | indent 8 }} +{{- end }} + labels: + app: {{ template "nginx.name" . }} + release: {{ .Release.Name }} + spec: + containers: + - name: {{ template "nginx.name" . }} + image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" + imagePullPolicy: {{ .Values.image.pullPolicy }} + ports: + - name: http + containerPort: 80 + protocol: TCP +`, } var FilesUpdated = map[string]string{ diff --git a/sync/sync_test.go b/sync/sync_test.go index 368c2c4b2..ee4168b12 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/exec" - "path/filepath" "reflect" "strings" "testing" @@ -45,11 +44,11 @@ func TestSync(t *testing.T) { } checkClusterMatchesFiles(t, manifests, clus, checkout.ManifestDir()) - for file := range testfiles.Files { - if err := execCommand("rm", filepath.Join(checkout.ManifestDir(), file)); err != nil { + for _, res := range testfiles.ServiceMap(checkout.ManifestDir()) { + if err := execCommand("rm", res[0]); err != nil { t.Fatal(err) } - commitAction := &git.CommitAction{Author: "", Message: "deleted " + file} + commitAction := &git.CommitAction{Author: "", Message: "deleted " + res[0]} if err := checkout.CommitAndPush(context.Background(), commitAction, nil); err != nil { t.Fatal(err) } From 8a4f83532e03ff20de7d895603749d6d9281342f Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Fri, 9 Mar 2018 12:40:21 +0000 Subject: [PATCH 3/3] Mention the Chart skipping in docs --- site/requirements.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/site/requirements.md b/site/requirements.md index f5b92fb32..43ed615e0 100644 --- a/site/requirements.md +++ b/site/requirements.md @@ -25,6 +25,11 @@ Flux has some requirements of the files it finds in your git repo. namespace in which you want them to run. Otherwise, the conventional default (`"default"`) will be assumed. + * Flux will ignore directories that look like Helm charts, to avoid + applying templated YAML manifests. A directory will be skipped if + its contents include the files `Chart.yaml` and `values.yaml`, as + these are the (only) mandatory components of a Helm chart. + It is _not_ a requirement that the files are arranged in any particular way into directories. Flux will look in subdirectories for YAML files recursively, but does not infer any meaning from the