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

Fixes #32030, #32188 - process events with unknown hosts #55

Merged
merged 3 commits into from Jun 6, 2022

Conversation

ezr-ondrej
Copy link
Member

When ansible-runner executes a local task (through delegate_to: localhost or any other available mechanism in Ansible to do so), it
breaks the implicit assumption that all logs are related to a host
that we know about (with “we” being Foreman in general, and the
inputs["targets"] Dynflow structure in this particular case).

  • Guard against this possibility (and drop the log entry in this case).
  • Thwart silly Rubocop check - IMO "bite if angry" style with a
    composite "angry" clause (as in here) makes the code easy to misunderstand.
  • Broadcast messages for localhost
  • Raise an error for unknown other hosts

For events that don't have host, try to get the remote_addr that is
being reported for underlying communication events and guess hosts from
there.

When ansible-runner executes a local task (through `delegate_to:
localhost` or any other available mechanism in Ansible to do so), it
breaks the implicit assumption that all logs are related to a host
that we know about (with “we” being Foreman in general, and the
`inputs["targets"]` Dynflow structure in this particular case).

- Guard against this possibility (and drop the log entry in this case).
- Thwart silly Rubocop check - IMO "bite if angry" style with a
composite "angry" clause (as in here) makes the code easy to misunderstand.
- Broadcast messages for `localhost`
- Raise an error for unknown other hosts

For events that don't have host, try to get the `remote_addr` that is
being reported for underlying communication events and guess hosts from
there.
@ezr-ondrej
Copy link
Member Author

@adamruzicka this came off of my radar, but I belive the theforeman/foreman_ansible#398 (comment) is not entirely trua and this would be beneficial (try guess the host from the addr field).

Should this be warning if we don't find a host? Probably not.
Should this broadcast for localhost? Well yeah, but implications... so maybe also no

Happy to remove or change any part of it, but would love to get it in :)

@adamruzicka
Copy link
Contributor

If I have a task with delegate_to: localhost, event.event_data.host is still set to the correct hostname. What is the right incantation to break this?

2022-06-03T14:42:09  [D] [foreman_ansible] - handling event for host: "amber-bursey.addie-dulberg.example.com": {
  "uuid": "4939eba1-7d54-475d-924e-3ee7fa2f9577",
  "counter": 20,
  "stdout": "\u001b[0;33mchanged: [amber-bursey.addie-dulberg.example.com -> localhost]\u001b[0m",
  "start_line": 23,
  "end_line": 24,
  "runner_ident": "173914ec-e8f7-4b3b-a5b8-5792460ccc93",
  "event": "runner_on_ok",
  "pid": 388853,
  "created": "2022-06-03T12:42:07.951541",
  "parent_uuid": "84c5a6e6-749f-42fa-4455-000000000009",
  "event_data": {
    "playbook": "playbook.yml",
    "playbook_uuid": "a448247c-125a-4195-9ac3-0159e3cfa86d",
    "play": "all",
    "play_uuid": "84c5a6e6-749f-42fa-4455-000000000006",
    "play_pattern": "all",
    "task": "command",
    "task_uuid": "84c5a6e6-749f-42fa-4455-000000000009",
    "task_action": "command",
    "task_args": "",
    "task_path": "/home/aruzicka/.toolbox/f35/.foreman-ansible/d20220603-388334-1fafgaq/project/playbook.yml:5",
    "host": "amber-bursey.addie-dulberg.example.com",
    "remote_addr": "amber-bursey.addie-dulberg.example.com",
    "res": {
      "cmd": "ls -la",
      "stdout": "total 4\ndrwxr-xr-x 2 aruzicka users  26 Jun  3 14:42 .\ndrwx------ 6 aruzicka users  78 Jun  3 14:42 ..\n-rw-r--r-- 1 aruzicka users 101 Jun  3 14:42 playbook.yml",
      "stderr": "",
      "rc": 0,
      "start": "2022-06-03 14:42:07.914718",
      "end": "2022-06-03 14:42:07.923931",
      "delta": "0:00:00.009213",
      "changed": true,
      "invocation": {
        "module_args": {
          "_raw_params": "ls -la",
          "_uses_shell": true,
          "warn": true,
          "stdin_add_newline": true,
          "strip_empty_ends": true,
          "argv": null,
          "chdir": null,
          "executable": null,
          "creates": null,
          "removes": null,
          "stdin": null
        }
      },
      "stdout_lines": [
        "total 4",
        "drwxr-xr-x 2 aruzicka users  26 Jun  3 14:42 .",
        "drwx------ 6 aruzicka users  78 Jun  3 14:42 ..",
        "-rw-r--r-- 1 aruzicka users 101 Jun  3 14:42 playbook.yml"
      ],
      "stderr_lines": [

      ],
      "_ansible_no_log": false,
      "_ansible_delegated_vars": {
        "ansible_host": "localhost",
        "ansible_port": null,
        "ansible_user": null,
        "ansible_connection": "local"
      }
    },
    "start": "2022-06-03T12:42:07.687848",
    "end": "2022-06-03T12:42:07.951345",
    "duration": 0.263497,
    "event_loop": null,
    "uuid": "4939eba1-7d54-475d-924e-3ee7fa2f9577"
  }
}

@ezr-ondrej
Copy link
Member Author

If I have a task with delegate_to: localhost

Interesting 🤔 that's not what I've seen, happy to see.
But this change is mostly aimed to route the connection messages from ansible -vvv or -vvvv runs to correct host (there you get events for ssh coms and those are broadcasted ATM).

Co-authored-by: Ondřej Ezr <ezrik12@gmail.com>
@adamruzicka
Copy link
Contributor

This is what I had to use to break things

---
- hosts: all
  tasks:
    - add_host:
        name: "added.host"
        ansible_ssh_port: "10022"
        ansible_ssh_host: 127.0.0.1
        ansible_ssh_user: root
    - shell: ls -la /
      delegate_to: added.host
- hosts: added.host
  tasks:
    - shell: ls -la /

The add_host and the task with delegate_to worked fine even without this patch, but the second play broke it. With this patch it works.

Should this be warning if we don't find a host? Probably not.

I'd say maybe yes? It breaks our assumptions about what's going on and we should mention somewhere that anything that happens from that point onwards is in a grey area?

Should this broadcast for localhost? Well yeah, but implications... so maybe also no

An edge case, but what if I have a regular host in foreman called localhost?

@ezr-ondrej
Copy link
Member Author

An edge case, but what if I have a regular host in foreman called localhost?

then your whole infrustructure breaks I guess, but here it would only break if that host is in the current batch I think, becuase we try to look for it in the current batch only, not in the whole Foreman inventory.
And If you have host FQDN = localhost, then someone should tell you, you are doing it whole wrong, it should probably be foreman I'd take that, but probably not REX and not foreman_ansible IMHO ^_^

For what would happen tho: It would just sent the localhost runs to that host if it is in the batch 🤷

@adamruzicka
Copy link
Contributor

becuase we try to look for it in the current batch only, not in the whole Foreman inventory.

If I'm reading it right we don't check for it at all, events for localhost get caught by the guard and broadcasted silently. If those updates went to host called localhost if its in the batch ,then I would be fine with it.

And If you have host FQDN = localhost, then someone should tell you, you are doing it whole wrong

Touche :)

@ezr-ondrej
Copy link
Member Author

You're right, looks cleaner, just sqush on merge PLS :)

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Looks good, let's get this in

@adamruzicka adamruzicka merged commit 6398f95 into theforeman:master Jun 6, 2022
@adamruzicka
Copy link
Contributor

Thank you @ezr-ondrej !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants