From 3332621c4418eed358da357ea56cd8b0d99f961a Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Mon, 25 Nov 2019 21:04:47 +0100 Subject: [PATCH 01/19] KEP-0017: Pipe task implementation Summary: as defined in KEP-0017 we introduce a new Pipe task. Given a task specification like: ``` tasks: - name: genfiles kind: Pipe spec: container: container.yaml pipe: - file: /tmp/foo.txt kind: Secret # or ConfigMap key: foo ``` KUDO will: - create a Pod with the provided `container.yaml` - wait for successful execution of the container - copy out specified pipe files - store them in the API server (in the above example as a Secret) - the secret can be referenced in the subsequent resources as `{{Pipes.foo}}` For more information about the implementation please consult the KEP-0017. Fixes: #774 --- go.mod | 2 + go.sum | 10 + keps/0017-pipe-tasks.md | 55 ++- .../kudo/v1beta1/operatorversion_types.go | 14 + .../kudo/v1beta1/zz_generated.deepcopy.go | 38 ++ pkg/engine/health/health.go | 7 + pkg/engine/task/pod_exec.go | 87 ++++ pkg/engine/task/task.go | 80 +++- pkg/engine/task/task_apply.go | 2 +- pkg/engine/task/task_apply_test.go | 5 +- pkg/engine/task/task_pipe.go | 427 ++++++++++++++++++ pkg/engine/task/task_pipe_test.go | 68 +++ pkg/kudoctl/packages/package.go | 8 +- 13 files changed, 761 insertions(+), 42 deletions(-) create mode 100644 pkg/engine/task/pod_exec.go create mode 100644 pkg/engine/task/task_pipe.go create mode 100644 pkg/engine/task/task_pipe_test.go diff --git a/go.mod b/go.mod index 758a8edb1..777d08876 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/Masterminds/sprig v2.22.0+incompatible github.com/Microsoft/go-winio v0.4.14 // indirect github.com/containerd/containerd v1.2.9 // indirect + github.com/davecgh/go-spew v1.1.1 github.com/docker/docker v1.4.2-0.20190916154449-92cc603036dd github.com/docker/go-connections v0.4.0 // indirect github.com/docker/go-units v0.4.0 // indirect @@ -38,6 +39,7 @@ require ( github.com/stretchr/testify v1.4.0 github.com/xlab/treeprint v0.0.0-20181112141820-a009c3971eca golang.org/x/net v0.0.0-20190923162816-aa69164e4478 + golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e golang.org/x/sys v0.0.0-20191025090151-53bf42e6b339 // indirect golang.org/x/tools v0.0.0-20191025023517-2077df36852e // indirect gopkg.in/yaml.v2 v2.2.4 diff --git a/go.sum b/go.sum index 1e84204fb..032bf2f4d 100644 --- a/go.sum +++ b/go.sum @@ -21,6 +21,7 @@ github.com/Azure/go-autorest/tracing v0.5.0/go.mod h1:r/s2XiOKccPW3HrqB+W0TQzfbt github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= +github.com/MakeNowJust/heredoc v0.0.0-20170808103936-bb23615498cd h1:sjQovDkwrZp8u+gxLtPgKGjk5hCxuy2hrRejBTA9xFU= github.com/MakeNowJust/heredoc v0.0.0-20170808103936-bb23615498cd/go.mod h1:64YHyfSL2R96J44Nlwm39UHepQbyR5q10x7iYa1ks2E= github.com/Masterminds/goutils v1.1.0 h1:zukEsf/1JZwCMgHiK3GZftabmxiCw4apj3a28RPBiVg= github.com/Masterminds/goutils v1.1.0/go.mod h1:8cTjp+g8YejhMuvIA5y2vz3BpJxksy863GQaJW2MFNU= @@ -60,6 +61,7 @@ github.com/blang/semver v3.5.0+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnweb github.com/bombsimon/wsl v1.2.5 h1:9gTOkIwVtoDZywvX802SDHokeX4kW1cKnV8ZTVAPkRs= github.com/bombsimon/wsl v1.2.5/go.mod h1:43lEF/i0kpXbLCeDXL9LMT8c92HyBywXb0AsgMHYngM= github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc= +github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5 h1:7aWHqerlJ41y6FOsEUvknqgXnGmJyJSbjhAWq5pO4F8= github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5/go.mod h1:/iP1qXHoty45bqomnu2LM+VVyAEdWN+vtSHGlQgyxbw= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/containerd/containerd v1.2.9 h1:6tyNjBmAMG47QuFPIT9LgiiexoVxC6qpTGR+eD0R0Z8= @@ -75,9 +77,11 @@ github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3Ee github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7 h1:u9SHYsPQNyt5tgDm3YN7+9dYrpK96E5wFilTFWIDZOM= github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= +github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e h1:Wf6HqHfScWJN9/ZjdUKyjop4mf3Qdd+1TvvltAvM3m8= github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/coreos/pkg v0.0.0-20180108230652-97fdf19511ea h1:n2Ltr3SrfQlf/9nOna1DoGKxLx3qTSI8Ttl6Xrqp6mw= github.com/coreos/pkg v0.0.0-20180108230652-97fdf19511ea/go.mod h1:E3G3o1h8I7cfcXa63jLwjI0eiQQMgzzUDFVpN/nH/eA= +github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f h1:lBNOc5arjvs8E5mO2tbpBpLoyyu8B6e44T7hJy6potg= github.com/coreos/pkg v0.0.0-20180928190104-399ea9e2e55f/go.mod h1:E3G3o1h8I7cfcXa63jLwjI0eiQQMgzzUDFVpN/nH/eA= github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwcJI5acqYI6dE= github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY= @@ -99,10 +103,12 @@ github.com/docker/go-connections v0.4.0/go.mod h1:Gbd7IOopHjR8Iph03tsViu4nIes5Xh github.com/docker/go-units v0.3.3/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= github.com/docker/go-units v0.4.0 h1:3uh0PgVws3nIA0Q+MwDC8yjEPf9zjRfZZWXZYDct3Tw= github.com/docker/go-units v0.4.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk= +github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96 h1:cenwrSVm+Z7QLSV/BsnenAOcDXdX4cMv4wP0B/5QbPg= github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96/go.mod h1:Qh8CwZgvJUkLughtfhJv5dyTYa91l1fOUCrgjqmcifM= github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE= github.com/dustinkirkland/golang-petname v0.0.0-20170921220637-d3c2ba80e75e h1:bRcq7ruHMqCVB/ugLbBylx+LrccNACFDEaqAD/aZ80Q= github.com/dustinkirkland/golang-petname v0.0.0-20170921220637-d3c2ba80e75e/go.mod h1:V+Qd57rJe8gd4eiGzZyg4h54VLHmYVVw54iMnlAMrF8= +github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e h1:p1yVGRW3nmb85p1Sh1ZJSDm4A4iKLS5QNbvUHMgGu/M= github.com/elazarl/goproxy v0.0.0-20170405201442-c4fc26588b6e/go.mod h1:/Zj4wYkgs4iZTTu3o/KG3Itv/qCCa8VVMlb3i9OVuzc= github.com/emicklei/go-restful v0.0.0-20170410110728-ff4f55a20633/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= github.com/emicklei/go-restful v2.9.5+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT5YD2WR4BBwUdeQoFo8l/7tVs= @@ -111,6 +117,7 @@ github.com/emicklei/go-restful v2.9.6+incompatible/go.mod h1:otzb+WCGbkyDHkqmQmT github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= github.com/evanphx/json-patch v4.5.0+incompatible h1:ouOWdg56aJriqS0huScTkVXPC5IcNrDCXZ6OoTAWu7M= github.com/evanphx/json-patch v4.5.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLiYLvXMP4fmwYFNcr97nuDLSk= +github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d h1:105gxyaGwCFad8crR9dcMQWvV9Hvulu6hwUh4tWPJnM= github.com/exponent-io/jsonpath v0.0.0-20151013193312-d6023ce2651d/go.mod h1:ZZMPRZwes7CROmyNKgQzC3XPs6L/G2EJLHddWejkmf4= github.com/fatih/camelcase v1.0.0 h1:hxNvNX/xYBp0ovncs8WyWZrOrpBNub/JfaMvbURyft8= github.com/fatih/camelcase v1.0.0/go.mod h1:yN2Sb0lFhZJUdVvtELVWefmrXpuZESvPmqwoZc+/fpc= @@ -392,6 +399,7 @@ github.com/mitchellh/copystructure v1.0.0/go.mod h1:SNtv71yrdKgLRyLFxmLdkAbkKEFW github.com/mitchellh/go-homedir v1.1.0 h1:lukF9ziXFxDFPkA1vsr5zpc1XuPDn/wFntq5mG+4E0Y= github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0= github.com/mitchellh/go-ps v0.0.0-20190716172923-621e5597135b/go.mod h1:r1VsdOzOPt1ZSrGZWFoNhsAedKnEd6r9Np1+5blZCWk= +github.com/mitchellh/go-wordwrap v1.0.0 h1:6GlHJ/LTGMrIJbwgdqdl2eEH8o+Exx/0m8ir9Gns0u4= github.com/mitchellh/go-wordwrap v1.0.0/go.mod h1:ZXFpozHsX6DPmq2I0TCekCxypsnAUbP2oI0UX1GXzOo= github.com/mitchellh/mapstructure v1.1.2 h1:fmNYVwqnSfB9mZU6OS2O6GsXM+wcskZDuKQzvN1EDeE= github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y= @@ -468,6 +476,7 @@ github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c/go.mod h1: github.com/remyoudompheng/bigfft v0.0.0-20170806203942-52369c62f446/go.mod h1:uYEyJGbgTkfkS4+E/PavXkNJcbFIpEtjt2B0KDQ5+9M= github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg= github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= +github.com/russross/blackfriday v1.5.2 h1:HyvC0ARfnZBqnXwABFeSZHpKvJHJJfPz81GNueLj0oo= github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g= github.com/securego/gosec v0.0.0-20191002120514-e680875ea14d h1:BzRvVq1EHuIjxpijCEKpAxzKUUMurOQ4sknehIATRh8= github.com/securego/gosec v0.0.0-20191002120514-e680875ea14d/go.mod h1:w5+eXa0mYznDkHaMCXA4XYffjlH+cy1oyKbfzJXa2Do= @@ -613,6 +622,7 @@ golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20190227155943-e225da77a7e6/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e h1:vcxGaoTs7kV8m5Np9uUNQin4BrLOthgV7252N8V+FwY= golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20170830134202-bb24a47a89ea/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180117170059-2c42eef0765b/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= diff --git a/keps/0017-pipe-tasks.md b/keps/0017-pipe-tasks.md index 49e8d1e0b..27492be30 100644 --- a/keps/0017-pipe-tasks.md +++ b/keps/0017-pipe-tasks.md @@ -43,21 +43,20 @@ Allowing to pipe all kind of files (>1Mb) between tasks requires a general-purpo ## Proposal -This section describes how pipe tasks and files they produce are configured in the operator. Here is a task definition that produces a file that will be stored as a Secret, referenced by `{{.Pipes.Certificate}}` key: +This section describes how pipe tasks and files they produce are configured in the operator. Here is a pipe task definition that produces a file that will be stored as a Secret: ```yaml tasks: - name: gencert kind: Pipe spec: - containerSpec: - ... + container: cert-container.yaml pipe: - file: /usr/share/MyKey.key - kind: Secret # ConfigMap - key: {{.Pipes.Certificate}} + - file: /usr/share/MyKey.key + kind: Secret # ConfigMap + key: Mycertificate ``` -`containerSpec` field is described in detail below. `pipe` field defines how the produced file will be stored and referenced. The key `{{.Pipes.Certificate}}` value will be generated by KUDO to avoid collisions and to make sure that it is not used before the file is generated. In the above example we would generate a secret name like `instance-myapp.deploy.bootstrap.gencert.pipes.certificate-#hash` to capture instance name along with plan/phase/step/task of the secret origin and the hash of its content. KUDO would use this hash to avoid recreating secrets for unchanged files on a plan rerun. We would also use labels ensuring that the secret is cleaned up when the corresponding Instance is removed. +`container` field is described in detail below. `key` will be used by the resources referencing generated file as following: `{{.Pipes.Mycertificate}}`. In the above example we would create a secret named `instance-myapp.deploy.bootstrap.gencert.mycertificate` to capture instance name along with plan/phase/step/task of the secret origin. The secret name would be stable so that a user can rerun the certificate generating task and reload all pods using it. Pipe file name will be used as Secret/ConfigMap data key. We would also use labels ensuring that the secret is cleaned up when the corresponding Instance is removed. `pipe` field is an array and can define how multiple generated files are stored and referenced. The corresponding `gencert` task can be used as usual in e.g.: ```yaml @@ -75,28 +74,28 @@ plans: Note that piped files has to be generated before they can be used. In the example above, `bootstrap` phase has a strategy `serial` so that certificate generated in the `gencert` step can be used in subsequent steps. Or stated differently resources can not reference piped secrets/configmaps generated within the same step or within a parallel series of steps (it has to be a different step in the phase with serial strategy or a different phase). -For the pipe task `containerSpec`, we allow a [ContainerSpec](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#container-v1-core) to be specified. Reasons for that are explained below in the implementation details. The ContainerSpec has to define a shared volume where the files are stored. +For the pipe task `container`, we allow a [core/v1 container](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#container-v1-core) resource to be specified. Reasons for that are explained below in the implementation details. The container has to define a volumeMount where the files are stored. ```yaml -containerSpec: - volumes: - - name: shared-data - emptyDir: {} +volumes: +- name: shared-data + emptyDir: {} - containers: - - name: gencert - image: frapsoft/openssl - imagePullPolicy: Always - command: [ "sh", "-c" ] - args: - - openssl req -new -newkey rsa:4096 -x509 -sha256 -days 365 -nodes -out MyCertificate.crt -keyout /usr/share/MyKey.key - volumeMounts: - - name: shared-data - mountPath: /usr/share/ +containers: + - name: gencert + image: frapsoft/openssl + imagePullPolicy: Always + command: [ "sh", "-c" ] + args: + - openssl req -new -newkey rsa:4096 -x509 -sha256 -days 365 -nodes -out MyCertificate.crt -keyout /usr/share/MyKey.key + volumeMounts: + - name: shared-data + mountPath: /usr/share/ ``` -Any subsequent step resource (if the phase strategy is `serial`) might reference previously generated file by its key `{{.Pipes.Certificate}}` e.g.: +Any subsequent step resource (if the phase strategy is `serial`) might reference previously generated file by its key e.g.: ```yaml +# some Pod spec referenced in a subsequent step spec: containers: - name: myapp @@ -107,18 +106,18 @@ spec: volumes: - name: cert secret: - secretName: {{.Pipes.Certificate}} + secretName: {{.Pipes.Mycertificate}} ``` ### Limitations -- File generating container has to be side-effect free (meaning side-effects that are observable outside of the container like a 3rd party API call) as the container might be executed multiple times on failure. If that's not the case, `restartPolicy: Never` has to be set which would prevent the task from finishing successfully. +- File generating container has to be side-effect free (meaning side-effects that are observable outside of the container like a 3rd party API call) as the container might be executed multiple times on failure. If that's not the case, `restartPolicy: OnFailure` has to be set to ensure the pod would be restarted if there is an issue with the initContainer. - Only files <1Mb are applicable to be stored as ConfigMap or Secret. - Only one resource per pipe task is allowed. If needed multiple tasks must be used. ### Implementation Details/Notes/Constraints There are several ways to implement pipe tasks, each one having its challenges and complexities. The approach below allows us not to worry about Pod container life-cycle as well as keep the storing logic in the KUDO controller: -- Provided ContainerSpec is injected into a Pod as an [InitContainer](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). This is the simplest way to wait for container completion. This is also the reason why pipe task resource definition is a ContainerSpec and not a complete Pod specification. Pod init container can not have Lifecycle actions, Readiness probes, or Liveness probes fields defined. +- Provided container is injected into a Pod as an [InitContainer](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). This is the simplest way to wait for container completion. This is also the reason why pipe task resource definition is a container and not a complete Pod specification. Pod init container can not have Lifecycle actions, Readiness probes, or Liveness probes fields defined. - The main container is a `busybox` image, running the `sleep infinity` command, which purpose is to wait for KUDO to extract and store the files. - Pod status `READY: 1/1, STATUS: Running` means that the init container has run successfully. As this point KUDO can copy out referenced files using `kubectl cp` and store them as specified. - Once files are stored, KUDO can delete the Pod and proceed to the next task. @@ -134,7 +133,7 @@ spec: - name: shared-data emptyDir: {} - # Inject provided ContainerSpec generating /tmp/foo.txt + # Inject provided container generating /tmp/foo.txt initContainers: - name: init image: busybox @@ -165,7 +164,7 @@ foo-bar-bazz ## Alternatives -An alternative approach would use the provided ContainerSpec as the main container (or let the user provide a complete Pod spec). We would inject a sidecar with a go executable which would: +An alternative approach would use the provided `container` as the main container (or let the user provide a complete Pod spec). We would inject a sidecar with a go executable which would: - Use controller runtime, watch its own Pod status and wait for termination of the main container - Once the main container exits, it would copy the referenced files and store them as specified - We would use `restartPolicy: Never/OnFailure` to prevent the main container from restarting diff --git a/pkg/apis/kudo/v1beta1/operatorversion_types.go b/pkg/apis/kudo/v1beta1/operatorversion_types.go index f9fbdec45..2bd51ae0a 100644 --- a/pkg/apis/kudo/v1beta1/operatorversion_types.go +++ b/pkg/apis/kudo/v1beta1/operatorversion_types.go @@ -121,6 +121,7 @@ type Task struct { type TaskSpec struct { ResourceTaskSpec DummyTaskSpec + PipeTaskSpec } // ResourceTaskSpec is referencing a list of resources @@ -135,6 +136,19 @@ type DummyTaskSpec struct { Done bool `json:"done"` } +// PipeTask specifies a task that generates files and stores them for later usage in subsequent tasks +type PipeTaskSpec struct { + Container string `json:"container"` + Pipe []PipeSpec `json:"pipe"` +} + +// PipeSpec describes how a file generated by a PipeTask is stored and referenced +type PipeSpec struct { + File string `json:"file"` + Kind string `json:"kind"` + Key string `json:"key"` +} + // OperatorVersionStatus defines the observed state of OperatorVersion. type OperatorVersionStatus struct { // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster diff --git a/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go b/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go index e11ff47d9..e55cc3558 100644 --- a/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/kudo/v1beta1/zz_generated.deepcopy.go @@ -544,6 +544,43 @@ func (in *PhaseStatus) DeepCopy() *PhaseStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PipeSpec) DeepCopyInto(out *PipeSpec) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PipeSpec. +func (in *PipeSpec) DeepCopy() *PipeSpec { + if in == nil { + return nil + } + out := new(PipeSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PipeTaskSpec) DeepCopyInto(out *PipeTaskSpec) { + *out = *in + if in.Pipe != nil { + in, out := &in.Pipe, &out.Pipe + *out = make([]PipeSpec, len(*in)) + copy(*out, *in) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PipeTaskSpec. +func (in *PipeTaskSpec) DeepCopy() *PipeTaskSpec { + if in == nil { + return nil + } + out := new(PipeTaskSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Plan) DeepCopyInto(out *Plan) { *out = *in @@ -671,6 +708,7 @@ func (in *TaskSpec) DeepCopyInto(out *TaskSpec) { *out = *in in.ResourceTaskSpec.DeepCopyInto(&out.ResourceTaskSpec) out.DummyTaskSpec = in.DummyTaskSpec + in.PipeTaskSpec.DeepCopyInto(&out.PipeTaskSpec) return } diff --git a/pkg/engine/health/health.go b/pkg/engine/health/health.go index 3e23d0cb5..75173d60c 100644 --- a/pkg/engine/health/health.go +++ b/pkg/engine/health/health.go @@ -6,6 +6,7 @@ import ( "log" "reflect" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" @@ -67,6 +68,12 @@ func IsHealthy(obj runtime.Object) error { } return fmt.Errorf("instance's active plan is in state %v", obj.Status.AggregatedStatus.Status) + case *corev1.Pod: + if obj.Status.Phase == corev1.PodRunning { + return nil + } + return fmt.Errorf("pod \"%v\" is not running yet: %s", obj.Name, obj.Status.Phase) + // unless we build logic for what a healthy object is, assume it's healthy when created. default: log.Printf("HealthUtil: Unknown type %s is marked healthy by default", reflect.TypeOf(obj)) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go new file mode 100644 index 000000000..0d286fc24 --- /dev/null +++ b/pkg/engine/task/pod_exec.go @@ -0,0 +1,87 @@ +package task + +import ( + "fmt" + "io" + + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/remotecommand" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" +) + +// PodExec defines a command that will be executed in a running Pod. +// RestCfg - The REST configuration for the cluster. +// PodName - The pod name on which to execute the command. +// PodNamespace - Namespace of the pod +// ContainerName - optional container to execute the command in. If empty, first container is taken +// Args - The command (and args) to execute. +// In - An (optional) command input stream. +// Out - The command output stream set by `Run()`. +// Err - the command error stream set by `Run()`. +type PodExec struct { + RestCfg *rest.Config + PodName string + PodNamespace string + ContainerName string + Args []string + In io.Reader + Out io.Writer + Err io.Writer +} + +// Run execution the pod command. +func (pe *PodExec) Run() error { + codec := serializer.NewCodecFactory(scheme.Scheme) + restClient, err := apiutil.RESTClientForGVK( + schema.GroupVersionKind{ + Version: "v1", + Kind: "pods", + }, + pe.RestCfg, + codec) + if err != nil { + return err + } + + req := restClient. + Post(). + Resource("pods"). + Name(pe.PodName). + Namespace(pe.PodNamespace). + SubResource("exec") + + poe := &v1.PodExecOptions{ + Stdin: pe.In != nil, + Stdout: pe.Out != nil, + Stderr: pe.Err != nil, + TTY: false, + Container: pe.ContainerName, + Command: pe.Args, + } + req.VersionedParams(poe, scheme.ParameterCodec) + + exec, err := remotecommand.NewSPDYExecutor(pe.RestCfg, "POST", req.URL()) + if err != nil { + return err + } + + so := remotecommand.StreamOptions{ + Stdin: pe.In, + Stdout: pe.Out, + Stderr: pe.Err, + Tty: false, + } + + go func(exec remotecommand.Executor, so remotecommand.StreamOptions) { + err = exec.Stream(so) + if err != nil { + fmt.Printf("error during pod command %+v execution: %v", pe, err) + } + }(exec, so) + + return nil +} diff --git a/pkg/engine/task/task.go b/pkg/engine/task/task.go index 0cc2a2861..0ade18975 100644 --- a/pkg/engine/task/task.go +++ b/pkg/engine/task/task.go @@ -1,7 +1,9 @@ package task import ( + "errors" "fmt" + "regexp" "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/engine/renderer" @@ -36,49 +38,105 @@ const ( ApplyTaskKind = "Apply" DeleteTaskKind = "Delete" DummyTaskKind = "Dummy" + PipeTaskKind = "Pipe" ) var ( - taskRenderingError = "TaskRenderingError" - taskEnhancementError = "TaskEnhancementError" - dummyTaskError = "DummyTaskError" + taskRenderingError = "TaskRenderingError" + taskEnhancementError = "TaskEnhancementError" + dummyTaskError = "DummyTaskError" + resourceUnmarshalError = "ResourceUnmarshalError" + resourceValidationError = "ResourceValidationError" ) // Build factory method takes an v1beta1.Task and returns a corresponding Tasker object func Build(task *v1beta1.Task) (Tasker, error) { switch task.Kind { case ApplyTaskKind: - return newApply(task), nil + return newApply(task) case DeleteTaskKind: - return newDelete(task), nil + return newDelete(task) case DummyTaskKind: - return newDummy(task), nil + return newDummy(task) + case PipeTaskKind: + return newPipe(task) default: return nil, fmt.Errorf("unknown task kind %s", task.Kind) } } -func newApply(task *v1beta1.Task) ApplyTask { +func newApply(task *v1beta1.Task) (Tasker, error) { + // validate ApplyTask + if len(task.Spec.ResourceTaskSpec.Resources) == 0 { + return nil, errors.New("task validation error: apply task has an empty resource list. if that's what you need, use a Dummy task instead") + } + return ApplyTask{ Name: task.Name, Resources: task.Spec.ResourceTaskSpec.Resources, - } + }, nil } -func newDelete(task *v1beta1.Task) DeleteTask { +func newDelete(task *v1beta1.Task) (Tasker, error) { + // validate DeleteTask + if len(task.Spec.ResourceTaskSpec.Resources) == 0 { + return nil, errors.New("task validation error: delete task has an empty resource list. if that's what you need, use a Dummy task instead") + } + return DeleteTask{ Name: task.Name, Resources: task.Spec.ResourceTaskSpec.Resources, - } + }, nil } -func newDummy(task *v1beta1.Task) DummyTask { +func newDummy(task *v1beta1.Task) (Tasker, error) { return DummyTask{ Name: task.Name, WantErr: task.Spec.DummyTaskSpec.WantErr, Fatal: task.Spec.DummyTaskSpec.Fatal, Done: task.Spec.DummyTaskSpec.Done, + }, nil +} + +func newPipe(task *v1beta1.Task) (Tasker, error) { + // validate PipeTask + if len(task.Spec.PipeTaskSpec.Pipe) == 0 { + return nil, errors.New("task validation error: pipe task has an empty pipe files list") + } + + var pipeFiles []PipeFile + for _, pp := range task.Spec.PipeTaskSpec.Pipe { + pf := PipeFile{File: pp.File, Kind: pp.Kind, Key: pp.Key} + // validate pipe file + if err := validPipeFile(pf); err != nil { + return nil, err + } + pipeFiles = append(pipeFiles, pf) + } + + return PipeTask{ + Name: task.Name, + Container: task.Spec.PipeTaskSpec.Container, + PipeFiles: pipeFiles, + }, nil +} + +var ( + pipeFileKeyRe = regexp.MustCompile(`^[a-zA-Z0-9_\-]+$`) //a-z, A-Z, 0-9, _ and - are allowed + pipeFileKindRe = regexp.MustCompile("^Secret$|^ConfigMap$") // only Secret or ConfigMap are allowed +) + +func validPipeFile(pf PipeFile) error { + if pf.File == "" { + return fmt.Errorf("task validation error: pipe file is empty: %v", pf) + } + if !pipeFileKindRe.MatchString(pf.Kind) { + return fmt.Errorf("task validation error: invalid pipe kind (must be Secret or ConfigMap): %v", pf) + } + if !pipeFileKeyRe.MatchString(pf.Key) { + return fmt.Errorf("task validation error: invalid pipe key (only letters, numbers and _ and - are allowed): %v", pf) } + return nil } // fatalExecutionError is a helper method providing proper wrapping an message for the ExecutionError diff --git a/pkg/engine/task/task_apply.go b/pkg/engine/task/task_apply.go index a451b7cc9..47b8f4af3 100644 --- a/pkg/engine/task/task_apply.go +++ b/pkg/engine/task/task_apply.go @@ -58,7 +58,7 @@ func (at ApplyTask) Run(ctx Context) (bool, error) { // apply method takes a slice of k8s object and applies them using passed client. If an object // doesn't exist it will be created. An already existing object will be patched. func apply(ro []runtime.Object, c client.Client) ([]runtime.Object, error) { - applied := make([]runtime.Object, len(ro)) + applied := make([]runtime.Object, 0) for _, r := range ro { key, _ := client.ObjectKeyFromObject(r) diff --git a/pkg/engine/task/task_apply_test.go b/pkg/engine/task/task_apply_test.go index e3b737680..c085645d9 100644 --- a/pkg/engine/task/task_apply_test.go +++ b/pkg/engine/task/task_apply_test.go @@ -88,7 +88,7 @@ func TestApplyTask_Run(t *testing.T) { }, }, { - name: "succeeds when the resource is healthy (unknown type Pod is marked healthy by default)", + name: "succeeds when the resource is healthy (pod has PodStatus.Phase = Running)", task: ApplyTask{ Name: "task", Resources: []string{"pod"}, @@ -143,6 +143,9 @@ func pod(name string, namespace string) *corev1.Pod { Namespace: namespace, }, Spec: corev1.PodSpec{}, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, } return pod } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go new file mode 100644 index 000000000..a6946412e --- /dev/null +++ b/pkg/engine/task/task_pipe.go @@ -0,0 +1,427 @@ +package task + +import ( + "archive/tar" + "errors" + "fmt" + "io" + "log" + "os" + "path" + "path/filepath" + "regexp" + "strings" + + "github.com/spf13/afero" + "golang.org/x/sync/errgroup" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/yaml" +) + +var ( + pipeTaskError = "PipeTaskError" + + pipePodContainerName = "waiter" +) + +type PipeTask struct { + Name string + Container string + PipeFiles []PipeFile +} + +type PipeFile struct { + File string + Kind string + Key string +} + +func (pt PipeTask) Run(ctx Context) (bool, error) { + // 1. - Render container template - + rendered, err := render([]string{pt.Container}, ctx.Templates, ctx.Parameters, ctx.Meta) + if err != nil { + return false, fatalExecutionError(err, taskRenderingError, ctx.Meta) + } + + // 2. - Create core/v1 container object - + container, err := unmarshal(rendered[pt.Container]) + if err != nil { + return false, fatalExecutionError(err, resourceUnmarshalError, ctx.Meta) + } + + // 3. - Validate the container object - + err = validate(container, pt.PipeFiles) + if err != nil { + return false, fatalExecutionError(err, resourceValidationError, ctx.Meta) + } + + // 4. - Create a pod using the container - + podName := podName(ctx) + podStr, err := pipePod(container, podName) + if err != nil { + return false, fatalExecutionError(err, pipeTaskError, ctx.Meta) + } + + // 5. - Kustomize pod with metadata + podObj, err := kustomize(map[string]string{"pipe-pod.yaml": podStr}, ctx.Meta, ctx.Enhancer) + if err != nil { + return false, fatalExecutionError(err, taskEnhancementError, ctx.Meta) + } + + // 6. - Apply pod using the client - + podObj, err = apply(podObj, ctx.Client) + if err != nil { + return false, err + } + + // 7. - Wait for the pod to be ready - + err = isHealthy(podObj) + // once the pod is Ready, it means that its initContainer finished successfully and we can copy + // out the generated files. An error during a health check is not treated as task execution error + if err != nil { + return false, nil + } + + // 8. - Copy out the pipe files - + log.Printf("PipeTask: %s/%s copying pipe files", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName) + fs := afero.NewMemMapFs() + pipePod := podObj[0].(*corev1.Pod) + + err = copyFiles(fs, pt.PipeFiles, pipePod, ctx) + if err != nil { + return false, err + } + + // 9. - Create k8s artifacts (ConfigMap/Secret) from the pipe files - + log.Printf("PipeTask: %s/%s creating pipe artifacts", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName) + artStr, err := pipeFiles(fs, pt.PipeFiles, ctx) + if err != nil { + return false, err + } + + // 10. - Kustomize artifacts - + artObj, err := kustomize(artStr, ctx.Meta, ctx.Enhancer) + if err != nil { + return false, fatalExecutionError(err, taskEnhancementError, ctx.Meta) + } + + // 11. - Apply artifacts using the client - + _, err = apply(artObj, ctx.Client) + if err != nil { + return false, err + } + + // 12. - Delete pipe pod - + log.Printf("PipeTask: %s/%s deleting pipe pod", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName) + err = delete(podObj, ctx.Client) + if err != nil { + return false, err + } + + return true, nil +} + +func unmarshal(ctrStr string) (*corev1.Container, error) { + ctr := &corev1.Container{} + err := yaml.Unmarshal([]byte(ctrStr), ctr) + if err != nil { + return nil, fmt.Errorf("failed to unmarshall pipe container: %v", err) + } + return ctr, nil +} + +// this regexp is used by K87 for validation Secret/ConfigMap data keys. +var pipeFileRe = regexp.MustCompile(`[-._a-zA-Z0-9]+`) + +func isRelative(base, file string) bool { + rp, err := filepath.Rel(base, file) + return err == nil && !strings.HasPrefix(rp, ".") +} + +// validate method validates passed pipe container. It is expected to: +// - have exactly one volume mount +// - not have readiness probes (as k87 does not support them for init containers) +// - pipe files should have valid names and exist within the volume mount +func validate(ctr *corev1.Container, ff []PipeFile) error { + if len(ctr.VolumeMounts) != 1 { + return errors.New("pipe container should have exactly one volume mount") + } + + if ctr.ReadinessProbe != nil { + return errors.New("pipe container does not support readiness probes") + } + + // check if all referenced pipe files are children of the container mountPath + mountPath := ctr.VolumeMounts[0].MountPath + for _, f := range ff { + if !isRelative(mountPath, f.File) { + return fmt.Errorf("pipe file %s should be a child of %s mount path", f.File, mountPath) + } + + fileName := path.Base(f.File) + // k8s uses the same validation for Secret/ConfigMap data keys: a valid key must consist of alphanumeric + // characters, '-', '_' or '.' (e.g. 'key.name', or 'KEY_NAME', or 'key-name', regex used for + // validation is '[-._a-zA-Z0-9]+') + if !pipeFileRe.MatchString(fileName) { + return fmt.Errorf("pipe file name %s should only contain alphanumeric characters, '.', '_' and '-'", fileName) + } + } + return nil +} + +func pipePod(ctr *corev1.Container, name string) (string, error) { + volumeName := ctr.VolumeMounts[0].Name + mountPath := ctr.VolumeMounts[0].MountPath + + pod := corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: corev1.PodSpec{ + Volumes: []corev1.Volume{ + { + Name: volumeName, + VolumeSource: corev1.VolumeSource{EmptyDir: nil}, + }, + }, + InitContainers: []corev1.Container{*ctr}, + Containers: []corev1.Container{ + { + Name: pipePodContainerName, + Image: "busybox", + Command: []string{"/bin/sh", "-c"}, + Args: []string{"sleep infinity"}, + VolumeMounts: []corev1.VolumeMount{ + { + Name: volumeName, + MountPath: mountPath, + }, + }, + }, + }, + RestartPolicy: corev1.RestartPolicyOnFailure, + }, + } + + b, err := yaml.Marshal(pod) + if err != nil { + return "", fmt.Errorf("failed to create pipe task pod: %v", err) + } + + return string(b), nil +} + +func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { + restCfg, err := config.GetConfig() + if err != nil { + return fatalExecutionError(fmt.Errorf("failed to fetch cluster REST config: %v", err), pipeTaskError, ctx.Meta) + } + + var g errgroup.Group + + for _, f := range ff { + f := f + log.Printf("PipeTask: %s/%s copying pipe file %s", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName, f.File) + g.Go(func() error { + return copyFile(fs, f, pod, restCfg) + }) + } + + err = g.Wait() + return err +} + +func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) error { + reader, stdout := io.Pipe() + + defer reader.Close() + defer stdout.Close() + + pe := PodExec{ + RestCfg: restCfg, + PodName: pod.Name, + PodNamespace: pod.Namespace, + ContainerName: pipePodContainerName, + Args: []string{"tar", "cf", "-", pf.File}, + In: nil, + Out: stdout, + Err: nil, + } + + if err := pe.Run(); err != nil { + return fmt.Errorf("failed to copy pipe file. err: %v", err) + } + + if err := untarFile(fs, "", reader, pf.File); err != nil { + return fmt.Errorf("failed to untar pipe file: %v", err) + } + + return nil +} + +// pipeFiles iterates through passed pipe files and their copied data, reads them, constructs k8s artifacts +// and marshals them. +func pipeFiles(fs afero.Fs, files []PipeFile, ctx Context) (map[string]string, error) { + artifacts := map[string]string{} + + for _, pf := range files { + data, err := afero.ReadFile(fs, pf.File) + if err != nil { + return nil, fmt.Errorf("error opening pipe file %s", pf.File) + } + + // API server has a limit of 1Mb for Secret/ConfigMap + if len(data) > 1024*1024 { + return nil, fatalExecutionError(fmt.Errorf("pipe file %s size (%d bytes) exceeds max size limit of 1Mb", pf.File, len(data)), pipeTaskError, ctx.Meta) + } + + var art string + switch pf.Kind { + case "Secret": + art, err = pipeSecret(pf, data, ctx) + case "ConfigMap": + art, err = pipeConfigMap(pf, data, ctx) + default: + return nil, fmt.Errorf("unknown pipe file kind: %+v", pf) + } + + if err != nil { + return nil, err + } + artifacts[pf.Key] = art + } + + return artifacts, nil +} + +// pipeSecret method creates a core/v1/Secret object using passed data. Pipe file name is used +// as Secret data key. Secret name will be of the form ....- +func pipeSecret(pf PipeFile, data []byte, ctx Context) (string, error) { + name := artifactName(ctx, pf.Key) + key := path.Base(pf.File) + + secret := corev1.Secret{ + TypeMeta: metav1.TypeMeta{ + Kind: "Secret", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Data: map[string][]byte{key: data}, + Type: corev1.SecretTypeOpaque, + } + + b, err := yaml.Marshal(secret) + if err != nil { + return "", fmt.Errorf("failed to marshal pipe secret for pipe file %s: %v", pf.File, err) + } + + return string(b), nil +} + +// pipeConfigMap method creates a core/v1/ConfigMap object using passed data. Pipe file name is used +// as ConfigMap data key. ConfigMap name will be of the form ....- +func pipeConfigMap(pf PipeFile, data []byte, ctx Context) (string, error) { + name := artifactName(ctx, pf.Key) + key := path.Base(pf.File) + + configMap := corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + BinaryData: map[string][]byte{key: data}, + } + + b, err := yaml.Marshal(configMap) + if err != nil { + return "", fmt.Errorf("failed to marshal pipe configMap for pipe file %s: %v", pf.File, err) + } + + return string(b), nil +} + +// untar takes a destination path and a reader; a tar reader loops over the tarfile +// creating the file structure at 'path' along the way, and writing any files +func untarFile(fs afero.Fs, dest string, r io.Reader, file string) (err error) { + tr := tar.NewReader(r) + + for { + header, err := tr.Next() + + switch { + case err == io.EOF: // if no more files are found return + return nil + + case err != nil: // return any other error + return err + + case header == nil: // if the header is nil, just skip it + continue + } + + // the target location of the file. tar strips the leading "/" however, we treat the pipe file path + // as a key to the underlying data (otherwise we'll have to start splitting paths). To avoid all + // the complexity and because we only extract one file here, the path is taken from the PipeFile configuration + target := file + + // check the file type + switch header.Typeflag { + // if it's a file create it + case tar.TypeReg: + f, err := fs.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) + if err != nil { + return err + } + + // copy over contents + if _, err := io.Copy(f, tr); err != nil { + return err + } + + // manually close here after each file operation; deferring would cause each file close + // to wait until all operations have completed. + f.Close() // nolint + + default: + fmt.Printf("skipping %s because it is not a regular file or a directory", header.Name) + } + } +} + +// podName returns a deterministic name for a pipe pod +func podName(ctx Context) string { return name(ctx, "pipe-pod") } + +// artifactName returns a deterministic name for pipe artifact (ConfigMap, Secret) +func artifactName(ctx Context, key string) string { return name(ctx, key) } + +var ( + nameRe = regexp.MustCompile(`^[^a-zA-Z0-9\-.]+`) + // TODO: this is the reqexp that API server is using for Secret/ConfigMap name validation + //artifactNameRe = regexp.MustCompile(`[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*`) +) + +// name returns a deterministic names for pipe artifacts (Pod, Secret, ConfigMap) in the form: +// ....- All characters aside from these defined in +// nameRe are removed and replaced with "" +func name(ctx Context, suffix string) string { + name := fmt.Sprintf("%s.%s.%s.%s.%s-%s", + ctx.Meta.InstanceName, + ctx.Meta.PlanName, + ctx.Meta.PhaseName, + ctx.Meta.StepName, + ctx.Meta.TaskName, + suffix) + return nameRe.ReplaceAllString(strings.ToLower(name), "") +} diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go new file mode 100644 index 000000000..7087123e9 --- /dev/null +++ b/pkg/engine/task/task_pipe_test.go @@ -0,0 +1,68 @@ +package task + +import ( + "fmt" + "testing" +) + +func Test_isRelative(t *testing.T) { + tests := []struct { + base string + file string + relative bool + }{ + { + base: "/dir", + file: "/dir/../link", + relative: false, + }, + { + base: "/dir", + file: "/dir/../../link", + relative: false, + }, + { + base: "/dir", + file: "/link", + relative: false, + }, + { + base: "/dir", + file: "/dir/link", + relative: true, + }, + { + base: "/dir", + file: "/dir/int/../link", + relative: true, + }, + { + base: "dir", + file: "dir/link", + relative: true, + }, + { + base: "dir", + file: "dir/int/../link", + relative: true, + }, + { + base: "dir", + file: "dir/../../link", + relative: false, + }, + { + base: "/tmp", + file: "/tmp/foo.txt", + relative: true, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + if test.relative != isRelative(test.base, test.file) { + t.Errorf("unexpected result for: base %q, file %q", test.base, test.file) + } + }) + } +} diff --git a/pkg/kudoctl/packages/package.go b/pkg/kudoctl/packages/package.go index f43563eda..a0d6653bd 100644 --- a/pkg/kudoctl/packages/package.go +++ b/pkg/kudoctl/packages/package.go @@ -99,18 +99,24 @@ func (p *Files) Resources() (*Resources, error) { } func validateTask(t v1beta1.Task, templates map[string]string) []string { + var errs []string var resources []string switch t.Kind { case task.ApplyTaskKind: resources = t.Spec.ResourceTaskSpec.Resources case task.DeleteTaskKind: resources = t.Spec.ResourceTaskSpec.Resources + case task.PipeTaskKind: + resources = append(resources, t.Spec.PipeTaskSpec.Container) + + if len(t.Spec.PipeTaskSpec.Pipe) == 0 { + errs = append(errs, fmt.Sprintf("task %s does not have pipe files specified", t.Name)) + } case task.DummyTaskKind: default: log.Printf("no validation for task kind %s implemented", t.Kind) } - var errs []string for _, res := range resources { if _, ok := templates[res]; !ok { errs = append(errs, fmt.Sprintf("task %s missing template: %s", t.Name, res)) From d58e063681151faf3640c1823cde8d82a8d07d30 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Tue, 26 Nov 2019 16:54:14 +0100 Subject: [PATCH 02/19] Added comments and TODOs --- pkg/engine/task/pod_exec.go | 15 +++++++++++---- pkg/engine/task/task_pipe.go | 16 +++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index 0d286fc24..c4bf6c3eb 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -33,7 +33,10 @@ type PodExec struct { Err io.Writer } -// Run execution the pod command. +// Run executes a command in a pod. This is a distilled version of what `kubectl exec` (and +// also `kubectl cp`) doing under the hood: a POST request is made to the `exec` subresource +// of the v1/pods endpoint containing the pod information and the command. Here is a good article +// describing it in detail: https://erkanerol.github.io/post/how-kubectl-exec-works/ func (pe *PodExec) Run() error { codec := serializer.NewCodecFactory(scheme.Scheme) restClient, err := apiutil.RESTClientForGVK( @@ -54,15 +57,14 @@ func (pe *PodExec) Run() error { Namespace(pe.PodNamespace). SubResource("exec") - poe := &v1.PodExecOptions{ + req.VersionedParams(&v1.PodExecOptions{ Stdin: pe.In != nil, Stdout: pe.Out != nil, Stderr: pe.Err != nil, TTY: false, Container: pe.ContainerName, Command: pe.Args, - } - req.VersionedParams(poe, scheme.ParameterCodec) + }, scheme.ParameterCodec) exec, err := remotecommand.NewSPDYExecutor(pe.RestCfg, "POST", req.URL()) if err != nil { @@ -76,7 +78,12 @@ func (pe *PodExec) Run() error { Tty: false, } + // Executor.Stream() call has to be made in a goroutine, otherwise it blocks the execution. + // We don't wait for the execution to end: the result of the command is returned though the + // streams (In, Out and Err) defined in the PodExec, e.g. when downloading a file, Out will + // contain the file bytes. go func(exec remotecommand.Executor, so remotecommand.StreamOptions) { + // TODO: we need to propagate this error to the caller err = exec.Stream(so) if err != nil { fmt.Printf("error during pod command %+v execution: %v", pe, err) diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index a6946412e..e6ea04515 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -133,7 +133,6 @@ func unmarshal(ctrStr string) (*corev1.Container, error) { return ctr, nil } -// this regexp is used by K87 for validation Secret/ConfigMap data keys. var pipeFileRe = regexp.MustCompile(`[-._a-zA-Z0-9]+`) func isRelative(base, file string) bool { @@ -162,9 +161,9 @@ func validate(ctr *corev1.Container, ff []PipeFile) error { } fileName := path.Base(f.File) - // k8s uses the same validation for Secret/ConfigMap data keys: a valid key must consist of alphanumeric - // characters, '-', '_' or '.' (e.g. 'key.name', or 'KEY_NAME', or 'key-name', regex used for - // validation is '[-._a-zA-Z0-9]+') + // Same as K87 we use file names as ConfigMap data keys. A valid key name for a ConfigMap must consist + // of alphanumeric characters, '-', '_' or '.' (e.g. 'key.name', or 'KEY_NAME', or 'key-name', regex + // used for validation is '[-._a-zA-Z0-9]+') if !pipeFileRe.MatchString(fileName) { return fmt.Errorf("pipe file name %s should only contain alphanumeric characters, '.', '_' and '-'", fileName) } @@ -259,7 +258,7 @@ func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) e return fmt.Errorf("failed to copy pipe file. err: %v", err) } - if err := untarFile(fs, "", reader, pf.File); err != nil { + if err := untarFile(fs, reader, pf.File); err != nil { return fmt.Errorf("failed to untar pipe file: %v", err) } @@ -352,9 +351,8 @@ func pipeConfigMap(pf PipeFile, data []byte, ctx Context) (string, error) { return string(b), nil } -// untar takes a destination path and a reader; a tar reader loops over the tarfile -// creating the file structure at 'path' along the way, and writing any files -func untarFile(fs afero.Fs, dest string, r io.Reader, file string) (err error) { +// untarFile extracts a tar file from the passed reader using passed file name. +func untarFile(fs afero.Fs, r io.Reader, fileName string) (err error) { tr := tar.NewReader(r) for { @@ -374,7 +372,7 @@ func untarFile(fs afero.Fs, dest string, r io.Reader, file string) (err error) { // the target location of the file. tar strips the leading "/" however, we treat the pipe file path // as a key to the underlying data (otherwise we'll have to start splitting paths). To avoid all // the complexity and because we only extract one file here, the path is taken from the PipeFile configuration - target := file + target := fileName // check the file type switch header.Typeflag { From 7b6d5f388164f48fbc4c07bbf48afa30ed1d3a35 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Wed, 27 Nov 2019 17:25:00 +0100 Subject: [PATCH 03/19] Fixed a deadlock when executing a pod command when downloading a file --- pkg/engine/task/pod_exec.go | 23 +++++------------ pkg/engine/task/task_pipe.go | 50 ++++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index c4bf6c3eb..d6b87a90d 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -1,7 +1,6 @@ package task import ( - "fmt" "io" v1 "k8s.io/api/core/v1" @@ -19,9 +18,9 @@ import ( // PodNamespace - Namespace of the pod // ContainerName - optional container to execute the command in. If empty, first container is taken // Args - The command (and args) to execute. -// In - An (optional) command input stream. -// Out - The command output stream set by `Run()`. -// Err - the command error stream set by `Run()`. +// In - Command input stream. +// Out - Command output stream +// Err - Command error stream type PodExec struct { RestCfg *rest.Config PodName string @@ -78,17 +77,7 @@ func (pe *PodExec) Run() error { Tty: false, } - // Executor.Stream() call has to be made in a goroutine, otherwise it blocks the execution. - // We don't wait for the execution to end: the result of the command is returned though the - // streams (In, Out and Err) defined in the PodExec, e.g. when downloading a file, Out will - // contain the file bytes. - go func(exec remotecommand.Executor, so remotecommand.StreamOptions) { - // TODO: we need to propagate this error to the caller - err = exec.Stream(so) - if err != nil { - fmt.Printf("error during pod command %+v execution: %v", pe, err) - } - }(exec, so) - - return nil + // The result of the executor.Stream() call itself is returned through the streams (In, Out and Err) + // defined in the PodExec, e.g. when downloading a file, pe.Out will contain the file bytes. + return exec.Stream(so) } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index e6ea04515..ca37ae30f 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -2,6 +2,7 @@ package task import ( "archive/tar" + "bytes" "errors" "fmt" "io" @@ -239,9 +240,7 @@ func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) error { reader, stdout := io.Pipe() - - defer reader.Close() - defer stdout.Close() + stderr := bytes.Buffer{} pe := PodExec{ RestCfg: restCfg, @@ -251,17 +250,37 @@ func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) e Args: []string{"tar", "cf", "-", pf.File}, In: nil, Out: stdout, - Err: nil, + Err: &stderr, } - if err := pe.Run(); err != nil { - return fmt.Errorf("failed to copy pipe file. err: %v", err) - } + // When downloading files using remotecommand.Executor, the call HAS to be wrapped into + // a goroutine, otherwise it blocks the execution. It boils down to calling remotecommand/v4.go:54 + // method, which will try copy stdout and stderr into provided buffers. We're using io.Pipe to move data + // from remote stdout to the local reader. It works synchronously as it copies data directly from the + // Write to the corresponding Read, there is no internal buffering. + // + // IMPORTANT: the PodExec.Run call doesn't return until we consume data from the pipe reader! It has + // to be executed in a goroutine and we have to consume the passed stdout first. + // Otherwise this code will deadlock. This is why consuming error from the errCh has to + // happen AFTER we try to extract the file! 🤯 + // + // See `kubectl cp` copyFromPod method for another example: + // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 + errCh := make(chan error, 0) + go func() { + defer stdout.Close() + errCh <- pe.Run() + }() if err := untarFile(fs, reader, pf.File); err != nil { return fmt.Errorf("failed to untar pipe file: %v", err) } + // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE READER. SEE THE COMMENT ABOVE. + if err := <-errCh; err != nil { + return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) + } + return nil } @@ -357,16 +376,11 @@ func untarFile(fs afero.Fs, r io.Reader, fileName string) (err error) { for { header, err := tr.Next() - - switch { - case err == io.EOF: // if no more files are found return - return nil - - case err != nil: // return any other error - return err - - case header == nil: // if the header is nil, just skip it - continue + if err != nil { + if err != io.EOF { + return err + } + break } // the target location of the file. tar strips the leading "/" however, we treat the pipe file path @@ -396,6 +410,8 @@ func untarFile(fs afero.Fs, r io.Reader, fileName string) (err error) { fmt.Printf("skipping %s because it is not a regular file or a directory", header.Name) } } + + return nil } // podName returns a deterministic name for a pipe pod From 8848320f4ac03aba84d3a95c9d253499ee1b95a7 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 28 Nov 2019 02:33:13 +0100 Subject: [PATCH 04/19] Added tests for validating pipe task and improved pipe artifact naming --- pkg/engine/task/task.go | 7 ++- pkg/engine/task/task_pipe.go | 69 ++++++++++++++----------- pkg/engine/task/task_test.go | 97 ++++++++++++++++++++++++++++++++++-- 3 files changed, 136 insertions(+), 37 deletions(-) diff --git a/pkg/engine/task/task.go b/pkg/engine/task/task.go index 0ade18975..d693c3f54 100644 --- a/pkg/engine/task/task.go +++ b/pkg/engine/task/task.go @@ -106,7 +106,7 @@ func newPipe(task *v1beta1.Task) (Tasker, error) { var pipeFiles []PipeFile for _, pp := range task.Spec.PipeTaskSpec.Pipe { - pf := PipeFile{File: pp.File, Kind: pp.Kind, Key: pp.Key} + pf := PipeFile{File: pp.File, Kind: PipeFileKind(pp.Kind), Key: pp.Key} // validate pipe file if err := validPipeFile(pf); err != nil { return nil, err @@ -122,15 +122,14 @@ func newPipe(task *v1beta1.Task) (Tasker, error) { } var ( - pipeFileKeyRe = regexp.MustCompile(`^[a-zA-Z0-9_\-]+$`) //a-z, A-Z, 0-9, _ and - are allowed - pipeFileKindRe = regexp.MustCompile("^Secret$|^ConfigMap$") // only Secret or ConfigMap are allowed + pipeFileKeyRe = regexp.MustCompile(`^[a-zA-Z0-9_\-]+$`) //a-z, A-Z, 0-9, _ and - are allowed ) func validPipeFile(pf PipeFile) error { if pf.File == "" { return fmt.Errorf("task validation error: pipe file is empty: %v", pf) } - if !pipeFileKindRe.MatchString(pf.Kind) { + if pf.Kind != PipeFileKindSecret && pf.Kind != PipeFileKindConfigMap { return fmt.Errorf("task validation error: invalid pipe kind (must be Secret or ConfigMap): %v", pf) } if !pipeFileKeyRe.MatchString(pf.Key) { diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index ca37ae30f..e1901f8bb 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -13,6 +13,7 @@ import ( "regexp" "strings" + "github.com/kudobuilder/kudo/pkg/engine/renderer" "github.com/spf13/afero" "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" @@ -28,6 +29,15 @@ var ( pipePodContainerName = "waiter" ) +type PipeFileKind string + +const ( + // PipeFile will be persisted as a Secret + PipeFileKindSecret PipeFileKind = "Secret" + // PipeFile will be persisted as a ConfigMap + PipeFileKindConfigMap PipeFileKind = "ConfigMap" +) + type PipeTask struct { Name string Container string @@ -36,7 +46,7 @@ type PipeTask struct { type PipeFile struct { File string - Kind string + Kind PipeFileKind Key string } @@ -60,7 +70,7 @@ func (pt PipeTask) Run(ctx Context) (bool, error) { } // 4. - Create a pod using the container - - podName := podName(ctx) + podName := PipePodName(ctx.Meta) podStr, err := pipePod(container, podName) if err != nil { return false, fatalExecutionError(err, pipeTaskError, ctx.Meta) @@ -98,7 +108,7 @@ func (pt PipeTask) Run(ctx Context) (bool, error) { // 9. - Create k8s artifacts (ConfigMap/Secret) from the pipe files - log.Printf("PipeTask: %s/%s creating pipe artifacts", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName) - artStr, err := pipeFiles(fs, pt.PipeFiles, ctx) + artStr, err := pipeFiles(fs, pt.PipeFiles, ctx.Meta) if err != nil { return false, err } @@ -286,7 +296,7 @@ func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) e // pipeFiles iterates through passed pipe files and their copied data, reads them, constructs k8s artifacts // and marshals them. -func pipeFiles(fs afero.Fs, files []PipeFile, ctx Context) (map[string]string, error) { +func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[string]string, error) { artifacts := map[string]string{} for _, pf := range files { @@ -297,15 +307,15 @@ func pipeFiles(fs afero.Fs, files []PipeFile, ctx Context) (map[string]string, e // API server has a limit of 1Mb for Secret/ConfigMap if len(data) > 1024*1024 { - return nil, fatalExecutionError(fmt.Errorf("pipe file %s size (%d bytes) exceeds max size limit of 1Mb", pf.File, len(data)), pipeTaskError, ctx.Meta) + return nil, fatalExecutionError(fmt.Errorf("pipe file %s size (%d bytes) exceeds max size limit of 1Mb", pf.File, len(data)), pipeTaskError, meta) } var art string switch pf.Kind { case "Secret": - art, err = pipeSecret(pf, data, ctx) + art, err = pipeSecret(pf, data, meta) case "ConfigMap": - art, err = pipeConfigMap(pf, data, ctx) + art, err = pipeConfigMap(pf, data, meta) default: return nil, fmt.Errorf("unknown pipe file kind: %+v", pf) } @@ -321,8 +331,8 @@ func pipeFiles(fs afero.Fs, files []PipeFile, ctx Context) (map[string]string, e // pipeSecret method creates a core/v1/Secret object using passed data. Pipe file name is used // as Secret data key. Secret name will be of the form ....- -func pipeSecret(pf PipeFile, data []byte, ctx Context) (string, error) { - name := artifactName(ctx, pf.Key) +func pipeSecret(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { + name := PipeArtifactName(meta, pf.Key) key := path.Base(pf.File) secret := corev1.Secret{ @@ -347,8 +357,8 @@ func pipeSecret(pf PipeFile, data []byte, ctx Context) (string, error) { // pipeConfigMap method creates a core/v1/ConfigMap object using passed data. Pipe file name is used // as ConfigMap data key. ConfigMap name will be of the form ....- -func pipeConfigMap(pf PipeFile, data []byte, ctx Context) (string, error) { - name := artifactName(ctx, pf.Key) +func pipeConfigMap(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { + name := PipeArtifactName(meta, pf.Key) key := path.Base(pf.File) configMap := corev1.ConfigMap{ @@ -414,28 +424,29 @@ func untarFile(fs afero.Fs, r io.Reader, fileName string) (err error) { return nil } -// podName returns a deterministic name for a pipe pod -func podName(ctx Context) string { return name(ctx, "pipe-pod") } +// PipePodName returns a deterministic name for a pipe pod +func PipePodName(meta renderer.Metadata) string { return name(meta, "pipepod") } -// artifactName returns a deterministic name for pipe artifact (ConfigMap, Secret) -func artifactName(ctx Context, key string) string { return name(ctx, key) } +// PipeArtifactName returns a deterministic name for pipe artifact (ConfigMap, Secret) +func PipeArtifactName(meta renderer.Metadata, key string) string { return name(meta, key) } var ( - nameRe = regexp.MustCompile(`^[^a-zA-Z0-9\-.]+`) - // TODO: this is the reqexp that API server is using for Secret/ConfigMap name validation - //artifactNameRe = regexp.MustCompile(`[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*`) + alnum = regexp.MustCompile(`[^a-z0-9]+`) ) // name returns a deterministic names for pipe artifacts (Pod, Secret, ConfigMap) in the form: -// ....- All characters aside from these defined in -// nameRe are removed and replaced with "" -func name(ctx Context, suffix string) string { - name := fmt.Sprintf("%s.%s.%s.%s.%s-%s", - ctx.Meta.InstanceName, - ctx.Meta.PlanName, - ctx.Meta.PhaseName, - ctx.Meta.StepName, - ctx.Meta.TaskName, - suffix) - return nameRe.ReplaceAllString(strings.ToLower(name), "") +// ....- All non alphanumeric characters are removed and +// replaced with "". +// A name for e.g a ConfigMap has to match a DNS-1123 subdomain, must consist of lower case alphanumeric +// characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', +// regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') +func name(meta renderer.Metadata, suffix string) string { + i := alnum.ReplaceAllString(strings.ToLower(meta.InstanceName), "") + pl := alnum.ReplaceAllString(strings.ToLower(meta.PlanName), "") + ph := alnum.ReplaceAllString(strings.ToLower(meta.PhaseName), "") + st := alnum.ReplaceAllString(strings.ToLower(meta.StepName), "") + ts := alnum.ReplaceAllString(strings.ToLower(meta.TaskName), "") + sf := alnum.ReplaceAllString(strings.ToLower(suffix), "") + + return fmt.Sprintf("%s.%s.%s.%s.%s-%s", i, pl, ph, st, ts, sf) } diff --git a/pkg/engine/task/task_test.go b/pkg/engine/task/task_test.go index ec63ae38c..9de5d0bb6 100644 --- a/pkg/engine/task/task_test.go +++ b/pkg/engine/task/task_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" - "github.com/stretchr/testify/assert" "sigs.k8s.io/yaml" ) @@ -59,6 +58,96 @@ spec: }, wantErr: false, }, + { + name: "pipe task with a pipe file kind Secret", + taskYaml: ` +name: pipe-task +kind: Pipe +spec: + container: container.yaml + pipe: + - file: /tmp/foo.txt + kind: Secret + key: Foo`, + want: PipeTask{ + Name: "pipe-task", + Container: "container.yaml", + PipeFiles: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: "Secret", + Key: "Foo", + }, + }, + }, + wantErr: false, + }, + { + name: "pipe task with a pipe file kind ConfigMap", + taskYaml: ` +name: pipe-task +kind: Pipe +spec: + container: container.yaml + pipe: + - file: /tmp/bar.txt + kind: ConfigMap + key: Bar`, + want: PipeTask{ + Name: "pipe-task", + Container: "container.yaml", + PipeFiles: []PipeFile{ + { + File: "/tmp/bar.txt", + Kind: "ConfigMap", + Key: "Bar", + }, + }, + }, + wantErr: false, + }, + { + name: "pipe task with an invalid pipe file kind", + taskYaml: ` +name: pipe-task +kind: Pipe +spec: + container: container.yaml + pipe: + - file: /tmp/bar.txt + kind: Invalid + key: Bar`, + want: nil, + wantErr: true, + }, + { + name: "pipe task file must be defined", + taskYaml: ` +name: pipe-task +kind: Pipe +spec: + container: container.yaml + pipe: + - file: + kind: Secret + key: Bar`, + want: nil, + wantErr: true, + }, + { + name: "pipe task keu is invalid", + taskYaml: ` +name: pipe-task +kind: Pipe +spec: + container: container.yaml + pipe: + - file: /tmp/bar.txt" + kind: Secret + key: $Bar^`, + want: nil, + wantErr: true, + }, { name: "unknown task", taskYaml: ` @@ -80,10 +169,10 @@ spec: } got, err := Build(task) - if tt.wantErr { - assert.Error(t, err, "Build(%s) error = %v, wantErr %v", tt.name, err, tt.wantErr) - return + if (err != nil) != tt.wantErr { + t.Errorf("Build(%s) should've failed but hasn't: error = %v, wantErr %v", tt.name, err, tt.wantErr) } + if !reflect.DeepEqual(got, tt.want) { t.Errorf("Build(%s) got = %v, want %v", tt.name, got, tt.want) } From 61dd83812dc3c1035f0771c8af97ed08c18c6e48 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 28 Nov 2019 02:59:06 +0100 Subject: [PATCH 05/19] Improved pipe artifact naming function --- pkg/engine/task/task_pipe.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index e1901f8bb..f6470c45b 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -435,18 +435,18 @@ var ( ) // name returns a deterministic names for pipe artifacts (Pod, Secret, ConfigMap) in the form: -// ....- All non alphanumeric characters are removed and -// replaced with "". +// ..... All non alphanumeric characters are removed. // A name for e.g a ConfigMap has to match a DNS-1123 subdomain, must consist of lower case alphanumeric // characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', // regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') func name(meta renderer.Metadata, suffix string) string { - i := alnum.ReplaceAllString(strings.ToLower(meta.InstanceName), "") - pl := alnum.ReplaceAllString(strings.ToLower(meta.PlanName), "") - ph := alnum.ReplaceAllString(strings.ToLower(meta.PhaseName), "") - st := alnum.ReplaceAllString(strings.ToLower(meta.StepName), "") - ts := alnum.ReplaceAllString(strings.ToLower(meta.TaskName), "") - sf := alnum.ReplaceAllString(strings.ToLower(suffix), "") - - return fmt.Sprintf("%s.%s.%s.%s.%s-%s", i, pl, ph, st, ts, sf) + sanitize := func(s string) string { + return alnum.ReplaceAllString(strings.ToLower(s), "") + } + + var parts []string + for _, s := range []string{meta.InstanceName, meta.PlanName, meta.PhaseName, meta.StepName, meta.TaskName, suffix} { + parts = append(parts, sanitize(s)) + } + return strings.Join(parts, ".") } From 3811c1ac55d6d64ca5990a89774a473252a57a52 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 28 Nov 2019 22:26:04 +0100 Subject: [PATCH 06/19] Instance controller now watches Pod events --- .../instance/instance_controller.go | 4 +- pkg/engine/task/task_pipe.go | 4 +- pkg/engine/task/task_pipe_test.go | 51 +++++++++++++++++++ 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index 7d9612f15..a6bf1c5f1 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -23,9 +23,8 @@ import ( "reflect" "time" - "github.com/kudobuilder/kudo/pkg/engine/renderer" - "github.com/kudobuilder/kudo/pkg/engine" + "github.com/kudobuilder/kudo/pkg/engine/renderer" "github.com/kudobuilder/kudo/pkg/engine/workflow" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/source" @@ -92,6 +91,7 @@ func (r *Reconciler) SetupWithManager( Owns(&corev1.Service{}). Owns(&batchv1.Job{}). Owns(&appsv1.StatefulSet{}). + Owns(&corev1.Pod{}). Watches(&source.Kind{Type: &kudov1beta1.OperatorVersion{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: addOvRelatedInstancesToReconcile}). Complete(r) } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index f6470c45b..86087aea5 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -312,9 +312,9 @@ func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[strin var art string switch pf.Kind { - case "Secret": + case PipeFileKindSecret: art, err = pipeSecret(pf, data, meta) - case "ConfigMap": + case PipeFileKindConfigMap: art, err = pipeConfigMap(pf, data, meta) default: return nil, fmt.Errorf("unknown pipe file kind: %+v", pf) diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go index 7087123e9..750e9ca2f 100644 --- a/pkg/engine/task/task_pipe_test.go +++ b/pkg/engine/task/task_pipe_test.go @@ -3,6 +3,9 @@ package task import ( "fmt" "testing" + + "github.com/kudobuilder/kudo/pkg/engine" + "github.com/kudobuilder/kudo/pkg/engine/renderer" ) func Test_isRelative(t *testing.T) { @@ -66,3 +69,51 @@ func Test_isRelative(t *testing.T) { }) } } + +func TestPipeNames(t *testing.T) { + tests := []struct { + name string + meta renderer.Metadata + key string + wantPodName string + wantArtifactName string + }{ + { + name: "simple", + meta: renderer.Metadata{ + Metadata: engine.Metadata{InstanceName: "foo-instance"}, + PlanName: "deploy", + PhaseName: "first", + StepName: "step", + TaskName: "genfiles", + }, + key: "foo", + wantPodName: "fooinstance.deploy.first.step.genfiles.pipepod", + wantArtifactName: "fooinstance.deploy.first.step.genfiles.foo", + }, + { + name: "with invalid characters", + meta: renderer.Metadata{ + Metadata: engine.Metadata{InstanceName: "Foo-Instance"}, + PlanName: "deploy", + PhaseName: "first", + StepName: "step", + TaskName: "gen_files", + }, + key: "$!Foo%", + wantPodName: "fooinstance.deploy.first.step.genfiles.pipepod", + wantArtifactName: "fooinstance.deploy.first.step.genfiles.foo", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := PipePodName(tt.meta); got != tt.wantPodName { + t.Errorf("PipePodName() = %v, want %v", got, tt.wantPodName) + } + + if got := PipeArtifactName(tt.meta, tt.key); got != tt.wantArtifactName { + t.Errorf("PipeArtifactName() = %v, want %v", got, tt.wantArtifactName) + } + }) + } +} From afb47ad5b614c8c70e59210eb3aa1c250799fe33 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Fri, 29 Nov 2019 18:02:32 +0100 Subject: [PATCH 07/19] Check pipe file size before downloading it --- .../instance/instance_controller.go | 2 +- pkg/engine/task/pod_exec.go | 152 +++++++++++++++++- pkg/engine/task/task_pipe.go | 120 +++----------- pkg/engine/workflow/engine.go | 2 +- 4 files changed, 171 insertions(+), 105 deletions(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index a6bf1c5f1..28018b64d 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -91,7 +91,7 @@ func (r *Reconciler) SetupWithManager( Owns(&corev1.Service{}). Owns(&batchv1.Job{}). Owns(&appsv1.StatefulSet{}). - Owns(&corev1.Pod{}). + Owns(&corev1.Pod{}). // TODO (ad): filter out Pod Delete events (possibly also all other k8s native objects?) #1116 Watches(&source.Kind{Type: &kudov1beta1.OperatorVersion{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: addOvRelatedInstancesToReconcile}). Complete(r) } diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index d6b87a90d..d40c01e28 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -1,8 +1,15 @@ package task import ( + "archive/tar" + "bytes" + "fmt" "io" + "io/ioutil" + "os" + "strconv" + "github.com/spf13/afero" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -30,6 +37,7 @@ type PodExec struct { In io.Reader Out io.Writer Err io.Writer + TTY bool } // Run executes a command in a pod. This is a distilled version of what `kubectl exec` (and @@ -60,7 +68,7 @@ func (pe *PodExec) Run() error { Stdin: pe.In != nil, Stdout: pe.Out != nil, Stderr: pe.Err != nil, - TTY: false, + TTY: pe.TTY, Container: pe.ContainerName, Command: pe.Args, }, scheme.ParameterCodec) @@ -74,10 +82,150 @@ func (pe *PodExec) Run() error { Stdin: pe.In, Stdout: pe.Out, Stderr: pe.Err, - Tty: false, + Tty: pe.TTY, } // The result of the executor.Stream() call itself is returned through the streams (In, Out and Err) // defined in the PodExec, e.g. when downloading a file, pe.Out will contain the file bytes. return exec.Stream(so) } + +// FileSize fetches the size of a file in a remote pod. It runs `stat -c %s file` command in the +// pod and parses the output. +func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { + readout, stdout := io.Pipe() + + pe := PodExec{ + RestCfg: restCfg, + PodName: pod.Name, + PodNamespace: pod.Namespace, + ContainerName: pipePodContainerName, + Args: []string{"stat", "-c", "%s", file}, + In: nil, + Out: stdout, + Err: nil, + TTY: true, // this will forward 2>&1. otherwise, reading from Out will never return for e.g. missing files + } + + errCh := make(chan error, 1) + go func() { + defer stdout.Close() + errCh <- pe.Run() + }() + + buf, err := ioutil.ReadAll(readout) + if err != nil { + return 0, fmt.Errorf("failed to get the size of %s `: %v", file, err) + } + + raw := string(buf) + + // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE STDOUT/STDERR READER. + if err := <-errCh; err != nil { + return 0, fmt.Errorf("failed to get the size of %s, err: %v, %s", file, err, raw) + } + + raw = raw[:len(raw)-2] // remove trailing \n\r + size, err := strconv.ParseInt(raw, 10, 64) + if err != nil { + return 0, fmt.Errorf("failed to parse the size '%s' of the file %s : %v", raw, file, err) + } + + return size, nil +} + +// DownloadFile fetches a file from a remote pod. It runs `tar cf - file` command and streams contents +// of the file via the stdout. Locally, the tar file is extracted into the passed afero filesystem where +// it is saved under the same path. Afero filesystem is used to allow the caller downloading and persisting +// of multiple files concurrently (afero filesystem is thread-safe). +func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) error { + readout, writeout := io.Pipe() + stderr := bytes.Buffer{} + + pe := PodExec{ + RestCfg: restCfg, + PodName: pod.Name, + PodNamespace: pod.Namespace, + ContainerName: pipePodContainerName, + Args: []string{"tar", "cf", "-", file}, + In: nil, + Out: writeout, + Err: &stderr, + } + + // When downloading files using remotecommand.Executor, we pump the contents of the tar file through + // the stdout. PodExec.Run() calls the stream executor, which first writes stdout and stderr of the + // command and then returns with an exit code. Since we're using io.Pipe reader and writer which is + // tread-safe but SYNCHRONOUS, Run() call will not return until we've consumed the streams (and will + // block the execution). (for more details on how the underlying streams are copied see remotecommand/v4.go:54 + // + // TL;DR: + // - execute PodExec.Run() in a goroutine + // - when using io.Pipe for In or Out streams, they have to be consumed first because io.Pipe is + // synchronous (and thread-safe) + // - there seems to be a bug when using BOTH Out AND Err pipe-based streams. When trying to consume + // both (in goroutines), one of them would end up blocking the execution ¯\_(ツ)_/¯ + // + // See `kubectl cp` copyFromPod method for another example: + // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 + errCh := make(chan error, 0) + go func() { + defer writeout.Close() + errCh <- pe.Run() + }() + + if err := untarFile(fs, readout, file); err != nil { + return fmt.Errorf("failed to untar pipe file: %v", err) + } + + // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE STDOUT/STDERR READER. + if err := <-errCh; err != nil { + return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) + } + + return nil +} + +// untarFile extracts a tar file from the passed reader using passed file name. +func untarFile(fs afero.Fs, r io.Reader, fileName string) error { + tr := tar.NewReader(r) + + for { + header, err := tr.Next() + if err != nil { + if err != io.EOF { + return err + } + break + } + + // the target location of the file. tar strips the leading "/" however, we treat the pipe file path + // as a key to the underlying data (otherwise we'll have to start splitting paths). To avoid all + // the complexity and because we only extract one file here, the path is taken from the PipeFile configuration + target := fileName + + // check the file type + switch header.Typeflag { + // if it's a file create it + case tar.TypeReg: + f, err := fs.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) + if err != nil { + return err + } + + // copy over contents + if _, err := io.Copy(f, tr); err != nil { + return err + } + + // manually close here after each file operation; deferring would cause each file close + // to wait until all operations have completed. + f.Close() // nolint + + default: + fmt.Printf("skipping %s because it is not a regular file or a directory", header.Name) + } + } + + return nil +} diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index 86087aea5..ff3f5c5bb 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -1,13 +1,9 @@ package task import ( - "archive/tar" - "bytes" "errors" "fmt" - "io" "log" - "os" "path" "path/filepath" "regexp" @@ -18,7 +14,6 @@ import ( "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client/config" "sigs.k8s.io/yaml" ) @@ -26,9 +21,15 @@ import ( var ( pipeTaskError = "PipeTaskError" + // name of the main pipe pod container pipePodContainerName = "waiter" ) +const ( + // Max file size of a pipe file. + maxPipeFileSize = 1024 * 1024 +) + type PipeFileKind string const ( @@ -240,7 +241,19 @@ func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { f := f log.Printf("PipeTask: %s/%s copying pipe file %s", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName, f.File) g.Go(func() error { - return copyFile(fs, f, pod, restCfg) + // Check the size of the pipe file first. K87 has a inherent limit on the size of + // Secret/ConfigMap, so we avoid unnecessary copying of files that are too big by + // checking its size first. + size, err := FileSize(f.File, pod, restCfg) + if err != nil { + return fatalExecutionError(err, pipeTaskError, ctx.Meta) + } + + if size > maxPipeFileSize { + return fatalExecutionError(fmt.Errorf("pipe file %s size %d exceeds maximum file size of %d bytes", f.File, size, maxPipeFileSize), pipeTaskError, ctx.Meta) + } + + return DownloadFile(fs, f.File, pod, restCfg) }) } @@ -248,52 +261,6 @@ func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { return err } -func copyFile(fs afero.Fs, pf PipeFile, pod *corev1.Pod, restCfg *rest.Config) error { - reader, stdout := io.Pipe() - stderr := bytes.Buffer{} - - pe := PodExec{ - RestCfg: restCfg, - PodName: pod.Name, - PodNamespace: pod.Namespace, - ContainerName: pipePodContainerName, - Args: []string{"tar", "cf", "-", pf.File}, - In: nil, - Out: stdout, - Err: &stderr, - } - - // When downloading files using remotecommand.Executor, the call HAS to be wrapped into - // a goroutine, otherwise it blocks the execution. It boils down to calling remotecommand/v4.go:54 - // method, which will try copy stdout and stderr into provided buffers. We're using io.Pipe to move data - // from remote stdout to the local reader. It works synchronously as it copies data directly from the - // Write to the corresponding Read, there is no internal buffering. - // - // IMPORTANT: the PodExec.Run call doesn't return until we consume data from the pipe reader! It has - // to be executed in a goroutine and we have to consume the passed stdout first. - // Otherwise this code will deadlock. This is why consuming error from the errCh has to - // happen AFTER we try to extract the file! 🤯 - // - // See `kubectl cp` copyFromPod method for another example: - // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 - errCh := make(chan error, 0) - go func() { - defer stdout.Close() - errCh <- pe.Run() - }() - - if err := untarFile(fs, reader, pf.File); err != nil { - return fmt.Errorf("failed to untar pipe file: %v", err) - } - - // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE READER. SEE THE COMMENT ABOVE. - if err := <-errCh; err != nil { - return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) - } - - return nil -} - // pipeFiles iterates through passed pipe files and their copied data, reads them, constructs k8s artifacts // and marshals them. func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[string]string, error) { @@ -305,11 +272,6 @@ func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[strin return nil, fmt.Errorf("error opening pipe file %s", pf.File) } - // API server has a limit of 1Mb for Secret/ConfigMap - if len(data) > 1024*1024 { - return nil, fatalExecutionError(fmt.Errorf("pipe file %s size (%d bytes) exceeds max size limit of 1Mb", pf.File, len(data)), pipeTaskError, meta) - } - var art string switch pf.Kind { case PipeFileKindSecret: @@ -380,50 +342,6 @@ func pipeConfigMap(pf PipeFile, data []byte, meta renderer.Metadata) (string, er return string(b), nil } -// untarFile extracts a tar file from the passed reader using passed file name. -func untarFile(fs afero.Fs, r io.Reader, fileName string) (err error) { - tr := tar.NewReader(r) - - for { - header, err := tr.Next() - if err != nil { - if err != io.EOF { - return err - } - break - } - - // the target location of the file. tar strips the leading "/" however, we treat the pipe file path - // as a key to the underlying data (otherwise we'll have to start splitting paths). To avoid all - // the complexity and because we only extract one file here, the path is taken from the PipeFile configuration - target := fileName - - // check the file type - switch header.Typeflag { - // if it's a file create it - case tar.TypeReg: - f, err := fs.OpenFile(target, os.O_CREATE|os.O_RDWR, os.FileMode(header.Mode)) - if err != nil { - return err - } - - // copy over contents - if _, err := io.Copy(f, tr); err != nil { - return err - } - - // manually close here after each file operation; deferring would cause each file close - // to wait until all operations have completed. - f.Close() // nolint - - default: - fmt.Printf("skipping %s because it is not a regular file or a directory", header.Name) - } - } - - return nil -} - // PipePodName returns a deterministic name for a pipe pod func PipePodName(meta renderer.Metadata) string { return name(meta, "pipepod") } diff --git a/pkg/engine/workflow/engine.go b/pkg/engine/workflow/engine.go index 6dd589205..2a867e627 100644 --- a/pkg/engine/workflow/engine.go +++ b/pkg/engine/workflow/engine.go @@ -183,7 +183,7 @@ func Execute(pl *ActivePlan, em *engine.Metadata, c client.Client, enh renderer. case err != nil: message := fmt.Sprintf("A transient error when executing task %s.%s.%s.%s. Will retry. %v", pl.Name, ph.Name, st.Name, t.Name, err) stepStatus.SetWithMessage(v1beta1.ErrorStatus, message) - fmt.Printf("PlanExecution: %s", message) + log.Printf("PlanExecution: %s", message) case done: delete(tasksLeft, t.Name) } From a921f6c18851f623ed680a9f74d27b7daf60e328 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Sat, 30 Nov 2019 01:46:16 +0100 Subject: [PATCH 08/19] Ran go mod tidy --- go.mod | 1 - pkg/engine/task/pod_exec.go | 13 ++++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 777d08876..4a960f837 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,6 @@ require ( github.com/Masterminds/sprig v2.22.0+incompatible github.com/Microsoft/go-winio v0.4.14 // indirect github.com/containerd/containerd v1.2.9 // indirect - github.com/davecgh/go-spew v1.1.1 github.com/docker/docker v1.4.2-0.20190916154449-92cc603036dd github.com/docker/go-connections v0.4.0 // indirect github.com/docker/go-units v0.4.0 // indirect diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index d40c01e28..6449ef170 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -109,7 +109,7 @@ func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { errCh := make(chan error, 1) go func() { - defer stdout.Close() + defer stdout.Close() // Close never returns an error errCh <- pe.Run() }() @@ -160,17 +160,16 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e // block the execution). (for more details on how the underlying streams are copied see remotecommand/v4.go:54 // // TL;DR: - // - execute PodExec.Run() in a goroutine - // - when using io.Pipe for In or Out streams, they have to be consumed first because io.Pipe is - // synchronous (and thread-safe) - // - there seems to be a bug when using BOTH Out AND Err pipe-based streams. When trying to consume - // both (in goroutines), one of them would end up blocking the execution ¯\_(ツ)_/¯ + // - execute PodExec.Run() in a goroutine when using io.Pipe for Out or Err streams, they + // have to be consumed first because io.Pipe is synchronous (and thread-safe) + // - there seems to be a bug when using BOTH Out AND Err pipe-based streams. When trying to + // consume both (in goroutines), one of them ends up blocking the execution ¯\_(ツ)_/¯ // // See `kubectl cp` copyFromPod method for another example: // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 errCh := make(chan error, 0) go func() { - defer writeout.Close() + defer writeout.Close() // Close never returns an error errCh <- pe.Run() }() From fe78dd737b52b95860181503626cddd89384c456 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Mon, 2 Dec 2019 14:02:35 +0100 Subject: [PATCH 09/19] Added `{{ .Pipes. }} ` config object to template rendering --- .../instance/instance_controller.go | 53 ++++++- .../instance_controller_suite_test.go | 2 +- .../instance/instance_controller_test.go | 144 ++++++++++++++++++ pkg/engine/task/pod_exec.go | 5 +- pkg/engine/task/render.go | 21 +-- pkg/engine/task/task.go | 1 + pkg/engine/task/task_apply.go | 14 +- pkg/engine/task/task_delete.go | 2 +- pkg/engine/task/task_pipe.go | 2 +- pkg/engine/workflow/engine.go | 2 + 10 files changed, 215 insertions(+), 31 deletions(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index 28018b64d..ac2d3b943 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -25,6 +25,7 @@ import ( "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/engine/renderer" + "github.com/kudobuilder/kudo/pkg/engine/task" "github.com/kudobuilder/kudo/pkg/engine/workflow" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/source" @@ -36,6 +37,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" @@ -174,7 +176,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) { InstanceName: instance.Name, } - activePlan, err := preparePlanExecution(instance, ov, activePlanStatus) + activePlan, err := preparePlanExecution(instance, ov, activePlanStatus, metadata) if err != nil { err = r.handleError(err, instance, oldInstance) return reconcile.Result{}, err @@ -225,14 +227,15 @@ func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Ins return nil } -func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion, activePlanStatus *kudov1beta1.PlanStatus) (*workflow.ActivePlan, error) { - params := getParameters(instance, ov) - +func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion, activePlanStatus *kudov1beta1.PlanStatus, meta *engine.Metadata) (*workflow.ActivePlan, error) { planSpec, ok := ov.Spec.Plans[activePlanStatus.Name] if !ok { return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not find required plan: %v", engine.ErrFatalExecution, activePlanStatus.Name), EventName: "InvalidPlan"} } + params := makeParams(instance, ov) + pipes := makePipes(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta) + return &workflow.ActivePlan{ Name: activePlanStatus.Name, Spec: &planSpec, @@ -240,6 +243,7 @@ func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.Operat Tasks: ov.Spec.Tasks, Templates: ov.Spec.Templates, Params: params, + Pipes: pipes, }, nil } @@ -312,7 +316,8 @@ func (r *Reconciler) getOperatorVersion(instance *kudov1beta1.Instance) (ov *kud return ov, nil } -func getParameters(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string { +// makeParams generates {{ Params.* }} map of keys and values which is later used during template rendering. +func makeParams(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string { params := make(map[string]string) for k, v := range instance.Spec.Parameters { @@ -329,7 +334,43 @@ func getParameters(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1. return params } -func parameterDifference(old, new map[string]string) map[string]string { +// makePipes generates {{ Pipes.* }} map of keys and values which is later used during template rendering. +func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta *engine.Metadata) map[string]string { + taskByName := func(name string) (*v1beta1.Task, bool) { + for _, t := range tasks { + if t.Name == name { + return &t, true + } + } + return nil, false + } + + pipes := make(map[string]string) + + for _, ph := range plan.Phases { + for _, st := range ph.Steps { + for _, tn := range st.Tasks { + rmeta := renderer.Metadata{ + Metadata: *emeta, + PlanName: planName, + PhaseName: ph.Name, + StepName: st.Name, + TaskName: tn, + } + + if t, ok := taskByName(tn); ok && t.Kind == task.PipeTaskKind { + for _, pipe := range t.Spec.PipeTaskSpec.Pipe { + pipes[pipe.Key] = task.PipeArtifactName(rmeta, pipe.Key) + } + } + } + } + } + + return pipes +} + +func parameterDiff(old, new map[string]string) map[string]string { diff := make(map[string]string) for key, val := range old { diff --git a/pkg/controller/instance/instance_controller_suite_test.go b/pkg/controller/instance/instance_controller_suite_test.go index c3408ceae..cc041a44d 100644 --- a/pkg/controller/instance/instance_controller_suite_test.go +++ b/pkg/controller/instance/instance_controller_suite_test.go @@ -70,7 +70,7 @@ func TestSpecParameterDifference(t *testing.T) { var old = map[string]string{"one": "1", "two": "2"} for _, test := range testParams { - diff := parameterDifference(old, test.new) + diff := parameterDiff(old, test.new) g.Expect(diff).Should(gomega.Equal(test.diff), test.name) } } diff --git a/pkg/controller/instance/instance_controller_test.go b/pkg/controller/instance/instance_controller_test.go index d5713e0fc..99f6f015d 100644 --- a/pkg/controller/instance/instance_controller_test.go +++ b/pkg/controller/instance/instance_controller_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" + "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/util/kudo" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" @@ -108,3 +109,146 @@ func startTestManager(t *testing.T) (chan struct{}, *sync.WaitGroup, client.Clie }() return stop, wg, mgr.GetClient() } + +func Test_makePipes(t *testing.T) { + meta := &engine.Metadata{ + InstanceName: "first-operator-instance", + InstanceNamespace: "default", + OperatorName: "first-operator", + OperatorVersionName: "first-operator-1.0", + OperatorVersion: "1.0", + } + + tests := []struct { + name string + planName string + plan *v1beta1.Plan + tasks []v1beta1.Task + emeta *engine.Metadata + want map[string]string + }{ + { + name: "no tasks, no pipes", + planName: "deploy", + plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{ + { + Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{ + { + Name: "step", Tasks: []string{}}, + }}, + }}, + tasks: []v1beta1.Task{}, + emeta: meta, + want: map[string]string{}, + }, + { + name: "no pipe tasks, no pipes", + planName: "deploy", + plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{ + { + Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{ + { + Name: "step", Tasks: []string{"task"}}, + }}, + }}, + tasks: []v1beta1.Task{ + { + Name: "task", + Kind: "Dummy", + Spec: v1beta1.TaskSpec{ + DummyTaskSpec: v1beta1.DummyTaskSpec{Done: false}, + }, + }, + }, + emeta: meta, + want: map[string]string{}, + }, + { + name: "one pipe task, one pipes element", + planName: "deploy", + plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{ + { + Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{ + { + Name: "step", Tasks: []string{"task"}}, + }}, + }}, + tasks: []v1beta1.Task{ + { + Name: "task", + Kind: "Pipe", + Spec: v1beta1.TaskSpec{ + PipeTaskSpec: v1beta1.PipeTaskSpec{ + Container: "container.yaml", + Pipe: []v1beta1.PipeSpec{ + { + File: "foo.txt", + Kind: "Secret", + Key: "Foo", + }, + }, + }, + }, + }, + }, + emeta: meta, + want: map[string]string{"Foo": "firstoperatorinstance.deploy.phase.step.task.foo"}, + }, + { + name: "two pipe tasks, two pipes element", + planName: "deploy", + plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{ + { + Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{ + {Name: "stepOne", Tasks: []string{"task-one"}}, + {Name: "stepTwo", Tasks: []string{"task-two"}}, + }}, + }}, + tasks: []v1beta1.Task{ + { + Name: "task-one", + Kind: "Pipe", + Spec: v1beta1.TaskSpec{ + PipeTaskSpec: v1beta1.PipeTaskSpec{ + Container: "container.yaml", + Pipe: []v1beta1.PipeSpec{ + { + File: "foo.txt", + Kind: "Secret", + Key: "Foo", + }, + }, + }, + }, + }, + { + Name: "task-two", + Kind: "Pipe", + Spec: v1beta1.TaskSpec{ + PipeTaskSpec: v1beta1.PipeTaskSpec{ + Container: "container.yaml", + Pipe: []v1beta1.PipeSpec{ + { + File: "bar.txt", + Kind: "ConfigMap", + Key: "Bar", + }, + }, + }, + }, + }, + }, + emeta: meta, + want: map[string]string{ + "Foo": "firstoperatorinstance.deploy.phase.stepone.taskone.foo", + "Bar": "firstoperatorinstance.deploy.phase.steptwo.tasktwo.bar", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := makePipes(tt.planName, tt.plan, tt.tasks, tt.emeta) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index 6449ef170..2d2f42a0a 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -153,11 +153,12 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e Err: &stderr, } + // IMPORTANT: // When downloading files using remotecommand.Executor, we pump the contents of the tar file through // the stdout. PodExec.Run() calls the stream executor, which first writes stdout and stderr of the // command and then returns with an exit code. Since we're using io.Pipe reader and writer which is // tread-safe but SYNCHRONOUS, Run() call will not return until we've consumed the streams (and will - // block the execution). (for more details on how the underlying streams are copied see remotecommand/v4.go:54 + // block the execution). For more details on how the underlying streams are copied see remotecommand/v4.go:54 // // TL;DR: // - execute PodExec.Run() in a goroutine when using io.Pipe for Out or Err streams, they @@ -167,7 +168,7 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e // // See `kubectl cp` copyFromPod method for another example: // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 - errCh := make(chan error, 0) + errCh := make(chan error, 1) go func() { defer writeout.Close() // Close never returns an error errCh <- pe.Run() diff --git a/pkg/engine/task/render.go b/pkg/engine/task/render.go index 03ffc3118..76f67f9b9 100644 --- a/pkg/engine/task/render.go +++ b/pkg/engine/task/render.go @@ -8,22 +8,23 @@ import ( ) // render method takes resource names and Instance parameters and then renders passed templates using kudo engine. -func render(resourceNames []string, templates map[string]string, params map[string]string, meta renderer.Metadata) (map[string]string, error) { +func render(resourceNames []string, ctx Context) (map[string]string, error) { configs := make(map[string]interface{}) - configs["OperatorName"] = meta.OperatorName - configs["Name"] = meta.InstanceName - configs["Namespace"] = meta.InstanceNamespace - configs["Params"] = params - configs["PlanName"] = meta.PlanName - configs["PhaseName"] = meta.PhaseName - configs["StepName"] = meta.StepName - configs["AppVersion"] = meta.AppVersion + configs["OperatorName"] = ctx.Meta.OperatorName + configs["Name"] = ctx.Meta.InstanceName + configs["Namespace"] = ctx.Meta.InstanceNamespace + configs["Params"] = ctx.Parameters + configs["Pipes"] = ctx.Pipes + configs["PlanName"] = ctx.Meta.PlanName + configs["PhaseName"] = ctx.Meta.PhaseName + configs["StepName"] = ctx.Meta.StepName + configs["AppVersion"] = ctx.Meta.AppVersion resources := map[string]string{} engine := renderer.New() for _, rn := range resourceNames { - resource, ok := templates[rn] + resource, ok := ctx.Templates[rn] if !ok { return nil, fmt.Errorf("error finding resource named %s", rn) diff --git a/pkg/engine/task/task.go b/pkg/engine/task/task.go index d693c3f54..1678135a9 100644 --- a/pkg/engine/task/task.go +++ b/pkg/engine/task/task.go @@ -19,6 +19,7 @@ type Context struct { Meta renderer.Metadata Templates map[string]string // Raw templates Parameters map[string]string // Instance and OperatorVersion parameters merged + Pipes map[string]string // Pipe artifacts } // Tasker is an interface that represents any runnable task for an operator. This method is treated diff --git a/pkg/engine/task/task_apply.go b/pkg/engine/task/task_apply.go index 47b8f4af3..97563ee8c 100644 --- a/pkg/engine/task/task_apply.go +++ b/pkg/engine/task/task_apply.go @@ -2,7 +2,6 @@ package task import ( "context" - "encoding/json" "fmt" "log" @@ -27,7 +26,7 @@ type ApplyTask struct { // resources are checked for health. func (at ApplyTask) Run(ctx Context) (bool, error) { // 1. - Render task templates - - rendered, err := render(at.Resources, ctx.Templates, ctx.Parameters, ctx.Meta) + rendered, err := render(at.Resources, ctx) if err != nil { return false, fatalExecutionError(err, taskRenderingError, ctx.Meta) } @@ -102,12 +101,12 @@ func patch(newObj runtime.Object, existingObj runtime.Object, c client.Client) e // strategic merge patch is not supported for these types, falling back to merge patch err := c.Patch(context.TODO(), newObj, client.ConstantPatch(types.MergePatchType, newObjJSON)) if err != nil { - return fmt.Errorf("failed to apply merge patch to object %s: %w", prettyPrint(key), err) + return fmt.Errorf("failed to apply merge patch to object %s/%s: %w", key.Name, key.Name, err) } } else { err := c.Patch(context.TODO(), existingObj, client.ConstantPatch(types.StrategicMergePatchType, newObjJSON)) if err != nil { - return fmt.Errorf("failed to apply StrategicMergePatch to object %s: %w", prettyPrint(key), err) + return fmt.Errorf("failed to apply StrategicMergePatch to object %s/%s: %w", key.Namespace, key.Name, err) } } return nil @@ -125,13 +124,8 @@ func isHealthy(ro []runtime.Object) error { err := health.IsHealthy(r) if err != nil { key, _ := client.ObjectKeyFromObject(r) - return fmt.Errorf("object %s is NOT healthy: %w", prettyPrint(key), err) + return fmt.Errorf("object %s/%s is NOT healthy: %w", key.Namespace, key.Name, err) } } return nil } - -func prettyPrint(key client.ObjectKey) string { - s, _ := json.MarshalIndent(key, "", " ") - return string(s) -} diff --git a/pkg/engine/task/task_delete.go b/pkg/engine/task/task_delete.go index 949146ef2..279467596 100644 --- a/pkg/engine/task/task_delete.go +++ b/pkg/engine/task/task_delete.go @@ -18,7 +18,7 @@ type DeleteTask struct { // creates runtime objects and kustomizes them, and finally removes them using the controller client. func (dt DeleteTask) Run(ctx Context) (bool, error) { // 1. - Render task templates - - rendered, err := render(dt.Resources, ctx.Templates, ctx.Parameters, ctx.Meta) + rendered, err := render(dt.Resources, ctx) if err != nil { return false, fatalExecutionError(err, taskRenderingError, ctx.Meta) } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index ff3f5c5bb..8599c5f9a 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -53,7 +53,7 @@ type PipeFile struct { func (pt PipeTask) Run(ctx Context) (bool, error) { // 1. - Render container template - - rendered, err := render([]string{pt.Container}, ctx.Templates, ctx.Parameters, ctx.Meta) + rendered, err := render([]string{pt.Container}, ctx) if err != nil { return false, fatalExecutionError(err, taskRenderingError, ctx.Meta) } diff --git a/pkg/engine/workflow/engine.go b/pkg/engine/workflow/engine.go index 2a867e627..97251566e 100644 --- a/pkg/engine/workflow/engine.go +++ b/pkg/engine/workflow/engine.go @@ -31,6 +31,7 @@ type ActivePlan struct { Tasks []v1beta1.Task Templates map[string]string Params map[string]string + Pipes map[string]string } func (ap *ActivePlan) taskByName(name string) (*v1beta1.Task, bool) { @@ -167,6 +168,7 @@ func Execute(pl *ActivePlan, em *engine.Metadata, c client.Client, enh renderer. Meta: exm, Templates: pl.Templates, Parameters: pl.Params, + Pipes: pl.Pipes, } // --- 4. Execute the engine task --- From d8589a954253480958d86fd3c8b08cfc11e31c2b Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Mon, 2 Dec 2019 16:23:08 +0100 Subject: [PATCH 10/19] Fix test/integration/upgrade-command --- pkg/test/utils/subset.go | 2 +- .../upgrade-command/first-operator/operator.yaml | 2 +- .../first-operator/templates/configmap.yaml | 6 ++++++ .../upgrade-command/first-operator/templates/pod.yaml | 9 --------- .../upgrade-command/second-operator/operator.yaml | 2 +- .../second-operator/templates/configmap.yaml | 6 ++++++ .../upgrade-command/second-operator/templates/pod.yaml | 9 --------- 7 files changed, 15 insertions(+), 21 deletions(-) create mode 100644 test/integration/upgrade-command/first-operator/templates/configmap.yaml delete mode 100644 test/integration/upgrade-command/first-operator/templates/pod.yaml create mode 100644 test/integration/upgrade-command/second-operator/templates/configmap.yaml delete mode 100644 test/integration/upgrade-command/second-operator/templates/pod.yaml diff --git a/pkg/test/utils/subset.go b/pkg/test/utils/subset.go index 7f9b0aac4..109ad3892 100644 --- a/pkg/test/utils/subset.go +++ b/pkg/test/utils/subset.go @@ -80,7 +80,7 @@ func IsSubset(expected, actual interface{}) error { } } else { return &SubsetError{ - message: fmt.Sprintf("value mismatch: %v != %v", expected, actual), + message: fmt.Sprintf("value mismatch, expected: %v != actual: %v", expected, actual), } } diff --git a/test/integration/upgrade-command/first-operator/operator.yaml b/test/integration/upgrade-command/first-operator/operator.yaml index 1d8eee55a..58ad07ec1 100644 --- a/test/integration/upgrade-command/first-operator/operator.yaml +++ b/test/integration/upgrade-command/first-operator/operator.yaml @@ -11,7 +11,7 @@ tasks: kind: Apply spec: resources: - - pod.yaml + - configmap.yaml plans: deploy: strategy: serial diff --git a/test/integration/upgrade-command/first-operator/templates/configmap.yaml b/test/integration/upgrade-command/first-operator/templates/configmap.yaml new file mode 100644 index 000000000..563ece391 --- /dev/null +++ b/test/integration/upgrade-command/first-operator/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test6 +data: + foo: bar-{{ .Params.version }} \ No newline at end of file diff --git a/test/integration/upgrade-command/first-operator/templates/pod.yaml b/test/integration/upgrade-command/first-operator/templates/pod.yaml deleted file mode 100644 index 52d7dcb44..000000000 --- a/test/integration/upgrade-command/first-operator/templates/pod.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: v1 -kind: Pod -metadata: - name: test6 -spec: - restartPolicy: Never - containers: - - name: nginx - image: nginx:{{ .Params.version }} \ No newline at end of file diff --git a/test/integration/upgrade-command/second-operator/operator.yaml b/test/integration/upgrade-command/second-operator/operator.yaml index cd1f4287c..5bf82d1b1 100644 --- a/test/integration/upgrade-command/second-operator/operator.yaml +++ b/test/integration/upgrade-command/second-operator/operator.yaml @@ -11,7 +11,7 @@ tasks: kind: Apply spec: resources: - - pod.yaml + - configmap.yaml plans: deploy: strategy: serial diff --git a/test/integration/upgrade-command/second-operator/templates/configmap.yaml b/test/integration/upgrade-command/second-operator/templates/configmap.yaml new file mode 100644 index 000000000..563ece391 --- /dev/null +++ b/test/integration/upgrade-command/second-operator/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test6 +data: + foo: bar-{{ .Params.version }} \ No newline at end of file diff --git a/test/integration/upgrade-command/second-operator/templates/pod.yaml b/test/integration/upgrade-command/second-operator/templates/pod.yaml deleted file mode 100644 index 52d7dcb44..000000000 --- a/test/integration/upgrade-command/second-operator/templates/pod.yaml +++ /dev/null @@ -1,9 +0,0 @@ -apiVersion: v1 -kind: Pod -metadata: - name: test6 -spec: - restartPolicy: Never - containers: - - name: nginx - image: nginx:{{ .Params.version }} \ No newline at end of file From f8e76c9f69fa457619cf2045de71a50025979986 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Tue, 3 Dec 2019 16:58:04 +0100 Subject: [PATCH 11/19] Updated the pipe specification to take a pod instead of a container additionally, instance controller doesn't not reconcile on pipe-pod deletion events --- keps/0017-pipe-tasks.md | 50 +++-- .../kudo/v1beta1/operatorversion_types.go | 4 +- .../instance/instance_controller.go | 27 ++- .../instance/instance_controller_test.go | 6 +- pkg/engine/task/task.go | 2 +- pkg/engine/task/task_pipe.go | 141 +++++++----- pkg/engine/task/task_pipe_test.go | 205 ++++++++++++++++++ pkg/engine/task/task_test.go | 20 +- pkg/kudoctl/packages/package.go | 2 +- 9 files changed, 362 insertions(+), 95 deletions(-) diff --git a/keps/0017-pipe-tasks.md b/keps/0017-pipe-tasks.md index 27492be30..1595738e0 100644 --- a/keps/0017-pipe-tasks.md +++ b/keps/0017-pipe-tasks.md @@ -49,14 +49,14 @@ tasks: - name: gencert kind: Pipe spec: - container: cert-container.yaml + pod: cert-pod.yaml pipe: - file: /usr/share/MyKey.key kind: Secret # ConfigMap key: Mycertificate ``` -`container` field is described in detail below. `key` will be used by the resources referencing generated file as following: `{{.Pipes.Mycertificate}}`. In the above example we would create a secret named `instance-myapp.deploy.bootstrap.gencert.mycertificate` to capture instance name along with plan/phase/step/task of the secret origin. The secret name would be stable so that a user can rerun the certificate generating task and reload all pods using it. Pipe file name will be used as Secret/ConfigMap data key. We would also use labels ensuring that the secret is cleaned up when the corresponding Instance is removed. `pipe` field is an array and can define how multiple generated files are stored and referenced. +`pod` field is described in detail below. `key` will be used by the resources referencing generated file as following: `{{.Pipes.Mycertificate}}`. In the above example we would create a secret named `instancemyapp.deploy.bootstrap.gencert.mycertificate` to capture instance name along with plan/phase/step/task of the secret origin. The secret name would be stable so that a user can rerun the certificate generating task and reload all pods using it. Pipe file name will be used as Secret/ConfigMap data key. We would also use labels ensuring that the secret is cleaned up when the corresponding Instance is removed. `pipe` field is an array and can define how multiple generated files are stored and referenced. The corresponding `gencert` task can be used as usual in e.g.: ```yaml @@ -74,23 +74,27 @@ plans: Note that piped files has to be generated before they can be used. In the example above, `bootstrap` phase has a strategy `serial` so that certificate generated in the `gencert` step can be used in subsequent steps. Or stated differently resources can not reference piped secrets/configmaps generated within the same step or within a parallel series of steps (it has to be a different step in the phase with serial strategy or a different phase). -For the pipe task `container`, we allow a [core/v1 container](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#container-v1-core) resource to be specified. Reasons for that are explained below in the implementation details. The container has to define a volumeMount where the files are stored. +For the pipe task `pod`, we allow a [core/v1 Pod](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#pod-v1-core) to be specified, however, there are limitations. Reasons for that are explained below in the implementation details. In a nutshell: +- a pipe pod should generate artifacts in the init container +- it has to define and mount an emptyDir volume (where generated files are stored) ```yaml -volumes: -- name: shared-data - emptyDir: {} +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: shared-data + emptyDir: {} -containers: - - name: gencert - image: frapsoft/openssl - imagePullPolicy: Always - command: [ "sh", "-c" ] - args: - - openssl req -new -newkey rsa:4096 -x509 -sha256 -days 365 -nodes -out MyCertificate.crt -keyout /usr/share/MyKey.key - volumeMounts: - - name: shared-data - mountPath: /usr/share/ + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - openssl req -new -newkey rsa:4096 -x509 -sha256 -days 365 -nodes -out MyCertificate.crt -keyout /usr/share/MyKey.key + volumeMounts: + - name: shared-data + mountPath: /tmp ``` Any subsequent step resource (if the phase strategy is `serial`) might reference previously generated file by its key e.g.: @@ -110,17 +114,15 @@ spec: ``` ### Limitations -- File generating container has to be side-effect free (meaning side-effects that are observable outside of the container like a 3rd party API call) as the container might be executed multiple times on failure. If that's not the case, `restartPolicy: OnFailure` has to be set to ensure the pod would be restarted if there is an issue with the initContainer. +- File generating Pod has to be side-effect free (meaning side-effects that are observable outside of the container like a 3rd party API call) as the container might be executed multiple times on failure. A `restartPolicy: OnFailure` is used for the pipe pod. - Only files <1Mb are applicable to be stored as ConfigMap or Secret. -- Only one resource per pipe task is allowed. If needed multiple tasks must be used. ### Implementation Details/Notes/Constraints - There are several ways to implement pipe tasks, each one having its challenges and complexities. The approach below allows us not to worry about Pod container life-cycle as well as keep the storing logic in the KUDO controller: -- Provided container is injected into a Pod as an [InitContainer](https://kubernetes.io/docs/concepts/workloads/pods/init-containers/). This is the simplest way to wait for container completion. This is also the reason why pipe task resource definition is a container and not a complete Pod specification. Pod init container can not have Lifecycle actions, Readiness probes, or Liveness probes fields defined. -- The main container is a `busybox` image, running the `sleep infinity` command, which purpose is to wait for KUDO to extract and store the files. -- Pod status `READY: 1/1, STATUS: Running` means that the init container has run successfully. As this point KUDO can copy out referenced files using `kubectl cp` and store them as specified. -- Once files are stored, KUDO can delete the Pod and proceed to the next task. +- Provided Pod is enriched with a main container, which uses a `busybox` image, running the `sleep infinity` command, which purpose is to wait for KUDO to extract and store the files. +- Generating files in the initContainer is the simplest way to wait for container completion. A pipe pod status: `READY: 1/1, STATUS: Running` means that the init container has run successfully. As this point KUDO can copy out referenced files using `kubectl cp` and store them as specified. +- Pod init container can not have Lifecycle actions, Readiness probes, or Liveness probes fields defined which simplifies implementation significantly +- Once files are stored, KUDO can delete the pipe pod and proceed to the next task. Here is a minimal example demonstrating the proposed implementation: ```yaml @@ -164,7 +166,7 @@ foo-bar-bazz ## Alternatives -An alternative approach would use the provided `container` as the main container (or let the user provide a complete Pod spec). We would inject a sidecar with a go executable which would: +An alternative approach would allow user to specify the main container (`containers` field) or let the user provide a complete Pod spec. We would inject a sidecar with a go executable which have to: - Use controller runtime, watch its own Pod status and wait for termination of the main container - Once the main container exits, it would copy the referenced files and store them as specified - We would use `restartPolicy: Never/OnFailure` to prevent the main container from restarting diff --git a/pkg/apis/kudo/v1beta1/operatorversion_types.go b/pkg/apis/kudo/v1beta1/operatorversion_types.go index 2bd51ae0a..771c8c92f 100644 --- a/pkg/apis/kudo/v1beta1/operatorversion_types.go +++ b/pkg/apis/kudo/v1beta1/operatorversion_types.go @@ -138,8 +138,8 @@ type DummyTaskSpec struct { // PipeTask specifies a task that generates files and stores them for later usage in subsequent tasks type PipeTaskSpec struct { - Container string `json:"container"` - Pipe []PipeSpec `json:"pipe"` + Pod string `json:"pod"` + Pipe []PipeSpec `json:"pipe"` } // PipeSpec describes how a file generated by a PipeTask is stored and referenced diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index ac2d3b943..b3150b0d4 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -27,7 +27,9 @@ import ( "github.com/kudobuilder/kudo/pkg/engine/renderer" "github.com/kudobuilder/kudo/pkg/engine/task" "github.com/kudobuilder/kudo/pkg/engine/workflow" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" "k8s.io/apimachinery/pkg/runtime" @@ -86,6 +88,26 @@ func (r *Reconciler) SetupWithManager( return requests }) + hasAnnotation := func(key string, annotations map[string]string) bool { + if annotations == nil { + return false + } + for k, _ := range annotations { + if k == key { + return true + } + } + return false + } + + // resPredicate ignores DeleteEvents for pipe-pods only (marked with task.PipePodAnnotation) + resPredicate := predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return true }, + DeleteFunc: func(e event.DeleteEvent) bool { return !hasAnnotation(task.PipePodAnnotation, e.Meta.GetAnnotations()) }, + UpdateFunc: func(event.UpdateEvent) bool { return true }, + GenericFunc: func(event.GenericEvent) bool { return true }, + } + return ctrl.NewControllerManagedBy(mgr). For(&kudov1beta1.Instance{}). Owns(&kudov1beta1.Instance{}). @@ -93,7 +115,8 @@ func (r *Reconciler) SetupWithManager( Owns(&corev1.Service{}). Owns(&batchv1.Job{}). Owns(&appsv1.StatefulSet{}). - Owns(&corev1.Pod{}). // TODO (ad): filter out Pod Delete events (possibly also all other k8s native objects?) #1116 + Owns(&corev1.Pod{}). + WithEventFilter(resPredicate). Watches(&source.Kind{Type: &kudov1beta1.OperatorVersion{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: addOvRelatedInstancesToReconcile}). Complete(r) } @@ -132,7 +155,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) { oldInstance := instance.DeepCopy() if err != nil { if apierrors.IsNotFound(err) { // not retrying if instance not found, probably someone manually removed it? - log.Printf("Instances in namespace %s not found, not retrying reconcile since this error is usually not recoverable (without manual intervention).", request.NamespacedName) + log.Printf("Instances %s was deleted, nothing to reconcile.", request.NamespacedName) return reconcile.Result{}, nil } return reconcile.Result{}, err diff --git a/pkg/controller/instance/instance_controller_test.go b/pkg/controller/instance/instance_controller_test.go index 99f6f015d..168da6a78 100644 --- a/pkg/controller/instance/instance_controller_test.go +++ b/pkg/controller/instance/instance_controller_test.go @@ -179,7 +179,7 @@ func Test_makePipes(t *testing.T) { Kind: "Pipe", Spec: v1beta1.TaskSpec{ PipeTaskSpec: v1beta1.PipeTaskSpec{ - Container: "container.yaml", + Pod: "pipe-pod.yaml", Pipe: []v1beta1.PipeSpec{ { File: "foo.txt", @@ -210,7 +210,7 @@ func Test_makePipes(t *testing.T) { Kind: "Pipe", Spec: v1beta1.TaskSpec{ PipeTaskSpec: v1beta1.PipeTaskSpec{ - Container: "container.yaml", + Pod: "pipe-pod.yaml", Pipe: []v1beta1.PipeSpec{ { File: "foo.txt", @@ -226,7 +226,7 @@ func Test_makePipes(t *testing.T) { Kind: "Pipe", Spec: v1beta1.TaskSpec{ PipeTaskSpec: v1beta1.PipeTaskSpec{ - Container: "container.yaml", + Pod: "pipe-pod.yaml", Pipe: []v1beta1.PipeSpec{ { File: "bar.txt", diff --git a/pkg/engine/task/task.go b/pkg/engine/task/task.go index 1678135a9..33a88ae38 100644 --- a/pkg/engine/task/task.go +++ b/pkg/engine/task/task.go @@ -117,7 +117,7 @@ func newPipe(task *v1beta1.Task) (Tasker, error) { return PipeTask{ Name: task.Name, - Container: task.Spec.PipeTaskSpec.Container, + Pod: task.Spec.PipeTaskSpec.Pod, PipeFiles: pipeFiles, }, nil } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index 8599c5f9a..221e7f42d 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -23,6 +23,8 @@ var ( // name of the main pipe pod container pipePodContainerName = "waiter" + + PipePodAnnotation = "kudo.dev/pipepod" ) const ( @@ -41,7 +43,7 @@ const ( type PipeTask struct { Name string - Container string + Pod string PipeFiles []PipeFile } @@ -53,32 +55,32 @@ type PipeFile struct { func (pt PipeTask) Run(ctx Context) (bool, error) { // 1. - Render container template - - rendered, err := render([]string{pt.Container}, ctx) + rendered, err := render([]string{pt.Pod, pt.Pod}, ctx) if err != nil { return false, fatalExecutionError(err, taskRenderingError, ctx.Meta) } // 2. - Create core/v1 container object - - container, err := unmarshal(rendered[pt.Container]) + usrPod, err := unmarshal(rendered[pt.Pod]) if err != nil { return false, fatalExecutionError(err, resourceUnmarshalError, ctx.Meta) } // 3. - Validate the container object - - err = validate(container, pt.PipeFiles) + err = validate(usrPod, pt.PipeFiles) if err != nil { return false, fatalExecutionError(err, resourceValidationError, ctx.Meta) } // 4. - Create a pod using the container - podName := PipePodName(ctx.Meta) - podStr, err := pipePod(container, podName) + podYaml, err := pipePod(usrPod, podName) if err != nil { return false, fatalExecutionError(err, pipeTaskError, ctx.Meta) } // 5. - Kustomize pod with metadata - podObj, err := kustomize(map[string]string{"pipe-pod.yaml": podStr}, ctx.Meta, ctx.Enhancer) + podObj, err := kustomize(map[string]string{"pipe-pod.yaml": podYaml}, ctx.Meta, ctx.Enhancer) if err != nil { return false, fatalExecutionError(err, taskEnhancementError, ctx.Meta) } @@ -136,13 +138,13 @@ func (pt PipeTask) Run(ctx Context) (bool, error) { return true, nil } -func unmarshal(ctrStr string) (*corev1.Container, error) { - ctr := &corev1.Container{} - err := yaml.Unmarshal([]byte(ctrStr), ctr) +func unmarshal(podStr string) (*corev1.Pod, error) { + pod := &corev1.Pod{} + err := yaml.Unmarshal([]byte(podStr), pod) if err != nil { - return nil, fmt.Errorf("failed to unmarshall pipe container: %v", err) + return nil, fmt.Errorf("failed to unmarshall pipe pod: %v", err) } - return ctr, nil + return pod, nil } var pipeFileRe = regexp.MustCompile(`[-._a-zA-Z0-9]+`) @@ -152,21 +154,69 @@ func isRelative(base, file string) bool { return err == nil && !strings.HasPrefix(rp, ".") } -// validate method validates passed pipe container. It is expected to: -// - have exactly one volume mount -// - not have readiness probes (as k87 does not support them for init containers) -// - pipe files should have valid names and exist within the volume mount -func validate(ctr *corev1.Container, ff []PipeFile) error { - if len(ctr.VolumeMounts) != 1 { - return errors.New("pipe container should have exactly one volume mount") +// sharedVolumeName method searches pod volumes for one of the type emptyDir and returns +// its name. Method expects exactly one such volume to exits and will return an error otherwise. +func sharedVolumeName(pod *corev1.Pod) (string, error) { + name := "" + vols := 0 + for _, v := range pod.Spec.Volumes { + if v.EmptyDir != nil { + name = v.Name + vols++ + } + } + if name == "" || vols != 1 { + return "", errors.New("pipe pod should define one emptyDir shared volume where the artifacts are temporary stored") + } + return name, nil +} + +// sharedMountPath method searches pod initContainer volume mounts for one with a passed name. +// It returns the mount path of the volume if found or an error otherwise. +func sharedMountPath(pod *corev1.Pod, volName string) (string, error) { + mountPath := "" + for _, vm := range pod.Spec.InitContainers[0].VolumeMounts { + if vm.Name == volName { + mountPath = vm.MountPath + } + } + + if mountPath == "" { + return "", fmt.Errorf("pipe pod should save generated artifacts in %s", volName) + } + return mountPath, nil +} + +// validate method validates passed pipe pod. It is expected to: +// - have one init container and zero containers specified +// - one emptyDir shared volume should be defined where the artifacts will be stored +// - shared volume should be mounted in the init container +// - pipe files should have valid names and exist within mounted shared volume +// - pod should not have a RestartPolicy defined (or define an "OnFailure" one) +func validate(pod *corev1.Pod, ff []PipeFile) error { + if len(pod.Spec.Containers) > 0 { + return errors.New("pipe pod should not have containers. pipe artifacts are generated in the init container") + } + + if len(pod.Spec.InitContainers) != 1 { + return errors.New("pipe pod should define one init container that generated artifacts defined") + } + + if pod.Spec.RestartPolicy != "" && pod.Spec.RestartPolicy != corev1.RestartPolicyOnFailure { + return errors.New("pipe pod RestartPolicy should be OnFailure") + } + + sharedVolName, err := sharedVolumeName(pod) + if err != nil { + return err } - if ctr.ReadinessProbe != nil { - return errors.New("pipe container does not support readiness probes") + mountPath, err := sharedMountPath(pod, sharedVolName) + if err != nil { + return err } // check if all referenced pipe files are children of the container mountPath - mountPath := ctr.VolumeMounts[0].MountPath for _, f := range ff { if !isRelative(mountPath, f.File) { return fmt.Errorf("pipe file %s should be a child of %s mount path", f.File, mountPath) @@ -183,41 +233,28 @@ func validate(ctr *corev1.Container, ff []PipeFile) error { return nil } -func pipePod(ctr *corev1.Container, name string) (string, error) { - volumeName := ctr.VolumeMounts[0].Name - mountPath := ctr.VolumeMounts[0].MountPath +func pipePod(pod *corev1.Pod, name string) (string, error) { + volumeName, _ := sharedVolumeName(pod) + mountPath, _ := sharedMountPath(pod, volumeName) - pod := corev1.Pod{ - TypeMeta: metav1.TypeMeta{ - Kind: "Pod", - APIVersion: "v1", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - Spec: corev1.PodSpec{ - Volumes: []corev1.Volume{ - { - Name: volumeName, - VolumeSource: corev1.VolumeSource{EmptyDir: nil}, - }, - }, - InitContainers: []corev1.Container{*ctr}, - Containers: []corev1.Container{ + if pod.GetAnnotations() == nil { + pod.SetAnnotations(make(map[string]string)) + } + pod.Annotations[PipePodAnnotation] = "true" + pod.ObjectMeta.Name = name + pod.Spec.RestartPolicy = corev1.RestartPolicyOnFailure + pod.Spec.Containers = []corev1.Container{ + { + Name: pipePodContainerName, + Image: "busybox", + Command: []string{"/bin/sh", "-c"}, + Args: []string{"sleep infinity"}, + VolumeMounts: []corev1.VolumeMount{ { - Name: pipePodContainerName, - Image: "busybox", - Command: []string{"/bin/sh", "-c"}, - Args: []string{"sleep infinity"}, - VolumeMounts: []corev1.VolumeMount{ - { - Name: volumeName, - MountPath: mountPath, - }, - }, + Name: volumeName, + MountPath: mountPath, }, }, - RestartPolicy: corev1.RestartPolicyOnFailure, }, } diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go index 750e9ca2f..7153abe11 100644 --- a/pkg/engine/task/task_pipe_test.go +++ b/pkg/engine/task/task_pipe_test.go @@ -6,6 +6,7 @@ import ( "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/engine/renderer" + "github.com/stretchr/testify/assert" ) func Test_isRelative(t *testing.T) { @@ -117,3 +118,207 @@ func TestPipeNames(t *testing.T) { }) } } + +func Test_validate(t *testing.T) { + tests := []struct { + name string + podYaml string + ff []PipeFile + wantErr bool + }{ + { + name: "a valid pipe pod with one init container", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: shared-data + emptyDir: {} + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: false, + }, + { + name: "an valid pipe pod with a container", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: shared-data + emptyDir: {} + + containers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: true, + }, + { + name: "an invalid pipe pod with wrong volume mount", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: conf-data + configMap: + name: my-conf + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: true, + }, + { + name: "a valid pipe pod with at least one emptyDir volume mount", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: conf-data + configMap: + name: my-conf + - name: shared-data + emptyDir: {} + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: false, + }, + { + name: "an invalid pipe pod where init container does not mount shared volume", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: conf-data + configMap: + name: my-conf + - name: shared-data + emptyDir: {} + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: conf-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: true, + }, + { + name: "an invalid pipe pod where init container does not mount shared volume", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: conf-data + configMap: + name: my-conf + - name: shared-data + emptyDir: {} + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/var/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pod, err := unmarshal(tt.podYaml) + assert.NoError(t, err, "error during pipe pod unmarshaling") + + if err := validate(pod, tt.ff); (err != nil) != tt.wantErr { + t.Errorf("validate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/pkg/engine/task/task_test.go b/pkg/engine/task/task_test.go index 9de5d0bb6..b9055876c 100644 --- a/pkg/engine/task/task_test.go +++ b/pkg/engine/task/task_test.go @@ -64,14 +64,14 @@ spec: name: pipe-task kind: Pipe spec: - container: container.yaml + pod: pipe-pod.yaml pipe: - file: /tmp/foo.txt kind: Secret key: Foo`, want: PipeTask{ - Name: "pipe-task", - Container: "container.yaml", + Name: "pipe-task", + Pod: "pipe-pod.yaml", PipeFiles: []PipeFile{ { File: "/tmp/foo.txt", @@ -88,14 +88,14 @@ spec: name: pipe-task kind: Pipe spec: - container: container.yaml + pod: pipe-pod.yaml pipe: - file: /tmp/bar.txt kind: ConfigMap key: Bar`, want: PipeTask{ - Name: "pipe-task", - Container: "container.yaml", + Name: "pipe-task", + Pod: "pipe-pod.yaml", PipeFiles: []PipeFile{ { File: "/tmp/bar.txt", @@ -112,7 +112,7 @@ spec: name: pipe-task kind: Pipe spec: - container: container.yaml + pod: pipe-pod.yaml pipe: - file: /tmp/bar.txt kind: Invalid @@ -126,7 +126,7 @@ spec: name: pipe-task kind: Pipe spec: - container: container.yaml + pod: pipe-pod.yaml pipe: - file: kind: Secret @@ -135,12 +135,12 @@ spec: wantErr: true, }, { - name: "pipe task keu is invalid", + name: "pipe task key is invalid", taskYaml: ` name: pipe-task kind: Pipe spec: - container: container.yaml + pod: pipe-pod.yaml pipe: - file: /tmp/bar.txt" kind: Secret diff --git a/pkg/kudoctl/packages/package.go b/pkg/kudoctl/packages/package.go index a0d6653bd..63a4d3d20 100644 --- a/pkg/kudoctl/packages/package.go +++ b/pkg/kudoctl/packages/package.go @@ -107,7 +107,7 @@ func validateTask(t v1beta1.Task, templates map[string]string) []string { case task.DeleteTaskKind: resources = t.Spec.ResourceTaskSpec.Resources case task.PipeTaskKind: - resources = append(resources, t.Spec.PipeTaskSpec.Container) + resources = append(resources, t.Spec.PipeTaskSpec.Pod) if len(t.Spec.PipeTaskSpec.Pipe) == 0 { errs = append(errs, fmt.Sprintf("task %s does not have pipe files specified", t.Name)) From 04c5870de9ff6a842fd3e63aac6053bff1c5c340 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Tue, 3 Dec 2019 17:07:25 +0100 Subject: [PATCH 12/19] Making linter happy --- pkg/controller/instance/instance_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index b3150b0d4..2f5debae1 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -92,7 +92,7 @@ func (r *Reconciler) SetupWithManager( if annotations == nil { return false } - for k, _ := range annotations { + for k := range annotations { if k == key { return true } From 67b9a1e67ff67326fac8ed15b1cfd33b441d6d0e Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Wed, 4 Dec 2019 00:10:28 +0100 Subject: [PATCH 13/19] Added more unit tests --- .../instance/instance_controller.go | 12 ++- .../instance/instance_controller_test.go | 48 +++++++++- pkg/engine/task/task_pipe_test.go | 92 +++++++++++++++++++ 3 files changed, 148 insertions(+), 4 deletions(-) diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index 2f5debae1..adaef1bf8 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -257,7 +257,10 @@ func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.Operat } params := makeParams(instance, ov) - pipes := makePipes(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta) + pipes, err := makePipes(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta) + if err != nil { + return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not make task pipes: %v", engine.ErrFatalExecution, err), EventName: "InvalidPlan"} + } return &workflow.ActivePlan{ Name: activePlanStatus.Name, @@ -358,7 +361,7 @@ func makeParams(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.Ope } // makePipes generates {{ Pipes.* }} map of keys and values which is later used during template rendering. -func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta *engine.Metadata) map[string]string { +func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta *engine.Metadata) (map[string]string, error) { taskByName := func(name string) (*v1beta1.Task, bool) { for _, t := range tasks { if t.Name == name { @@ -383,6 +386,9 @@ func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta if t, ok := taskByName(tn); ok && t.Kind == task.PipeTaskKind { for _, pipe := range t.Spec.PipeTaskSpec.Pipe { + if _, ok := pipes[pipe.Key]; ok { + return nil, fmt.Errorf("duplicated pipe key %s", pipe.Key) + } pipes[pipe.Key] = task.PipeArtifactName(rmeta, pipe.Key) } } @@ -390,7 +396,7 @@ func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta } } - return pipes + return pipes, nil } func parameterDiff(old, new map[string]string) map[string]string { diff --git a/pkg/controller/instance/instance_controller_test.go b/pkg/controller/instance/instance_controller_test.go index 168da6a78..1828e3b4c 100644 --- a/pkg/controller/instance/instance_controller_test.go +++ b/pkg/controller/instance/instance_controller_test.go @@ -126,6 +126,7 @@ func Test_makePipes(t *testing.T) { tasks []v1beta1.Task emeta *engine.Metadata want map[string]string + wantErr bool }{ { name: "no tasks, no pipes", @@ -244,10 +245,55 @@ func Test_makePipes(t *testing.T) { "Bar": "firstoperatorinstance.deploy.phase.steptwo.tasktwo.bar", }, }, + { + name: "one pipe task, duplicated pipe keys", + planName: "deploy", + plan: &v1beta1.Plan{Strategy: "serial", Phases: []v1beta1.Phase{ + { + Name: "phase", Strategy: "serial", Steps: []v1beta1.Step{ + { + Name: "step", Tasks: []string{"task"}}, + }}, + }}, + tasks: []v1beta1.Task{ + { + Name: "task", + Kind: "Pipe", + Spec: v1beta1.TaskSpec{ + PipeTaskSpec: v1beta1.PipeTaskSpec{ + Pod: "pipe-pod.yaml", + Pipe: []v1beta1.PipeSpec{ + { + File: "foo.txt", + Kind: "Secret", + Key: "Foo", + }, + { + File: "bar.txt", + Kind: "ConfigMap", + Key: "Foo", + }, + }, + }, + }, + }, + }, + emeta: meta, + want: nil, + wantErr: true, + }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := makePipes(tt.planName, tt.plan, tt.tasks, tt.emeta) + got, err := makePipes(tt.planName, tt.plan, tt.tasks, tt.emeta) + if err != nil { + if !tt.wantErr { + t.Fatalf("makePipes() error = %v, wantErr %v", err, tt.wantErr) + } + return + } + assert.Equal(t, tt.want, got) }) } diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go index 7153abe11..673a51180 100644 --- a/pkg/engine/task/task_pipe_test.go +++ b/pkg/engine/task/task_pipe_test.go @@ -6,7 +6,11 @@ import ( "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/engine/renderer" + "github.com/spf13/afero" "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/yaml" ) func Test_isRelative(t *testing.T) { @@ -322,3 +326,91 @@ spec: }) } } + +func Test_pipeFiles(t *testing.T) { + meta := renderer.Metadata{ + Metadata: engine.Metadata{InstanceName: "foo-instance"}, + PlanName: "deploy", + PhaseName: "first", + StepName: "step", + TaskName: "genfiles", + } + + tests := []struct { + name string + data map[string]string + file PipeFile + meta renderer.Metadata + wantArtifact interface{} + wantErr bool + }{ + { + name: "pipe a secret", + data: map[string]string{"/tmp/foo.txt": "foo"}, + file: PipeFile{ + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "Foo", + }, + meta: meta, + wantArtifact: v1.Secret{ + TypeMeta: metav1.TypeMeta{Kind: "Secret", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "fooinstance.deploy.first.step.genfiles.foo"}, + Data: map[string][]byte{"foo.txt": []byte("foo")}, + Type: v1.SecretTypeOpaque, + }, + wantErr: false, + }, + { + name: "pipe a configMap", + data: map[string]string{"/tmp/bar.txt": "bar"}, + file: PipeFile{ + File: "/tmp/bar.txt", + Kind: PipeFileKindConfigMap, + Key: "Bar", + }, + meta: meta, + wantArtifact: v1.ConfigMap{ + TypeMeta: metav1.TypeMeta{Kind: "ConfigMap", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "fooinstance.deploy.first.step.genfiles.bar"}, + BinaryData: map[string][]byte{"bar.txt": []byte("bar")}, + }, + wantErr: false, + }, + { + name: "return an error for an invalid pipe", + data: map[string]string{"nope.txt": ""}, + file: PipeFile{ + File: "nope.txt", + Kind: "Invalid", + Key: "Nope", + }, + meta: meta, + wantArtifact: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fs := afero.NewMemMapFs() + + for path, data := range tt.data { + assert.NoError(t, afero.WriteFile(fs, path, []byte(data), 0644), "error while preparing test: %s", tt.name) + } + + got, err := pipeFiles(fs, []PipeFile{tt.file}, tt.meta) + if err != nil { + if !tt.wantErr { + t.Fatalf("pipeFiles() error = %v, wantErr %v", err, tt.wantErr) + } + return + } + + out, err := yaml.Marshal(tt.wantArtifact) + assert.NoError(t, err, "failure during marshaling of the test pipe artifact in test: %s", tt.name) + + want := map[string]string{tt.file.Key: string(out)} + assert.Equal(t, want, got, "pipeFiles() unexpected return value") + }) + } +} From ef7ce240ffc44c230587ea09b99521370f34786f Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Wed, 4 Dec 2019 00:53:52 +0100 Subject: [PATCH 14/19] Minor comment improvement --- pkg/engine/task/pod_exec.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index 2d2f42a0a..6e35124bf 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -163,8 +163,9 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e // TL;DR: // - execute PodExec.Run() in a goroutine when using io.Pipe for Out or Err streams, they // have to be consumed first because io.Pipe is synchronous (and thread-safe) - // - there seems to be a bug when using BOTH Out AND Err pipe-based streams. When trying to - // consume both (in goroutines), one of them ends up blocking the execution ¯\_(ツ)_/¯ + // - there seems to be a bug when consuming the error stream of the above command: EOF is never + // sent so reading it never ends ¯\_(ツ)_/¯ This is why we use simple bytes.Buffer for the + // Err stream (it is only used in the error message) // // See `kubectl cp` copyFromPod method for another example: // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 From e83fc6421fb00c1148b2a59380032a50e5b3bfcf Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Wed, 4 Dec 2019 19:32:16 +0100 Subject: [PATCH 15/19] First round of review feedback --- keps/0017-pipe-tasks.md | 23 +++++++--- .../instance/instance_controller.go | 44 ++++++++++--------- .../instance/instance_controller_test.go | 4 +- pkg/engine/task/task_pipe.go | 20 ++++----- pkg/engine/task/task_pipe_test.go | 6 +-- 5 files changed, 54 insertions(+), 43 deletions(-) diff --git a/keps/0017-pipe-tasks.md b/keps/0017-pipe-tasks.md index 1595738e0..1ee598460 100644 --- a/keps/0017-pipe-tasks.md +++ b/keps/0017-pipe-tasks.md @@ -43,7 +43,7 @@ Allowing to pipe all kind of files (>1Mb) between tasks requires a general-purpo ## Proposal -This section describes how pipe tasks and files they produce are configured in the operator. Here is a pipe task definition that produces a file that will be stored as a Secret: +This section describes how pipe tasks and files they produce are configured in the operator.This proposal is currently limited to pipe tasks which create files which are assigned to a key in a ConfigMap or a Secret. Here is a pipe task definition that produces a file that will be stored as a Secret: ```yaml tasks: - name: gencert @@ -52,11 +52,20 @@ tasks: pod: cert-pod.yaml pipe: - file: /usr/share/MyKey.key - kind: Secret # ConfigMap + kind: Secret # or a ConfigMap key: Mycertificate ``` -`pod` field is described in detail below. `key` will be used by the resources referencing generated file as following: `{{.Pipes.Mycertificate}}`. In the above example we would create a secret named `instancemyapp.deploy.bootstrap.gencert.mycertificate` to capture instance name along with plan/phase/step/task of the secret origin. The secret name would be stable so that a user can rerun the certificate generating task and reload all pods using it. Pipe file name will be used as Secret/ConfigMap data key. We would also use labels ensuring that the secret is cleaned up when the corresponding Instance is removed. `pipe` field is an array and can define how multiple generated files are stored and referenced. +`pod` field is described in detail below. `key` will be used by in the template file to reference generated artifact e.g: + ```yaml +volumes: +- name: cert + secret: + secretName: {{ .Pipes.Mycertificate }} +``` +will create as a volume from the generated secret. + +In the above example we would create a secret named `instancemyapp.deploy.bootstrap.gencert.mycertificate` which captures instance name along with plan/phase/step/task of the secret origin. The secret name would be stable so that a user can rerun the certificate generating task and reload all pods using it. Pipe file name is used as Secret/ConfigMap data key. Secret/ConfigMap will be owned by the corresponding Instance so that artifacts are cleaned up when the Instance is deleted. `pipe` field is an array and can define how multiple generated files are stored and referenced. The corresponding `gencert` task can be used as usual in e.g.: ```yaml @@ -74,9 +83,9 @@ plans: Note that piped files has to be generated before they can be used. In the example above, `bootstrap` phase has a strategy `serial` so that certificate generated in the `gencert` step can be used in subsequent steps. Or stated differently resources can not reference piped secrets/configmaps generated within the same step or within a parallel series of steps (it has to be a different step in the phase with serial strategy or a different phase). -For the pipe task `pod`, we allow a [core/v1 Pod](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#pod-v1-core) to be specified, however, there are limitations. Reasons for that are explained below in the implementation details. In a nutshell: -- a pipe pod should generate artifacts in the init container -- it has to define and mount an emptyDir volume (where generated files are stored) +Pipe task's `spec.pod` field must reference a [core/v1 Pod](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#pod-v1-core) template. However, there are limitations. Reasons for that are explained below in the implementation details. In a nutshell: +- a pipe pod should generate artifacts in its init container +- it has to define and mount an emptyDir volume (where its generated files are stored) ```yaml apiVersion: v1 @@ -115,7 +124,7 @@ spec: ### Limitations - File generating Pod has to be side-effect free (meaning side-effects that are observable outside of the container like a 3rd party API call) as the container might be executed multiple times on failure. A `restartPolicy: OnFailure` is used for the pipe pod. -- Only files <1Mb are applicable to be stored as ConfigMap or Secret. +- Only files <1Mb are applicable to be stored as ConfigMap or Secret. A pipe task will fail should it try to copy files >1Mb ### Implementation Details/Notes/Constraints There are several ways to implement pipe tasks, each one having its challenges and complexities. The approach below allows us not to worry about Pod container life-cycle as well as keep the storing logic in the KUDO controller: diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go index adaef1bf8..daf284cfc 100644 --- a/pkg/controller/instance/instance_controller.go +++ b/pkg/controller/instance/instance_controller.go @@ -23,30 +23,27 @@ import ( "reflect" "time" + "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" + kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" "github.com/kudobuilder/kudo/pkg/engine" "github.com/kudobuilder/kudo/pkg/engine/renderer" "github.com/kudobuilder/kudo/pkg/engine/task" "github.com/kudobuilder/kudo/pkg/engine/workflow" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/source" - - "k8s.io/apimachinery/pkg/runtime" - "github.com/kudobuilder/kudo/pkg/util/kudo" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/tools/record" - - "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" - kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" appsv1 "k8s.io/api/apps/v1" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "sigs.k8s.io/controller-runtime/pkg/source" ) // Reconciler reconciles an Instance object. @@ -88,6 +85,7 @@ func (r *Reconciler) SetupWithManager( return requests }) + // hasAnnotation returns true if an annotation with the passed key is found in the map hasAnnotation := func(key string, annotations map[string]string) bool { if annotations == nil { return false @@ -100,7 +98,11 @@ func (r *Reconciler) SetupWithManager( return false } - // resPredicate ignores DeleteEvents for pipe-pods only (marked with task.PipePodAnnotation) + // resPredicate ignores DeleteEvents for pipe-pods only (marked with task.PipePodAnnotation). This is due to an + // inherent race that was described in detail in #1116 (https://github.com/kudobuilder/kudo/issues/1116) + // tl;dr: pipe task will delete the pipe pod at the end of the execution. this would normally trigger another + // Instance reconciliation which might end up copying pipe files twice. we avoid this by explicitly ignoring + // DeleteEvents for pipe-pods. resPredicate := predicate.Funcs{ CreateFunc: func(event.CreateEvent) bool { return true }, DeleteFunc: func(e event.DeleteEvent) bool { return !hasAnnotation(task.PipePodAnnotation, e.Meta.GetAnnotations()) }, @@ -155,7 +157,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) { oldInstance := instance.DeepCopy() if err != nil { if apierrors.IsNotFound(err) { // not retrying if instance not found, probably someone manually removed it? - log.Printf("Instances %s was deleted, nothing to reconcile.", request.NamespacedName) + log.Printf("Instance %s was deleted, nothing to reconcile.", request.NamespacedName) return reconcile.Result{}, nil } return reconcile.Result{}, err @@ -256,8 +258,8 @@ func preparePlanExecution(instance *kudov1beta1.Instance, ov *kudov1beta1.Operat return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not find required plan: %v", engine.ErrFatalExecution, activePlanStatus.Name), EventName: "InvalidPlan"} } - params := makeParams(instance, ov) - pipes, err := makePipes(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta) + params := paramsMap(instance, ov) + pipes, err := pipesMap(activePlanStatus.Name, &planSpec, ov.Spec.Tasks, meta) if err != nil { return nil, &engine.ExecutionError{Err: fmt.Errorf("%wcould not make task pipes: %v", engine.ErrFatalExecution, err), EventName: "InvalidPlan"} } @@ -342,8 +344,8 @@ func (r *Reconciler) getOperatorVersion(instance *kudov1beta1.Instance) (ov *kud return ov, nil } -// makeParams generates {{ Params.* }} map of keys and values which is later used during template rendering. -func makeParams(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string { +// paramsMap generates {{ Params.* }} map of keys and values which is later used during template rendering. +func paramsMap(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.OperatorVersion) map[string]string { params := make(map[string]string) for k, v := range instance.Spec.Parameters { @@ -360,8 +362,8 @@ func makeParams(instance *kudov1beta1.Instance, operatorVersion *kudov1beta1.Ope return params } -// makePipes generates {{ Pipes.* }} map of keys and values which is later used during template rendering. -func makePipes(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta *engine.Metadata) (map[string]string, error) { +// pipesMap generates {{ Pipes.* }} map of keys and values which is later used during template rendering. +func pipesMap(planName string, plan *v1beta1.Plan, tasks []v1beta1.Task, emeta *engine.Metadata) (map[string]string, error) { taskByName := func(name string) (*v1beta1.Task, bool) { for _, t := range tasks { if t.Name == name { diff --git a/pkg/controller/instance/instance_controller_test.go b/pkg/controller/instance/instance_controller_test.go index 1828e3b4c..49a7bdc35 100644 --- a/pkg/controller/instance/instance_controller_test.go +++ b/pkg/controller/instance/instance_controller_test.go @@ -286,10 +286,10 @@ func Test_makePipes(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := makePipes(tt.planName, tt.plan, tt.tasks, tt.emeta) + got, err := pipesMap(tt.planName, tt.plan, tt.tasks, tt.emeta) if err != nil { if !tt.wantErr { - t.Fatalf("makePipes() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("pipesMap() error = %v, wantErr %v", err, tt.wantErr) } return } diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index 221e7f42d..00436f52a 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -111,7 +111,7 @@ func (pt PipeTask) Run(ctx Context) (bool, error) { // 9. - Create k8s artifacts (ConfigMap/Secret) from the pipe files - log.Printf("PipeTask: %s/%s creating pipe artifacts", ctx.Meta.InstanceNamespace, ctx.Meta.InstanceName) - artStr, err := pipeFiles(fs, pt.PipeFiles, ctx.Meta) + artStr, err := storeArtifacts(fs, pt.PipeFiles, ctx.Meta) if err != nil { return false, err } @@ -223,7 +223,7 @@ func validate(pod *corev1.Pod, ff []PipeFile) error { } fileName := path.Base(f.File) - // Same as K87 we use file names as ConfigMap data keys. A valid key name for a ConfigMap must consist + // Same as k8s we use file names as ConfigMap data keys. A valid key name for a ConfigMap must consist // of alphanumeric characters, '-', '_' or '.' (e.g. 'key.name', or 'KEY_NAME', or 'key-name', regex // used for validation is '[-._a-zA-Z0-9]+') if !pipeFileRe.MatchString(fileName) { @@ -298,9 +298,9 @@ func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { return err } -// pipeFiles iterates through passed pipe files and their copied data, reads them, constructs k8s artifacts +// storeArtifacts iterates through passed pipe files and their copied data, reads them, constructs k8s artifacts // and marshals them. -func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[string]string, error) { +func storeArtifacts(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[string]string, error) { artifacts := map[string]string{} for _, pf := range files { @@ -312,9 +312,9 @@ func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[strin var art string switch pf.Kind { case PipeFileKindSecret: - art, err = pipeSecret(pf, data, meta) + art, err = storeSecret(pf, data, meta) case PipeFileKindConfigMap: - art, err = pipeConfigMap(pf, data, meta) + art, err = storeConfigMap(pf, data, meta) default: return nil, fmt.Errorf("unknown pipe file kind: %+v", pf) } @@ -328,9 +328,9 @@ func pipeFiles(fs afero.Fs, files []PipeFile, meta renderer.Metadata) (map[strin return artifacts, nil } -// pipeSecret method creates a core/v1/Secret object using passed data. Pipe file name is used +// storeSecret method creates a core/v1/Secret object using passed data. Pipe file name is used // as Secret data key. Secret name will be of the form ....- -func pipeSecret(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { +func storeSecret(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { name := PipeArtifactName(meta, pf.Key) key := path.Base(pf.File) @@ -354,9 +354,9 @@ func pipeSecret(pf PipeFile, data []byte, meta renderer.Metadata) (string, error return string(b), nil } -// pipeConfigMap method creates a core/v1/ConfigMap object using passed data. Pipe file name is used +// storeConfigMap method creates a core/v1/ConfigMap object using passed data. Pipe file name is used // as ConfigMap data key. ConfigMap name will be of the form ....- -func pipeConfigMap(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { +func storeConfigMap(pf PipeFile, data []byte, meta renderer.Metadata) (string, error) { name := PipeArtifactName(meta, pf.Key) key := path.Base(pf.File) diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go index 673a51180..cf076c889 100644 --- a/pkg/engine/task/task_pipe_test.go +++ b/pkg/engine/task/task_pipe_test.go @@ -398,10 +398,10 @@ func Test_pipeFiles(t *testing.T) { assert.NoError(t, afero.WriteFile(fs, path, []byte(data), 0644), "error while preparing test: %s", tt.name) } - got, err := pipeFiles(fs, []PipeFile{tt.file}, tt.meta) + got, err := storeArtifacts(fs, []PipeFile{tt.file}, tt.meta) if err != nil { if !tt.wantErr { - t.Fatalf("pipeFiles() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("storeArtifacts() error = %v, wantErr %v", err, tt.wantErr) } return } @@ -410,7 +410,7 @@ func Test_pipeFiles(t *testing.T) { assert.NoError(t, err, "failure during marshaling of the test pipe artifact in test: %s", tt.name) want := map[string]string{tt.file.Key: string(out)} - assert.Equal(t, want, got, "pipeFiles() unexpected return value") + assert.Equal(t, want, got, "storeArtifacts() unexpected return value") }) } } From 82be1a2369cab6e43fee9b197e2a6ca649fa0537 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 5 Dec 2019 00:27:58 +0100 Subject: [PATCH 16/19] Removed `io.Pipe` when executing pod commands and with them goroutines and a lot of complexity. --- pkg/engine/task/pod_exec.go | 73 +++++++++++-------------------------- 1 file changed, 21 insertions(+), 52 deletions(-) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/pod_exec.go index 6e35124bf..b26436a39 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/pod_exec.go @@ -5,9 +5,9 @@ import ( "bytes" "fmt" "io" - "io/ioutil" "os" "strconv" + "strings" "github.com/spf13/afero" v1 "k8s.io/api/core/v1" @@ -44,6 +44,12 @@ type PodExec struct { // also `kubectl cp`) doing under the hood: a POST request is made to the `exec` subresource // of the v1/pods endpoint containing the pod information and the command. Here is a good article // describing it in detail: https://erkanerol.github.io/post/how-kubectl-exec-works/ +// +// The result of the command execution is returned via passed PodExec.Out and PodExec.Err streams. +// Run calls the remotecommand executor, which executes the command in the remote pod, captures +// stdout and stderr, writes them to the provided Out and Err writers and then returns with an exit code. +// Note that when using SYNCHRONOUS io.Pipe for Out or Err streams Run call will not return until the +// streams are consumed. Here, Run has to be executed in a goroutine. func (pe *PodExec) Run() error { codec := serializer.NewCodecFactory(scheme.Scheme) restClient, err := apiutil.RESTClientForGVK( @@ -93,7 +99,7 @@ func (pe *PodExec) Run() error { // FileSize fetches the size of a file in a remote pod. It runs `stat -c %s file` command in the // pod and parses the output. func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { - readout, stdout := io.Pipe() + stdout := strings.Builder{} pe := PodExec{ RestCfg: restCfg, @@ -102,31 +108,18 @@ func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { ContainerName: pipePodContainerName, Args: []string{"stat", "-c", "%s", file}, In: nil, - Out: stdout, + Out: &stdout, Err: nil, TTY: true, // this will forward 2>&1. otherwise, reading from Out will never return for e.g. missing files } - errCh := make(chan error, 1) - go func() { - defer stdout.Close() // Close never returns an error - errCh <- pe.Run() - }() - - buf, err := ioutil.ReadAll(readout) - if err != nil { - return 0, fmt.Errorf("failed to get the size of %s `: %v", file, err) - } - - raw := string(buf) - - // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE STDOUT/STDERR READER. - if err := <-errCh; err != nil { - return 0, fmt.Errorf("failed to get the size of %s, err: %v, %s", file, err, raw) + if err := pe.Run(); err != nil { + return 0, fmt.Errorf("failed to get the size of %s, err: %v, stderr: %s", file, err, stdout.String()) } - raw = raw[:len(raw)-2] // remove trailing \n\r - size, err := strconv.ParseInt(raw, 10, 64) + raw := stdout.String() + trimmed := raw[:len(raw)-2] // remove trailing \n\r + size, err := strconv.ParseInt(trimmed, 10, 64) if err != nil { return 0, fmt.Errorf("failed to parse the size '%s' of the file %s : %v", raw, file, err) } @@ -139,8 +132,8 @@ func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { // it is saved under the same path. Afero filesystem is used to allow the caller downloading and persisting // of multiple files concurrently (afero filesystem is thread-safe). func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) error { - readout, writeout := io.Pipe() - stderr := bytes.Buffer{} + stdout := bytes.Buffer{} + stderr := strings.Builder{} pe := PodExec{ RestCfg: restCfg, @@ -149,39 +142,15 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e ContainerName: pipePodContainerName, Args: []string{"tar", "cf", "-", file}, In: nil, - Out: writeout, + Out: &stdout, Err: &stderr, } - - // IMPORTANT: - // When downloading files using remotecommand.Executor, we pump the contents of the tar file through - // the stdout. PodExec.Run() calls the stream executor, which first writes stdout and stderr of the - // command and then returns with an exit code. Since we're using io.Pipe reader and writer which is - // tread-safe but SYNCHRONOUS, Run() call will not return until we've consumed the streams (and will - // block the execution). For more details on how the underlying streams are copied see remotecommand/v4.go:54 - // - // TL;DR: - // - execute PodExec.Run() in a goroutine when using io.Pipe for Out or Err streams, they - // have to be consumed first because io.Pipe is synchronous (and thread-safe) - // - there seems to be a bug when consuming the error stream of the above command: EOF is never - // sent so reading it never ends ¯\_(ツ)_/¯ This is why we use simple bytes.Buffer for the - // Err stream (it is only used in the error message) - // - // See `kubectl cp` copyFromPod method for another example: - // https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp/cp.go#L291 - errCh := make(chan error, 1) - go func() { - defer writeout.Close() // Close never returns an error - errCh <- pe.Run() - }() - - if err := untarFile(fs, readout, file); err != nil { - return fmt.Errorf("failed to untar pipe file: %v", err) + if err := pe.Run(); err != nil { + return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) } - // THIS CHECK HAS TO HAPPEN AFTER WE CONSUME THE STDOUT/STDERR READER. - if err := <-errCh; err != nil { - return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) + if err := untarFile(fs, &stdout, file); err != nil { + return fmt.Errorf("failed to untar pipe file: %v", err) } return nil From c1087ecfafb094a7bc16653ed88dff7d43c678b3 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 5 Dec 2019 12:58:49 +0100 Subject: [PATCH 17/19] Moved `pod_exec.go` into a separate package additionally PodExec caller can now differentiate between command execution failures (exit code > 0) and other errors --- pkg/engine/task/{ => podexec}/pod_exec.go | 30 ++++++++++++------- pkg/engine/task/task_pipe.go | 36 +++++++++++++---------- 2 files changed, 40 insertions(+), 26 deletions(-) rename pkg/engine/task/{ => podexec}/pod_exec.go (87%) diff --git a/pkg/engine/task/pod_exec.go b/pkg/engine/task/podexec/pod_exec.go similarity index 87% rename from pkg/engine/task/pod_exec.go rename to pkg/engine/task/podexec/pod_exec.go index b26436a39..f46f5d952 100644 --- a/pkg/engine/task/pod_exec.go +++ b/pkg/engine/task/podexec/pod_exec.go @@ -1,8 +1,9 @@ -package task +package podexec import ( "archive/tar" "bytes" + "errors" "fmt" "io" "os" @@ -19,6 +20,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" ) +var ( + // ErrCommandExecution is returned for command with an exit code > 0 + ErrCommandFailed = errors.New("command failed: ") +) + // PodExec defines a command that will be executed in a running Pod. // RestCfg - The REST configuration for the cluster. // PodName - The pod name on which to execute the command. @@ -98,14 +104,14 @@ func (pe *PodExec) Run() error { // FileSize fetches the size of a file in a remote pod. It runs `stat -c %s file` command in the // pod and parses the output. -func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { +func FileSize(file string, pod *v1.Pod, ctrName string, restCfg *rest.Config) (int64, error) { stdout := strings.Builder{} pe := PodExec{ RestCfg: restCfg, PodName: pod.Name, PodNamespace: pod.Namespace, - ContainerName: pipePodContainerName, + ContainerName: ctrName, Args: []string{"stat", "-c", "%s", file}, In: nil, Out: &stdout, @@ -114,7 +120,7 @@ func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { } if err := pe.Run(); err != nil { - return 0, fmt.Errorf("failed to get the size of %s, err: %v, stderr: %s", file, err, stdout.String()) + return 0, fmt.Errorf("%wfailed to get the size of %s, err: %v, stderr: %s", ErrCommandFailed, file, err, stdout.String()) } raw := stdout.String() @@ -131,7 +137,7 @@ func FileSize(file string, pod *v1.Pod, restCfg *rest.Config) (int64, error) { // of the file via the stdout. Locally, the tar file is extracted into the passed afero filesystem where // it is saved under the same path. Afero filesystem is used to allow the caller downloading and persisting // of multiple files concurrently (afero filesystem is thread-safe). -func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) error { +func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, ctrName string, restCfg *rest.Config) error { stdout := bytes.Buffer{} stderr := strings.Builder{} @@ -139,14 +145,14 @@ func DownloadFile(fs afero.Fs, file string, pod *v1.Pod, restCfg *rest.Config) e RestCfg: restCfg, PodName: pod.Name, PodNamespace: pod.Namespace, - ContainerName: pipePodContainerName, + ContainerName: ctrName, Args: []string{"tar", "cf", "-", file}, In: nil, Out: &stdout, Err: &stderr, } if err := pe.Run(); err != nil { - return fmt.Errorf("failed to copy pipe file. err: %v, stderr: %s", err, stderr.String()) + return fmt.Errorf("%wfailed to copy pipe file. err: %v, stderr: %s", ErrCommandFailed, err, stderr.String()) } if err := untarFile(fs, &stdout, file); err != nil { @@ -182,16 +188,13 @@ func untarFile(fs afero.Fs, r io.Reader, fileName string) error { if err != nil { return err } + defer f.Close() // nolint // copy over contents if _, err := io.Copy(f, tr); err != nil { return err } - // manually close here after each file operation; deferring would cause each file close - // to wait until all operations have completed. - f.Close() // nolint - default: fmt.Printf("skipping %s because it is not a regular file or a directory", header.Name) } @@ -199,3 +202,8 @@ func untarFile(fs afero.Fs, r io.Reader, fileName string) error { return nil } + +// HasCommandFailed returns true if PodExec command returned an exit code > 0 +func HasCommandFailed(err error) bool { + return err != nil && errors.Is(err, ErrCommandFailed) +} diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index 00436f52a..6c41d7404 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/kudobuilder/kudo/pkg/engine/renderer" + "github.com/kudobuilder/kudo/pkg/engine/task/podexec" "github.com/spf13/afero" "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" @@ -157,34 +158,28 @@ func isRelative(base, file string) bool { // sharedVolumeName method searches pod volumes for one of the type emptyDir and returns // its name. Method expects exactly one such volume to exits and will return an error otherwise. func sharedVolumeName(pod *corev1.Pod) (string, error) { - name := "" - vols := 0 + volumes := []string{} for _, v := range pod.Spec.Volumes { if v.EmptyDir != nil { - name = v.Name - vols++ + volumes = append(volumes, v.Name) } } - if name == "" || vols != 1 { + if len(volumes) != 1 { return "", errors.New("pipe pod should define one emptyDir shared volume where the artifacts are temporary stored") } - return name, nil + return volumes[0], nil } // sharedMountPath method searches pod initContainer volume mounts for one with a passed name. // It returns the mount path of the volume if found or an error otherwise. func sharedMountPath(pod *corev1.Pod, volName string) (string, error) { - mountPath := "" for _, vm := range pod.Spec.InitContainers[0].VolumeMounts { if vm.Name == volName { - mountPath = vm.MountPath + return vm.MountPath, nil } } - if mountPath == "" { - return "", fmt.Errorf("pipe pod should save generated artifacts in %s", volName) - } - return mountPath, nil + return "", fmt.Errorf("pipe pod should save generated artifacts in %s", volName) } // validate method validates passed pipe pod. It is expected to: @@ -281,16 +276,27 @@ func copyFiles(fs afero.Fs, ff []PipeFile, pod *corev1.Pod, ctx Context) error { // Check the size of the pipe file first. K87 has a inherent limit on the size of // Secret/ConfigMap, so we avoid unnecessary copying of files that are too big by // checking its size first. - size, err := FileSize(f.File, pod, restCfg) + size, err := podexec.FileSize(f.File, pod, pipePodContainerName, restCfg) if err != nil { - return fatalExecutionError(err, pipeTaskError, ctx.Meta) + // Any remote command exit code > 0 is treated as a fatal error since retrying it doesn't make sense + if podexec.HasCommandFailed(err) { + return fatalExecutionError(err, pipeTaskError, ctx.Meta) + } + return err } if size > maxPipeFileSize { return fatalExecutionError(fmt.Errorf("pipe file %s size %d exceeds maximum file size of %d bytes", f.File, size, maxPipeFileSize), pipeTaskError, ctx.Meta) } - return DownloadFile(fs, f.File, pod, restCfg) + if err = podexec.DownloadFile(fs, f.File, pod, pipePodContainerName, restCfg); err != nil { + // Any remote command exit code > 0 is treated as a fatal error since retrying it doesn't make sense + if podexec.HasCommandFailed(err) { + return fatalExecutionError(err, pipeTaskError, ctx.Meta) + } + return err + } + return nil }) } From a6bf10e50cab16f33122ce14f402e1968cf07a31 Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 5 Dec 2019 13:02:21 +0100 Subject: [PATCH 18/19] Making linter happy --- pkg/engine/task/podexec/pod_exec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/engine/task/podexec/pod_exec.go b/pkg/engine/task/podexec/pod_exec.go index f46f5d952..d096e2234 100644 --- a/pkg/engine/task/podexec/pod_exec.go +++ b/pkg/engine/task/podexec/pod_exec.go @@ -21,7 +21,7 @@ import ( ) var ( - // ErrCommandExecution is returned for command with an exit code > 0 + // ErrCommandFailed is returned for command executions with an exit code > 0 ErrCommandFailed = errors.New("command failed: ") ) From 6e38698345f58cb36d9ad19c96908c4fcc9c190a Mon Sep 17 00:00:00 2001 From: Aleksey Dukhovniy Date: Thu, 5 Dec 2019 14:45:26 +0100 Subject: [PATCH 19/19] Additional test for pipe-pod validation --- pkg/engine/task/task_pipe.go | 2 +- pkg/engine/task/task_pipe_test.go | 43 ++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/pkg/engine/task/task_pipe.go b/pkg/engine/task/task_pipe.go index 6c41d7404..a94f8bb6d 100644 --- a/pkg/engine/task/task_pipe.go +++ b/pkg/engine/task/task_pipe.go @@ -19,7 +19,7 @@ import ( "sigs.k8s.io/yaml" ) -var ( +const ( pipeTaskError = "PipeTaskError" // name of the main pipe pod container diff --git a/pkg/engine/task/task_pipe_test.go b/pkg/engine/task/task_pipe_test.go index cf076c889..f8176ee91 100644 --- a/pkg/engine/task/task_pipe_test.go +++ b/pkg/engine/task/task_pipe_test.go @@ -131,7 +131,7 @@ func Test_validate(t *testing.T) { wantErr bool }{ { - name: "a valid pipe pod with one init container", + name: "a valid pipe-pod with one init container", podYaml: ` apiVersion: v1 kind: Pod @@ -160,7 +160,7 @@ spec: wantErr: false, }, { - name: "an valid pipe pod with a container", + name: "an invalid pipe-pod with a container", podYaml: ` apiVersion: v1 kind: Pod @@ -189,7 +189,7 @@ spec: wantErr: true, }, { - name: "an invalid pipe pod with wrong volume mount", + name: "an invalid pipe-pod with wrong volume mount", podYaml: ` apiVersion: v1 kind: Pod @@ -219,7 +219,7 @@ spec: wantErr: true, }, { - name: "a valid pipe pod with at least one emptyDir volume mount", + name: "a valid pipe-pod with two volumes, one of which is emptyDir type", podYaml: ` apiVersion: v1 kind: Pod @@ -251,7 +251,38 @@ spec: wantErr: false, }, { - name: "an invalid pipe pod where init container does not mount shared volume", + name: "an invalid pipe-pod with two emptyDir volumes", + podYaml: ` +apiVersion: v1 +kind: Pod +spec: + volumes: + - name: shared-data-one + emptyDir: {} + - name: shared-data-two + emptyDir: {} + + initContainers: + - name: init + image: busybox + command: [ "/bin/sh", "-c" ] + args: + - touch /tmp/foo.txt + volumeMounts: + - name: shared-data-one + mountPath: /tmp +`, + ff: []PipeFile{ + { + File: "/tmp/foo.txt", + Kind: PipeFileKindSecret, + Key: "foo", + }, + }, + wantErr: true, + }, + { + name: "an invalid pipe-pod where init container does not mount shared volume", podYaml: ` apiVersion: v1 kind: Pod @@ -283,7 +314,7 @@ spec: wantErr: true, }, { - name: "an invalid pipe pod where init container does not mount shared volume", + name: "an invalid pipe-pod where init container does not mount shared volume", podYaml: ` apiVersion: v1 kind: Pod