Repository: openfaas/faas-cli
Pull request #831 (2020-09-30 13:37:31)

Add CI env check to change dir and file permissions of artifcats
Signed-off-by: Utsav Anand <utsavanand2@gmail.com>

<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
This PR will enable users of faas-cli, especially the containerised version to have more permissive rules for artefacts created by the CLI to be shared across multiple users, which is typically the case in a CI/CD environment.

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Setting the env var CI to true or 1 will make the artefacts created by `faas-cli login` and `faas-cli build` to have 0777 permissions, with the files being created in the current directory, this will make sure the build and login artefacts can be shared across the host OS and user and the container in environments like GitHub Actions where only some host directories like the working dir are mounted to the container. But other environments like Google Cloud Build will benefit from the same.

- [x] I have raised an [issue](#830) to propose this change ([required](https://github.com/openfaas/faas/blob/master/CONTRIBUTING.md))

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
A running sample of the containerised binary is running and building OpenFaaS function here:
https://github.com/utsavanand2/faasd-hello/runs/1177059289?check_suite_focus=true
while maintaining the simplicity of the workflow action yaml
```yaml
name: CI/CD

on:
  push:
    branches: 
      - master
      - readable-workflow
jobs:
  func-build:
    runs-on: ubuntu-latest
    steps:
      - 
        name: Checkout
        uses: actions/checkout@v2
      -
        name: Pull template
        uses: docker://utsavanand2/faas-cli:latest-root
        with:
          args: template store pull golang-http
      - 
        name: Run shrinkwrap build
        uses: docker://utsavanand2/faas-cli:latest-root
        with:
          args: build -f faasd-hello.yml --shrinkwrap
      -
        name: Login to OpenFaaS Gateway
        uses: docker://utsavanand2/faas-cli:latest-root
        with:
          args: login -p ${{ secrets.OPENFAAS_GATEWAY_PASSWD }} -g ${{ secrets.OPENFAAS_GATEWAY }}
      -
        name: Set up QEMU
        uses: docker/setup-qemu-action@v1
      -
        name: Set up Docker Buildx
        uses: docker/setup-buildx-action@v1
      -
        name: Login to DockerHub
        if: success()
        uses: docker/login-action@v1
        with:
          username: ${{ secrets.DOCKER_USERNAME }}
          password: ${{ secrets.DOCKER_PASSWORD }}
      -
        name: List dir contents
        run: |
          ls -lah
      - 
        name: Build and Push the OpenFaaS function
        uses: docker/build-push-action@v2
        with:
          context: ./build/faasd-hello/
          file: ./build/faasd-hello/Dockerfile
          push: true
          platforms: linux/amd64,linux/arm64,linux/arm/v7
          tags: utsavanand2/faasd-hello:latest
      - 
        name: Deploy the function
        uses: docker://utsavanand2/faas-cli:latest-root
        with:
          args: deploy -f faasd-hello.yml
```
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the [CONTRIBUTION](https://github.com/openfaas/faas/blob/master/CONTRIBUTING.md) guide
- [x] I have signed-off my commits with `git commit -s`
- [ ] I have added tests to cover my changes.
- [x] All new and existing tests passed.


utsavanand2: Understood. Adding an additional global var with a doc string makes a lot
more sense.

On Sat, Oct 3, 2020, 8:51 PM Lucas Roesler <notifications@github.com> wrote:

> *@LucasRoesler* commented on this pull request.
> ------------------------------
>
> In config/config_file.go
> <https://github.com/openfaas/faas-cli/pull/831#discussion_r499156659>:
>
> > @@ -60,13 +73,14 @@ func New(filePath string) (*ConfigFile, error) {
>
>  // EnsureFile creates the root dir and config file
>  func EnsureFile() (string, error) {
> +	setDefaults()
>
> @utsavanand2 <https://github.com/utsavanand2> this is still mutating a
> global constant. From a practical perspective in the CLI, this is probably
> *ok* here, but not a general pattern we would want to use. Something like
> this simply references the global value when needed, but doesn't modify it
>
> permissions := DefaultPermissionsif IsRunningInCI() {
>     permissions = DefaultCIPermissions
> }// ...if err := os.MkdirAll(filepath.Dir(filePath), permissions); err != nil {// ...
> }
>
> additionally, having a value for defaultCIPermissions means you can add a
> doc string explaining why we need that specific value in CI
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/openfaas/faas-cli/pull/831#pullrequestreview-501553345>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AGAYDRNSBD5HDBVMXUN5243SI46PXANCNFSM4R7DLSBQ>
> .
>

LucasRoesler: @utsavanand2 two more things

1. can you document with and include these links to explain why we chose and look at the `CI` environment variable
    * https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables
    * https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables
    * https://docs.travis-ci.com/user/environment-variables/#default-environment-variables
2. similarly we should document that we want these changes for the permissions because we _have_ to run as root per this documentation in the Github Actions https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/dockerfile-support-for-github-actions#user
LucasRoesler: I have one other question about how `faas-cli login` works with this flow. The change should make it possible to read the config from the local working directory, but it isn't clear to me how or if that is happening now
utsavanand2: > @utsavanand2 two more things
> 
> 1. can you document with and include these links to explain why we chose and look at the `CI` environment variable
>    
>    * https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables
>    * https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables
>    * https://docs.travis-ci.com/user/environment-variables/#default-environment-variables
> 2. similarly we should document that we want these changes for the permissions because we _have_ to run as root per this documentation in the Github Actions https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/dockerfile-support-for-github-actions#user

Sure!
utsavanand2: > I have one other question about how `faas-cli login` works with this flow. The change should make it possible to read the config from the local working directory, but it isn't clear to me how or if that is happening now

@LucasRoesler What is really happening right now, is when the env var `CI` is set to `true` or `1`, then in the `login` step of the action, we create the config file in the current dir as `./openfaas/config.yml` rather than `~/.openfaas/config.yml`. Since the current workspace dir is mounted, the artifact i.e. the config file is preserved.
When we're ready to deploy our function in the last step, then config file is again looked up for from `./openfaas/config.yml`. As the workspace dir is again mounted to the containers working dir, making the command successful.
Your question made me realise that we might actually not need to change the permissions of the `config.yml` file since we're accessing it again inside the faas-cli container itself after the workdir mount.
Though this is not true for the `build` dir since it needs to be accessed by the Github Actions user to read it when invoking `docker build` with the shrinkwrapped build.
Let me know if I'm incorrect.

@alexellis Do you think we should trim down the permission further for the build dir from 0777 to 0744? Is that going to break anything? I'll sure try it out to see if it breaks my CI build.
LucasRoesler: > > I have one other question about how `faas-cli login` works with this flow. The change should make it possible to read the config from the local working directory, but it isn't clear to me how or if that is happening now
> 
> @LucasRoesler What is really happening right now, is when the env var `CI` is set to `true` or `1`, then in the `login` step of the action, we create the config file in the current dir as `./openfaas/config.yml` rather than `~/.openfaas/config.yml`. Since the current workspace dir is mounted, the artifact i.e. the config file is preserved.
> When we're ready to deploy our function in the last step, then config file is again looked up for from `./openfaas/config.yml`. As the workspace dir is again mounted to the containers working dir, making the command successful.
> Your question made me realise that we might actually not need to change the permissions of the `config.yml` file since we're accessing it again inside the faas-cli container itself after the workdir mount.
> Though this is not true for the `build` dir since it needs to be accessed by the Github Actions user to read it when invoking `docker build` with the shrinkwrapped build.
> Let me know if I'm incorrect.

@utsavanand2 that is the flow i was expecting. When i reread the code diff and thought about it again, i was surprised not to see any changes in a login related file, i guess this whole flow is determined by the change in `fileExists`


utsavanand2: > @utsavanand2 that is the flow i was expecting. When i reread the code diff and thought about it again, i was surprised not to see any changes in a login related file, i guess this whole flow is determined by the change in `fileExists`

@LucasRoesler Yes I expected the same before approaching the code, but it's really well organised in the config.go file, as you said we're just concerned with `fileExists` and `EnsureFile`.

Patch:
diff --git a/README.md b/README.md
index 319f1e51..07e00d4b 100644
--- a/README.md
+++ b/README.md
@@ -252,6 +252,16 @@ Alternatively you can assign a secret to the node to allow it to pull from your
 
 For Docker Swarm use the `--send-registry-auth` flag or its shorthand `-a` which will look up your registry credentials in your local credentials store and then transmit them over the wire to the deploy command on the API Gateway. Make sure HTTPS/TLS is enabled before attempting this.
 
+### Use faas-cli in CI environments
+
+If you're running faas-cli in a CI environment like [Github Actions](https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables), [CircleCI](https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables), or [Travis](https://docs.travis-ci.com/user/environment-variables/#default-environment-variables), chances are you get the env var `CI` set to true.
+
+If the `CI` env var is set to `true` or `1`, faas-cli change the location of the OpenFaaS config from the default `~/.openfaas/config.yml` to `.openfaas/config.yml` with elevated permissions for the `config.yml` and the shrinkwrapped `build` dir (if there is one).
+
+This is really useful when running faas-cli as a container image. The recommended image type to use in a CI environment is the root variant, tagged with `-root` suffix.
+CI environments like Github Actions require you to use Docker images having a root user. Learn more about it [here](https://docs.github.com/en/free-pro-team@latest/actions/creating-actions/dockerfile-support-for-github-actions#user).
+
+
 ### Use a YAML stack file
 
 Read the [YAML reference guide in the OpenFaaS docs](https://docs.openfaas.com/reference/yaml/).
diff --git a/builder/build.go b/builder/build.go
index 89637558..3e670b69 100644
--- a/builder/build.go
+++ b/builder/build.go
@@ -161,7 +161,19 @@ type dockerBuild struct {
 	BuildLabelMap    map[string]string
 }
 
-const defaultHandlerFolder = "function"
+var defaultDirPermissions os.FileMode = 0700
+
+const defaultHandlerFolder string = "function"
+
+// isRunningInCI checks the ENV var CI and returns true if it's set to true or 1
+func isRunningInCI() bool {
+	if env, ok := os.LookupEnv("CI"); ok {
+		if env == "true" || env == "1" {
+			return true
+		}
+	}
+	return false
+}
 
 // createBuildContext creates temporary build folder to perform a Docker build with language template
 func createBuildContext(functionName string, handler string, language string, useFunction bool, handlerFolder string, copyExtraPaths []string) (string, error) {
@@ -186,7 +198,11 @@ func createBuildContext(functionName string, handler string, language string, us
 
 	fmt.Printf("Preparing: %s %s\n", handler+"/", functionPath)
 
-	mkdirErr := os.MkdirAll(functionPath, 0700)
+	if isRunningInCI() {
+		defaultDirPermissions = 0777
+	}
+
+	mkdirErr := os.MkdirAll(functionPath, defaultDirPermissions)
 	if mkdirErr != nil {
 		fmt.Printf("Error creating path: %s - %s.\n", functionPath, mkdirErr.Error())
 		return tempPath, mkdirErr
diff --git a/builder/copy.go b/builder/copy.go
index 28565a1a..bac034a0 100644
--- a/builder/copy.go
+++ b/builder/copy.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"path"
 	"path/filepath"
 )
 
@@ -60,6 +61,11 @@ func copyFile(src, dest string) error {
 		return fmt.Errorf("error reading src file stats: %s", err.Error())
 	}
 
+	err = ensureBaseDir(dest)
+	if err != nil {
+		return fmt.Errorf("error creating dest base directory: %s", err.Error())
+	}
+
 	f, err := os.Create(dest)
 	if err != nil {
 		return fmt.Errorf("error creating dest file: %s", err.Error())
@@ -84,6 +90,18 @@ func copyFile(src, dest string) error {
 	return nil
 }
 
+// ensureBaseDir creates the base directory of the given file path, if needed.
+// For example, if fpath is 'build/extras/dictionary.txt`, ensureBaseDir will
+// make sure that the directory `buid/extras/` is created.
+func ensureBaseDir(fpath string) error {
+	baseDir := path.Dir(fpath)
+	info, err := os.Stat(baseDir)
+	if err == nil && info.IsDir() {
+		return nil
+	}
+	return os.MkdirAll(baseDir, 0755)
+}
+
 func debugPrint(message string) {
 
 	if val, exists := os.LookupEnv("debug"); exists && (val == "1" || val == "true") {
diff --git a/config/config_file.go b/config/config_file.go
index 1584662c..0e1270ec 100644
--- a/config/config_file.go
+++ b/config/config_file.go
@@ -19,8 +19,16 @@ import (
 )
 
 var (
-	DefaultDir  = "~/.openfaas"
-	DefaultFile = "config.yml"
+	DefaultDir         string      = "~/.openfaas"
+	DefaultFile        string      = "config.yml"
+	DefaultPermissions os.FileMode = 0700
+
+	// DefaultCIDir creates the 'openfaas' directory in the current directory
+	// if running in a CI environment.
+	DefaultCIDir string = ".openfaas"
+	// DefaultCIPermissions creates the config file with elevated permissions
+	// for it to be read by multiple users when running in a CI environment.
+	DefaultCIPermissions os.FileMode = 0744
 )
 
 //AuthType auth type
@@ -58,15 +66,31 @@ func New(filePath string) (*ConfigFile, error) {
 	return conf, nil
 }
 
+// isRunningInCI checks the ENV var CI and returns true if it's set to true or 1
+func isRunningInCI() bool {
+	if env, ok := os.LookupEnv("CI"); ok {
+		if env == "true" || env == "1" {
+			return true
+		}
+	}
+	return false
+}
+
 // EnsureFile creates the root dir and config file
 func EnsureFile() (string, error) {
-	dirPath, err := homedir.Expand(DefaultDir)
+	permission := DefaultPermissions
+	dir := DefaultDir
+	if isRunningInCI() {
+		permission = DefaultCIPermissions
+		dir = DefaultCIDir
+	}
+	dirPath, err := homedir.Expand(dir)
 	if err != nil {
 		return "", err
 	}
 
 	filePath := path.Clean(filepath.Join(dirPath, DefaultFile))
-	if err := os.MkdirAll(filepath.Dir(filePath), 0700); err != nil {
+	if err := os.MkdirAll(filepath.Dir(filePath), permission); err != nil {
 		return "", err
 	}
 
@@ -83,7 +107,11 @@ func EnsureFile() (string, error) {
 
 // FileExists returns true if the config file is located at the default path
 func fileExists() bool {
-	dirPath, err := homedir.Expand(DefaultDir)
+	dir := DefaultDir
+	if isRunningInCI() {
+		dir = DefaultCIDir
+	}
+	dirPath, err := homedir.Expand(dir)
 	if err != nil {
 		return false
 	}


Test patch:
diff --git a/builder/copy_test.go b/builder/copy_test.go
index 58eee5ac..824f2b13 100644
--- a/builder/copy_test.go
+++ b/builder/copy_test.go
@@ -28,14 +28,55 @@ func Test_CopyFiles(t *testing.T) {
 		}
 		defer os.RemoveAll(destDir)
 
-		CopyFiles(srcDir, destDir+"/")
-		err := checkDestinationFiles(destDir, 2, mode)
+		err := CopyFiles(srcDir, destDir+"/")
+		if err != nil {
+			t.Fatalf("Unexpected copy error\n%v", err)
+		}
+
+		err = checkDestinationFiles(destDir, 2, mode)
 		if err != nil {
 			t.Fatalf("Destination file mode differs from source file mode\n%v", err)
 		}
 	}
 }
 
+func Test_CopyFiles_ToDestinationWithIntermediateFolder(t *testing.T) {
+	dir := os.TempDir()
+	data := []byte("open faas")
+
+	// create a folder for source files
+	srcDir, dirError := ioutil.TempDir(dir, "openfaas-test-source-")
+	if dirError != nil {
+		t.Fatalf("Error creating source folder\n%v", dirError)
+	}
+	defer os.RemoveAll(srcDir)
+
+	// create a file inside the created folder
+	mode := 0600
+	srcFile := fmt.Sprintf("%s/test-file-1", srcDir)
+	fileErr := ioutil.WriteFile(srcFile, data, os.FileMode(mode))
+	if fileErr != nil {
+		t.Fatalf("Error creating source file\n%v", dirError)
+	}
+
+	// create a destination folder to copy the files to
+	destDir, destDirErr := ioutil.TempDir(dir, "openfaas-test-destination-")
+	if destDirErr != nil {
+		t.Fatalf("Error creating destination folder\n%v", destDirErr)
+	}
+	defer os.RemoveAll(destDir)
+
+	err := CopyFiles(srcFile, destDir+"/intermediate/test-file-1")
+	if err != nil {
+		t.Fatalf("Unexpected copy error\n%v", err)
+	}
+
+	err = checkDestinationFiles(destDir+"/intermediate/", 1, mode)
+	if err != nil {
+		t.Fatalf("Destination file mode differs from source file mode\n%v", err)
+	}
+}
+
 func setupSourceFolder(numberOfFiles, mode int) (string, error) {
 	dir := os.TempDir()
 	data := []byte("open faas")