Closes #2262: Allowing users to specify arbitrary CLI arguments for Ansible-based operators #3374

VenkatRamaraju · 2020-07-09T21:27:09Z

Description of the change:
Added a command-line flag for ansible operators to allow users to specify arbitrary CLI arguments that are passed through ansible-runner.

Motivation for the change:
Allows users to specify arbitrary CLI arguments for ansible-based operators.
Fixes issue #2262

Checklist

If the pull request includes user-facing changes, extra documentation is required:

[✓] Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

jmrodri

Very good. One minor nit on the changelog. And I have a questions about the affect an empty env/cmdline will have on things, if any.

jmrodri · 2020-07-09T21:53:01Z

changelog/fragments/arbitraryArgs.yaml

+      Added a command-line flag that allows users to specify arbitrary CLI arguments for ansible-based operators
+      that are passed through ansible-runner.


I would specify the actual flag you added. Something along the lines of "Added the --ansible-args command-line flag that ...."

jmrodri · 2020-07-09T21:56:36Z

pkg/ansible/runner/runner.go

+				fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath,
+				"--hosts", "localhost", "-i", ident, "run", inputDirPath)
 		}
 		return exec.Command("ansible-runner", "--rotate-artifacts",
-			fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath, "--hosts",
-			"localhost", "-i", ident, "run", inputDirPath)
+			fmt.Sprintf("%v", maxArtifacts), "--role", roleName, "--roles-path", rolePath,
+			"--hosts", "localhost", "-i", ident, "run", inputDirPath)
 	}


Thank you for wrapping this better.

jmrodri · 2020-07-09T21:59:43Z

pkg/ansible/runner/internal/inputdir/inputdir.go

+	err = i.addFile("env/cmdline", cmdLineBytes)
+	if err != nil {
+		return err
+	}



What if the i.CmdLine is empty? will having an empty env/cmdline affect things?

changelog/fragments/arbitraryArgs.yaml

test/ansible/watches.yaml

website/content/en/docs/ansible/reference/advanced_options.md

camilamacedo86

Just a few nits, otherwise show great 👍
Good work.

tengqm · 2020-07-13T05:59:48Z

website/content/en/docs/ansible/reference/advanced_options.md

@@ -212,12 +212,9 @@ spec:

 ## Passing Arbitrary Arguments to Ansible

-Users can specify an arbitrary command-line argument to the ```operator-sdk exec-entrypoint``` and ```run --local``` subcommands for Ansible-based Operators via the ```ansible-args``` flag.
+You are able to use the flag `--ansible-args` to pass an arbitrary argument to the Ansible-based Operator. With this option we can, for example, allow a playbook to run a specific part of the configuration without running the whole playbook:  


A second thought on this ... The so called --ansible-args are actually the arguments to be passed to ansible-runner rather than ansible or ansible-playbook. This means we can pass anything below using this new flag:

usage: ansible-runner [-h] [--version] [-p PLAYBOOK | -m MODULE | -r ROLE] [--debug] [--logfile LOGFILE] [-b BINARY] [-i IDENT] [--rotate-artifacts ROTATE_ARTIFACTS] [--artifact-dir ARTIFACT_DIR] [--project-dir PROJECT_DIR] [--inventory INVENTORY] [-j] [--omit-event-data] [--only-failed-event-data] [-q] [-v] [--limit LIMIT] [--cmdline CMDLINE] [--hosts HOSTS] [--forks FORKS] [--roles-path ROLES_PATH] [--role-vars ROLE_VARS] [--role-skip-facts] [-a MODULE_ARGS] [--process-isolation] [--process-isolation-executable PROCESS_ISOLATION_EXECUTABLE] [--process-isolation-path PROCESS_ISOLATION_PATH] [--process-isolation-hide-paths [PROCESS_ISOLATION_HIDE_PATHS [PROCESS_ISOLATION_HIDE_PATHS ...]]] [--process-isolation-show-paths [PROCESS_ISOLATION_SHOW_PATHS [PROCESS_ISOLATION_SHOW_PATHS ...]]] [--process-isolation-ro-paths [PROCESS_ISOLATION_RO_PATHS [PROCESS_ISOLATION_RO_PATHS ...]]] [--directory-isolation-base-path DIRECTORY_ISOLATION_BASE_PATH] [--resource-profiling] [--resource-profiling-base-cgroup RESOURCE_PROFILING_BASE_CGROUP] [--resource-profiling-cpu-poll-interval RESOURCE_PROFILING_CPU_POLL_INTERVAL] [--resource-profiling-memory-poll-interval RESOURCE_PROFILING_MEMORY_POLL_INTERVAL] [--resource-profiling-pid-poll-interval RESOURCE_PROFILING_PID_POLL_INTERVAL] [--resource-profiling-results-dir RESOURCE_PROFILING_RESULTS_DIR] COMMAND private_data_dir

It would be awkward if we later want to pass more arguments to ansible if we call this flag --ansible-args. Not quite sure will be exposing the --cmdline CMDLINE someday or even we do have such use cases. However, just for avoiding confusion, would it be better to call this as --runner-args?

These are actually arguments to ansible, not runner

They are written to the env/cmdline file and then ansible-runner passes them to the ansible command that it runs

Ah, didn't read the code carefully. That solves my concern. Thank you, @fabianvf .

changelog/fragments/arbitraryArgs.yaml

fabianvf · 2020-07-13T20:29:20Z

pkg/ansible/flags/flag.go

+		"ansible-args",
+		"",
+		strings.Join(append(helpTextPrefix,
+			"Ansible args."),


Add a more complete help text here

fabianvf · 2020-07-13T20:32:23Z

test/ansible/playbooks/args.yml

+          kind: ConfigMap
+          metadata:
+            name: '{{ meta.name }}'
+            namespace: '{{ meta.namespace }}'


This configmap needs a data field or it will just be empty

fabianvf · 2020-07-13T20:32:36Z

test/ansible/playbooks/args.yml

+          metadata:
+            name: '{{ meta.name }}'
+            namespace: '{{ meta.namespace }}'
+    - name: Get the decrypted message variable


This should happen before the configmap is created

fabianvf · 2020-07-13T20:35:06Z

test/ansible/playbooks/args.yml

+      include_vars:
+        file: /opt/ansible/vars.yml
+        name: the_secret
+    - data:


This belongs on the configmap, it's not a valid task by itself

fabianvf

/lgtm

internal/ansible/runner/internal/inputdir/inputdir.go

test/ansible/build/Dockerfile

camilamacedo86 · 2020-07-30T10:49:04Z

internal/ansible/runner/internal/inputdir/inputdir.go

+		if err != nil {
+			return err
+		}
+	}


Regards the addFile:

IMO I do not think that we need to add this func. See that we can replace that by:

err := ioutil.WriteFile(filepath.Join(i.Path, "path"), content, 0644) if err != nil { log.Error(err, "Unable to write file", "Path", fullPath) }

Also, the log.Error(err, "Unable to write file", "Path", fullPath) we can replace by:

log.Errorf("Unable to write file (%s) : (%s)", fullPath, err)

The 0644 needs to be replaced by the const in the projectutils. See: https://github.com/operator-framework/operator-sdk/blob/master/internal/util/projutil/project_util.go#L31

The reason here is because the addFile just do the above code so has no need for an extra func and then, it makes easier understand that it is about to write a file in the disk.

All the parameters above (Parameters, EnvVars, Settings) all use the addFile while having their bytes written to disk, so I thought we could use that function instead of repeating the code.

Hi @VenkatRamaraju,

Thank you for your reply. Let's see the addFile:

operator-sdk/internal/ansible/runner/internal/inputdir/inputdir.go

Lines 55 to 63 in 8303ef9

// addFile adds a file to the given relative path within the input directory.

func (i *InputDir) addFile(path string, content []byte) error {

fullPath := filepath.Join(i.Path, path)

err := ioutil.WriteFile(fullPath, content, 0644)

if err != nil {

log.Error(err, "Unable to write file", "Path", fullPath)

}

return err

}

Is not it the same of call the WriteFile?. So, why we need that?

err := ioutil.WriteFile(filepath.Join(i.Path, "path"), content, 0644) if err != nil { log.Errorf("Unable to write file (%s) : (%s)", fullPath, err) }

So my suggestion is to replace the addFiles calls to use ioutil.WriteFile directly.

And then, see that is better we use the FileMode const;

operator-sdk/internal/util/projutil/project_util.go

Line 31 in 8303ef9

FileMode = 0644

Feel free to ping me as well for we spoke about it.

@camilamacedo86 since this is just reusing the function the rest of the code is already using I think dropping the addFile function would make sense to do separately, I don't think it's worth blocking the PR to fix it here

Yep .. I just saw that.

camilamacedo86

a few nits for a better maintenance ability.

Changed runner args to just args Testing Changelog file Changes based on review Error checking Changes docs: arbitrary args + ansible-vault support Changes from review Changes Changes Changes

camilamacedo86 · 2020-07-30T22:17:00Z

cmd/ansible-operator/main.go

-<<<<<<< HEAD
 	root := cobra.Command{
 		Use: "ansible-operator",
-=======


rebase issue :)

camilamacedo86 · 2020-07-31T11:27:32Z

changelog/fragments/arbitraryArgs.yaml

+
+    kind: "addition"
+
+    breaking: false


nit space at the end.

fabianvf

/lgtm

camilamacedo86

/lgtm
Shows fine 👍

openshift-ci-robot requested review from camilamacedo86 and jmrodri July 9, 2020 21:27

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 9, 2020

jmrodri requested changes Jul 9, 2020

View reviewed changes

VenkatRamaraju requested a review from jmrodri July 10, 2020 18:00