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

Add k8s_exec module to execute command through the API #55029

Closed
wants to merge 2 commits into from

Conversation

TristanCacqueray
Copy link
Contributor

SUMMARY

This change adds a new module to execute command in a container through the API:
https://docs.okd.io/latest/dev_guide/executing_remote_commands.html#protocol

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

k8s_exec

ADDITIONAL INFORMATION

Test playbook:

- hosts: localhost
  gather_facts: no
  vars:
    pod: example-sf-zuul-scheduler-84bf645594-kw9lr
    namespace: myproject
  tasks:
    - name: Test Exec
      k8s_exec:
        pod: "{{ pod }}"
        namespace: "{{ namespace }}"
        command: zuul --version

    - name: Test Stderr
      k8s_exec:
        pod: "{{ pod }}"
        namespace: "{{ namespace }}"
        command: sh -c "echo toto > /dev/stderr"

Results in:

$ ansible-playbook -v test-exec.yaml
PLAY [localhost] *************************************************************

TASK [Test Exec] *************************************************************
changed: [localhost] => {
  "changed": true, "stderr": "", "stderr_lines": [],
  "stdout": "Zuul version: 3.7.2.dev37\n",
  "stdout_lines": ["Zuul version: 3.7.2.dev37"]
}

TASK [Test Stderr] ***********************************************************
changed: [localhost] => {
  "changed": true, "stderr": "toto\n", "stderr_lines": ["toto"],
  "stdout": "", "stdout_lines": []
}

PLAY RECAP *******************************************************************
localhost                  : ok=2    changed=2    unreachable=0    failed=0

@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 clustering Clustering category community_review In order to be merged, this PR must follow the community review workflow. k8s module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Apr 9, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ImportError: No module named kubernetes.client.apis

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ImportError: No module named kubernetes.client.apis

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ImportError: No module named 'kubernetes'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ModuleNotFoundError: No module named 'kubernetes'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ModuleNotFoundError: No module named 'kubernetes'

The test ansible-test sanity --test import --python 3.8 [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:69:0: ModuleNotFoundError: No module named 'kubernetes'

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: module should not be executable

The test ansible-test sanity --test validate-modules [explain] failed with 24 errors:

lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got 'Tristan de Cacqueray'
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E307 version_added should be '2.8'. Currently '2.9'
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'api' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'api_key' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'api_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'ca_cert' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'cert_file' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'client_cert' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'client_key' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'command' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'context' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'host' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'key_file' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'kubeconfig' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'name' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'namespace' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'password' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'pod' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'ssl_ca_cert' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'username' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'validate_certs' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'verify_ssl' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E322 Argument 'version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E324 Argument 'api_version' in argument_spec defines default as ('v1') but documentation defines default as (None)

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 9, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

The test ansible-test sanity --test shebang [explain] failed with 1 error:

lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: module should not be executable

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E305 DOCUMENTATION.author: Invalid author for dictionary value @ data['author']. Got 'Tristan de Cacqueray'
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: E307 version_added should be '2.8'. Currently '2.9'

click here for bot help

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 9, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

@erjohnso @fabianvf @flaper87 @kubevirt @supertom

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 9, 2019
@fabianvf
Copy link
Contributor

fabianvf commented Apr 9, 2019

This is really awesome. Out of curiousity, did you consider instead writing a k8s connection plugin (similar to the kubectl connection plugin but based on python instead of shelling out)? I think generally Ansible prefers to use connection plugins when executing against another host, rather than having modules that open a connection to another host and execute a command.

This is pretty clean and simple though so I don't have any personal opposition to it, but I'm curious to hear @willthames more expert opinion.

https://docs.ansible.com/ansible/latest/plugins/connection/kubectl.html
https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/connection/kubectl.py

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 9, 2019
@TristanCacqueray
Copy link
Contributor Author

I haven't consider a connection plugin because that exec api is rather limited and it will be hard to use it with generic task like shell or command. Moreover in the context of the k8s ansible operator, it would be easier to use such task instead of add_host followed by delegated action.

Another improvement to consider is to use a lookup('k8s', kind='Pod') object as a k8s_exec argument.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 17, 2019
@fabianvf
Copy link
Contributor

Makes sense to me

@fabianvf
Copy link
Contributor

shipit

@fabianvf
Copy link
Contributor

bot_status

@ansibot
Copy link
Contributor

ansibot commented May 31, 2019

Components

lib/ansible/modules/clustering/k8s/k8s_exec.py
support: community
maintainers: chouseknecht fabianvf flaper87 maxamillion willthames

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): fabianvf
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@TristanCacqueray
Copy link
Contributor Author

hum, the failed job doesn't seem related (Failed to create vault token for this job.)

@geerlingguy
Copy link
Contributor

The last commits fixed the issue for me in my standalone test playbook. I'm checking my operator's RBAC and it seems like for the pod in question (which it's trying to exec on), it should have access:

- apiGroups:
  - tower.ansible.com
  resources:
  - '*'
  verbs:
  - '*'

But it is probably not related to this module's code, so the module itself gets the green light from me. Test failure looks unrelated and probably just needs a re-run.

@TristanCacqueray
Copy link
Contributor Author

@geerlingguy thank you for the feedback. About your RBAC, iiuc the pods exec api is in the v1 apiGroups, or the rule could use: apiGroups: [""].

@geerlingguy
Copy link
Contributor

@TristanCacqueray ah, thanks. Digging further, I found I had to change my rule for pods to be like so:

- apiGroups:
  - ""
  resources:
  - pods/exec
  verbs:
  - create
  - get

Found in this comment: kubernetes-client/python#690 (comment)

Strangely, though, I'm now getting the following error from stream.py, which doesn't make sense to me:

File \"/usr/lib/python2.7/site-packages/kubernetes/stream/ws_client.py\", line 255, in websocket_call
    raise ApiException(status=0, reason=str(e))
kubernetes.client.rest.ApiException: (0)
Reason: Handshake status 200 OK

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 20, 2019
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jan 9, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 9, 2020

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: doc-required-mismatch: Argument 'command' in argument_spec is not required, but is documented as being required
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: doc-required-mismatch: Argument 'namespace' in argument_spec is not required, but is documented as being required
lib/ansible/modules/clustering/k8s/k8s_exec.py:0:0: doc-required-mismatch: Argument 'pod' in argument_spec is not required, but is documented as being required

click here for bot help

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 9, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 17, 2020
@TristanCacqueray
Copy link
Contributor Author

Something is broken indeed, I now also get kubernetes.client.rest.ApiException: (0) when using this module within an operator running on okd.

@fabianvf
Copy link
Contributor

@TristanCacqueray I think that's an issue with the proxy that the Ansible Operator creates, it seems for whatever reason it is unable to handle the exec connections made by the Python client, although it doesn't seem to have trouble with the exec implementation that kubectl uses. We still haven't figured out what the cause might be but I think it lies in the Python client or the proxy, not in your module

@TristanCacqueray
Copy link
Contributor Author

@fabianvf that would make sense. IIRC the kubectl clients uses the legacy spdy protocol while the python client uses the new websocket protocol. Would it be possible to shortcut the operator proxy for that module?

@geerlingguy
Copy link
Contributor

@TristanCacqueray - I can confirm that it's an issue with the Operator proxy as @fabianvf states; see operator-framework/operator-sdk#2204 for the downstream bug report.

@geerlingguy
Copy link
Contributor

@TristanCacqueray - The Kubernetes-related modules and plugins have been moved to a new community.kubernetes collection, where further development and module additions (like this one) will go. Would you be willing/able to create a new PR adding this module to that project?

Collection repo: https://github.com/ansible-collections/kubernetes

As I've already been testing this module thoroughly related to my work on some operators (it is extremely useful for many situations, and will be for more once that operator-sdk proxying issue is resolved, I for one am +1 to shipping it quickly once submitted!

@TristanCacqueray
Copy link
Contributor Author

@geerlingguy sure thing, here is the initial draft: ansible-collections/community.kubernetes#14

TristanCacqueray added a commit to TristanCacqueray/kubernetes that referenced this pull request Feb 6, 2020
TristanCacqueray added a commit to TristanCacqueray/kubernetes that referenced this pull request Feb 12, 2020
TristanCacqueray added a commit to TristanCacqueray/kubernetes that referenced this pull request Feb 12, 2020
TristanCacqueray added a commit to TristanCacqueray/kubernetes that referenced this pull request Feb 14, 2020
@TristanCacqueray
Copy link
Contributor Author

This now lives in ansible-collections/community.kubernetes#14

@ansible ansible locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 ci_verified Changes made in this PR are causing tests to fail. clustering Clustering category has_issue k8s module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants