Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions changelog/fragments/arbitraryArgs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
entries:
- description: >
Added the "--ansible-args" command-line flag that allows users to specify arbitrary
CLI arguments for ansible-based operators that are passed through ansible-runner.
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
For example, passing --ansible-vault as an arbitrary argument allows the user to store
sensitive data in encrypted files.

kind: "addition"

breaking: false
6 changes: 6 additions & 0 deletions internal/ansible/flags/flag.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Flags struct {
MetricsAddress string
LeaderElectionID string
LeaderElectionNamespace string
AnsibleArgs string
}

const AnsibleRolesPathEnvVar = "ANSIBLE_ROLES_PATH"
Expand Down Expand Up @@ -99,4 +100,9 @@ func (f *Flags) AddTo(flagSet *pflag.FlagSet) {
" holding the leader lock (required if running locally with leader"+
" election enabled).",
)
flagSet.StringVar(&f.AnsibleArgs,
"ansible-args",
"",
"Ansible args. Allows user to specify arbitrary arguments for ansible-based operators.",
)
}
14 changes: 14 additions & 0 deletions internal/ansible/runner/internal/inputdir/inputdir.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type InputDir struct {
Parameters map[string]interface{}
EnvVars map[string]string
Settings map[string]string
CmdLine string
}

// makeDirs creates the required directory structure.
Expand Down Expand Up @@ -131,6 +132,19 @@ func (i *InputDir) Write() error {
return err
}

// Trimming off the first and last characters if the command is wrapped by single quotations
if strings.HasPrefix(i.CmdLine, string("'")) && i.CmdLine[0] == i.CmdLine[len(i.CmdLine)-1] {
i.CmdLine = i.CmdLine[1 : len(i.CmdLine)-1]
}

camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
cmdLineBytes := []byte(i.CmdLine)
if len(cmdLineBytes) > 0 {
err = i.addFile("env/cmdline", cmdLineBytes)
if err != nil {
return err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @VenkatRamaraju,

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

// 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;

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep .. I just saw that.


// ANSIBLE_INVENTORY takes precedence over our generated hosts file
// so if the envvar is set we don't bother making it, we just copy
// the inventory into our runner directory
Expand Down
13 changes: 8 additions & 5 deletions internal/ansible/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,17 @@ func roleCmdFunc(path string) cmdFuncType {
// check the verbosity since the exec.Command will fail if an arg as "" or " " be informed
if verbosity > 0 {
return exec.Command("ansible-runner", ansibleVerbosityString(verbosity), "--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)
}
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)
}
}

// New - creates a Runner from a Watch struct
func New(watch watches.Watch) (Runner, error) {
func New(watch watches.Watch, runnerArgs string) (Runner, error) {
var path string
var cmdFunc, finalizerCmdFunc cmdFuncType

Expand Down Expand Up @@ -139,6 +139,7 @@ func New(watch watches.Watch) (Runner, error) {
GVK: watch.GroupVersionKind,
maxRunnerArtifacts: watch.MaxRunnerArtifacts,
ansibleVerbosity: watch.AnsibleVerbosity,
ansibleArgs: runnerArgs,
snakeCaseParameters: watch.SnakeCaseParameters,
}, nil
}
Expand All @@ -154,6 +155,7 @@ type runner struct {
maxRunnerArtifacts int
ansibleVerbosity int
snakeCaseParameters bool
ansibleArgs string
}

func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig string) (RunResult, error) {
Expand Down Expand Up @@ -188,6 +190,7 @@ func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig stri
"runner_http_url": receiver.SocketPath,
"runner_http_path": receiver.URLPath,
},
CmdLine: r.ansibleArgs,
}
// If Path is a dir, assume it is a role path. Otherwise assume it's a
// playbook path
Expand Down
2 changes: 1 addition & 1 deletion internal/ansible/runner/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func TestNew(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
testWatch := watches.New(tc.gvk, tc.role, tc.playbook, tc.vars, tc.finalizer)

testRunner, err := New(*testWatch)
testRunner, err := New(*testWatch, "")
if err != nil {
t.Fatalf("Error occurred unexpectedly: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion internal/cmd/ansible-operator/run/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func run(cmd *cobra.Command, f *flags.Flags) {
os.Exit(1)
}
for _, w := range watches {
runner, err := runner.New(w)
runner, err := runner.New(w, f.AnsibleArgs)
if err != nil {
log.Error(err, "Failed to create runner")
os.Exit(1)
Expand Down
4 changes: 3 additions & 1 deletion test/ansible/build/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,6 @@ USER root
RUN chmod -R ug+rwx /tmp/fixture_collection
USER 1001
RUN ansible-galaxy collection build /tmp/fixture_collection/ --output-path /tmp/fixture_collection/ \
&& ansible-galaxy collection install /tmp/fixture_collection/operator_sdk-test_fixtures-0.0.0.tar.gz
&& ansible-galaxy collection install /tmp/fixture_collection/operator_sdk-test_fixtures-0.0.0.tar.gz
RUN echo abc123 > /opt/ansible/pwd.yml \
&& ansible-vault encrypt_string --vault-password-file /opt/ansible/pwd.yml 'thisisatest' --name 'the_secret' > /opt/ansible/vars.yml
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 22 additions & 0 deletions test/ansible/deploy/crds/test.example.com_argstest_crd.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: argstests.test.example.com
spec:
group: test.example.com
names:
kind: ArgsTest
listKind: ArgsTestList
plural: argstests
singular: argstest
scope: Namespaced
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
type: object
x-kubernetes-preserve-unknown-fields: true
served: true
storage: true
subresources:
status: {}
26 changes: 26 additions & 0 deletions test/ansible/molecule/cluster/tasks/args_test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
- name: Create the test.example.com/v1alpha1.ArgsTest
k8s:
state: present
definition:
apiVersion: test.example.com/v1alpha1
kind: ArgsTest
metadata:
name: args-test
namespace: '{{ namespace }}'
spec:
field: value
wait: yes
wait_timeout: 300
wait_condition:
type: Running
reason: Successful
status: "True"
register: args_test

- name: Assert sentinel ConfigMap has been created for Molecule Test
assert:
that: cm.data.msg == "The decrypted value is thisisatest"
vars:
cm: "{{ q('k8s', api_version='v1', kind='ConfigMap', namespace=namespace,
resource_name='args-test').0 }}"
1 change: 1 addition & 0 deletions test/ansible/molecule/templates/operator.yaml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ spec:
port: 6789
initialDelaySeconds: 5
periodSeconds: 3
args: ["--ansible-args='--vault-password-file /opt/ansible/pwd.yml'"]
volumes:
- name: runner
emptyDir: {}
20 changes: 20 additions & 0 deletions test/ansible/playbooks/args.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
- hosts: localhost
gather_facts: no
collections:
- community.kubernetes
tasks:
- name: Get the decrypted message variable
include_vars:
file: /opt/ansible/vars.yml
name: the_secret
- name: Create configmap
k8s:
definition:
apiVersion: v1
kind: ConfigMap
metadata:
name: '{{ meta.name }}'
namespace: '{{ meta.namespace }}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

data:
msg: The decrypted value is {{the_secret.the_secret}}
7 changes: 7 additions & 0 deletions test/ansible/watches.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,10 @@
snakeCaseParameters: false
vars:
meta: '{{ ansible_operator_meta }}'

- version: v1alpha1
group: test.example.com
kind: ArgsTest
playbook: playbooks/args.yml
vars:
meta: '{{ ansible_operator_meta }}'
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,51 @@ spec:
served: true
storage: true
```

## Passing Arbitrary Arguments to Ansible

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:

```shell
ansible-operator run --ansible-args='--tags "configuration,packages"'
```
```
ansible-operator run --ansible-args='--skip-tags "notification"'
```
Ansible-runner will perform the task relevant to the command specified by the user in the ```---ansible-args``` flag.


## Using Ansible-Vault

[Ansible Vault][ansible-vault-doc] allows you to keep sensitive data such as passwords or keys in encrypted files, rather than as plaintext in playbooks or roles. You can specify Ansible-Vault file via an arbitrary argument by using the `--ansible-args` flag. For example, let's assume that a playbook reads in a file `vars.yml` which contains an encrypted text and stores it in a variable `secret`:

```
---
- name: Playbook to print debug messages
gather_facts: false
hosts: localhost
tasks:
- name: Get the decrypted message variable
include_vars:
file: vars.yml
name: secret
- debug:
msg: The decrypted value is {{secret.the_secret}}
```

Now, let's also assume that we have a password file, `pwd.yml`, that contains the password to decrypt the encrypted text. Then, by running the command `ansible-operator run --ansible-args='--vault-password-file pwd.yml'` the operator will read in the encrypted text from the file and perform decryption using the password stored in the `pwd.yml` file:

```
--------------------------- Ansible Task StdOut -------------------------------

TASK [debug] ********************************
ok: [localhost] => {
"msg": "The decrypted value is DECRYPTED-TEST-VALUE"
}

-------------------------------------------------------------------------------
```
[ansible-vault-doc]: https://docs.ansible.com/ansible/latest/user_guide/vault.html