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

[BUG] host module fails because REST API puts trailing slash now #41

Closed
elwood218 opened this issue May 12, 2022 · 16 comments · Fixed by #52
Closed

[BUG] host module fails because REST API puts trailing slash now #41

elwood218 opened this issue May 12, 2022 · 16 comments · Fixed by #52
Assignees
Labels
bug Something isn't working upstream There is something upstream blocking this

Comments

@elwood218
Copy link

elwood218 commented May 12, 2022

Please verify first that your feedback is not already reported here.
If possible and useful provide additional information below. This is optional however.

Provide your feedback
Beginning with Checkmk 2.1.0b6 the output of getting current_folder from REST API has changed. Now a trailing slash is added. So like in my case if the parameter folder had no trailing slash the condition if current_folder != module.params["folder"]: was failing after 2.1.0b5.
I added now a trailing slash and this could also be checked in the role I build but if I am correct then modules like "sychronize" and more also checking that.

Component Name

host

Ansible Version

$ ansible --version
ansible [core 2.12.5]

Collection Version

$ ansible-galaxy collection list
tribe29.checkmk               0.2.0 (also tested with 0.2.2)

Environment

Checkmk Version: 2.1.0b5 (WORKING - but maybe that would fail if folder parameter got an trailing slash)
Checkmk Version: 2.1.0b6, 2.1.0b7, 2.1.0b8 (FAILING - fixed by putting a trailing slash into folder variable)

Screenshots

Additional context

@elwood218 elwood218 added the feedback General feedback label May 12, 2022
@robin-checkmk
Copy link
Member

@elwood218 thanks for reporting this issue.
For us to be able to reproduce this, please fill in all the requested information as good as possible.
Thanks!

@elwood218
Copy link
Author

elwood218 commented May 14, 2022

@robin-tribe29

Done....

@robin-checkmk
Copy link
Member

@elwood218 Thanks for the update!
However, I cannot reproduce your issue as you described it.
Can you provide a minimal working example, how your configuration looks like?

@robin-checkmk robin-checkmk added bug Something isn't working and removed feedback General feedback labels May 16, 2022
@robin-checkmk robin-checkmk changed the title [FEED] host module fails because REST API puts trailing slash now [BUG] host module fails because REST API puts trailing slash now May 16, 2022
@elwood218
Copy link
Author

- name: "Add/update/remove host"
  tribe29.checkmk.host:
    server_url: "https://{{ cmk_central }}/"
    site: "{{ cmk_central_site }}"
    automation_user: "{{ cmk_site_user }}"
    automation_secret: "{{ cmk_site_password }}"
    host_name: "{{ cmk_host_name | default(host_name) }}"
    attributes:
      "{{ cmk_host_attributes | default(omit) }}"
    folder: "{{ default_host_folder }}"
    state: "{{ cmk_host_state | default('present') }}"
  delegate_to: localhost
  become: no
  notify:
    - activate changes

Working:

- set_fact:
    default_host_folder: "/{{meta_project_platform | lower}}/{{meta_customer_pretty|upper|mandatory}}/{{meta_project_short|upper}}/{{site|upper}}/"

NOT Working: (DIFFERENCE IS THE TRAILING SLASH)

- set_fact:
    default_host_folder: "/{{meta_project_platform | lower}}/{{meta_customer_pretty|upper|mandatory}}/{{meta_project_short|upper}}/{{site|upper}}"

@elwood218
Copy link
Author

PS: The host must be already there - I guess only then you can see it @robin-tribe29

@robin-checkmk
Copy link
Member

Now I can reproduce the issue, thanks!
This looks like it is related to #42.

@lgetwan can you take a look please?

fatal: [test1.tld -> localhost]: FAILED! => {"changed": false, "msg": "Error calling API. HTTP code 400. Details: b'{\"title\": \"Bad Request\", \"status\": 400, \"detail\": \"These fields have problems: target_folder\", \"fields\": {\"target_folder\": [\"\\'\\\\/test\\\\/\\' does not match pattern \\'(?:(?:[~\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\/]|(?:[~\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\/][-_ a-zA-Z0-9.]+)+)|[0-9a-fA-F]{32})\\'.\"]}}', "}

@elwood218
Copy link
Author

I got an different error:

TASK [cmk_host_registration : Add/update/remove host] ****************************************************************
fatal: [HOST -> localhost]: FAILED! => changed=false
  msg: 'Error calling API. HTTP code 400. Details: b''{"title": "Invalid move action", "status": 400, "detail": "The host is already part of the specified target folder"}'', '

@elwood218
Copy link
Author

@robin-tribe29 I just found out that if the host is added first time the "create host" function (not the correct name of the function, just to name it) does NOT want the trailing slash.
But if the host is already there the "move host" function wants the trailing slash.

@robin-checkmk
Copy link
Member

Thanks for your continuing feedback!
I think the problem is understood generally, but we need to make the time to fix this.
We will update as soon, as there is something to report.
Thanks for your patience!

@elwood218
Copy link
Author

That's very pity..... I know that the module is not supported officially but at the moment that is the only way. Furthermore the module is only not working anymore because of your changes to the API - before it was working fine. It also doesn't make sense that in the same module the function for creating needs a trailing slash and the function for moving doesn't want a trailing slash - this makes the whole module unusable.
So then you should avoid putting new bugs all the time with fixing a beta release!

@robin-checkmk
Copy link
Member

I understand your frustration, I really do.
Just try to understand our end here too: We are doing this, when time allows. And I want to say, we are as transparent as possible about that. That is why we want to rely at least partially on the community, and we hope that some people step up and help to push this collection forward.
And one last point I need to make: As you said yourself: You are using a beta version of Checkmk.

@elwood218
Copy link
Author

@robin-tribe29 Could you please add this as workaround to the host module?

    # Handle the host accordingly to above findings and desired state
    if state == "present" and current_state == "present":
        headers["If-Match"] = etag
        msg_tokens = []

        folder_var = module.params["folder"] + '/'

        if current_folder != folder_var:
            move_host(module, base_url, headers)
            msg_tokens.append("Host was moved.")

There is only one line added folder_var = module.params["folder"] + '/' and in the next line there is only the var changed after !=.
This would at least make the module working again until it is fixed correctly.

@lgetwan
Copy link
Contributor

lgetwan commented May 25, 2022

Hi @elwood218,
I implemented a quickfix, but slightly different as suggested:

if state == "present" and current_state == "present":
    headers["If-Match"] = etag
    msg_tokens = []

    if current_folder.endswith("/"):
        current_folder = current_folder.rstrip("/")

    if current_folder != module.params["folder"]:
        move_host(module, base_url, headers)
        msg_tokens.append("Host was moved.")

That way, it will be a workaround for the "trailing slash problem" and still work with CMK 2.0.0.
I have tested that with CMK 2.1.0b9 and 2.1.0p24 and pushed it into the devel branch.

Please check if that works for you.

lgetwan added a commit that referenced this issue May 25, 2022
@elwood218
Copy link
Author

@lgetwan I have tested it with your devel branch and Checkmk 2.1.0 stable and it seems to work now. Really thank you! :)

@lgetwan
Copy link
Contributor

lgetwan commented Jun 30, 2022

Since 2.1.0p4, the trailing slash bug was fixed.

@robin-checkmk robin-checkmk added the upstream There is something upstream blocking this label Jun 30, 2022
@elwood218
Copy link
Author

Well but now it isn't working anymore with the Ansible module...
And if you finally saw that it was a problem in the API you can give us back the credit you have put on our bug opening to this problem...

robin-checkmk pushed a commit that referenced this issue May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working upstream There is something upstream blocking this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants