ansible-collections / community.zabbix

Zabbix Ansible modules
http://galaxy.ansible.com/community/zabbix
Other
321 stars 280 forks source link

clarify and test backward compatibility with httpapi #558

Closed rockaut closed 1 year ago

rockaut commented 2 years ago
SUMMARY

Clarification on how backward compat for httpapi to zabbix-api can be implemented and guaranteed over the next releases.

See also #557

Current idea plan:

1.) implement a connection switch in ZapiWrapper With module._socket_path it's possible to check if httpapi got initialized in any form. This way its possible to even use httpapi if the ansible provided options are not set - we just use the "old" zabbix-api related module options to set the httpapi options. Then the _zapi property of the wrapper is an instance of ZabbixApiRequest (httpapi) instead of ZabbixAPI. As I designed ZabbixApiRequest to be an drop in replacement for ZabbixApi no further changes SHOULD be needed on module side.

2.) implement an new common option connection_type to all modules Possible values: auto, zabbix-api, httpapi Default value: auto

3.) Timed handling of connection_type and fading out of zabbix-api 3.1) For now auto will default to zabbix-api and as that in ZabbixAPI usage. Manuall switching of connection_type: httpapi will enable every enduser to test the implementation and optimizations. Given all works out like expected in point 1. no other changes have to be made on user side for now. This way we can declare the new connection as an experimental feature. I would say we promote this through Ansible Bullhorn at some point in time then. 3.2) After all doc changes and testing implementation we will introduce deprecation warnings for the zabbix-api module options alongside a major version bump and highlighting it an release docs while still keeping the default on zabbix-api. connection_type: auto will still prefer zabbix-api. This way enduser can update to newest version without changes needed. They can transition over to new configs (which should be automagically be recognized by ZapiWrapper) but can still run their old configs alongside. We also introduce deprecation warnings on module side - so calling a module without httpapi will bring up a deprecation notice but it still works. 3.3) After SOME (insert specific duration here (-; ) time/releases we again make a major version bump and switch connection_type: auto to prefer httpapi. Fallback to zabbix-api is only possible with MANUALL change to zabbix-api. Deprecation notices should be stricter/more alerting somehow. 3.4) Again after a major bump we then remove zabbix-api completely. I would say we keep connection_type alive to maybe handle future changes the same way.

ISSUE TYPE
COMPONENT NAME
BGmot commented 2 years ago

I currently can't successfully run mentioned in https://github.com/ansible-collections/community.zabbix/pull/444 code. Silly question how do I provide these variables if I run ansbile-test integration?

---
# hosts.yml

all:
  children:
    api:
      hosts:
        apilocal:
          ansible_host: host.docker.internal                            # the host to connect to ( f.e.: zabbix.mydomain.lan )
          ansible_httpapi_port: 80
          ansible_httpapi_use_ssl: false
          ansible_httpapi_validate_certs: false
          ansible_network_os: community.zabbix.jsonrpc
          ansible_connection: community.zabbix.httpapi
          ansible_user: Admin
          ansible_httpapi_pass: zabbix                                    # this should be a vault secret of course or ...
          # ansible_httpapi_session_key: slaksdljasdlfj           # ... instead of password use a session key (zabbix token)
D3DeFi commented 2 years ago

As a workaround, try to set them in the https://github.com/ansible-collections/community.zabbix/blob/main/tests/integration/targets/setup_zabbix/defaults/main.yml

This test roles is included from every other role. I am not sure if this is a good permanent place though.

BGmot commented 2 years ago

Yes, thanks, already tried that and it worked. I am just curious (and did not find anything meaningful on the Internet) how ansible-test "builds" the inventory and against what host (some "testhost"???) it is running all these roles (tests). Drastic change with what we have now is with httpai it will use "ansible_host" as a host to connect to to perform API requests while with zabbix-api we just specify server_url.

D3DeFi commented 2 years ago

I would guess it doesn't build anything and rather defaults to localhost as usually. ansible-test can be locked inside of a container or run on a host (which we are doing). Found out some --list-targets option in here https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html#testing-integration, but have no way to check atm.

BGmot commented 2 years ago

Spent some time learning how connection plugins work and I mus say it's very cool stuff. I have a question about introducing connection_type variable as proposed above. Why don't we implement backward compatibility following way: If a user defines

    ansible_network_os: community.zabbix.zabbix
    ansible_connection: httpapi

Then httpapi code is used by all our modules. Additionally in this case a user can define API token which will be used instead of username/password:

    ansible_httpapi_session_key:
      auth: 2920b1179da3041e23e60e38b421cd95582fc6b8ae06763e4979580d0fa7c8be

If ansible_network_os and ansible_connection are not defined (connection is local) then zabbix-api is in use. In my opinion this simple approach makes it very easy for users to test new approach. Then with time we can totally remove zabbix-api if needed.

rockaut commented 2 years ago

Did you have a look in my implementation? I basically already had it but doesn't have time to finalize it properly with test and so on.

The problem isn't really the httpapi implementation itself but to replace all the pyzabbix wrappers and keeping all back compatible.

https://github.com/rockaut/community.zabbix/tree/httpapi1

BGmot commented 2 years ago

Yes of course I am working off your implementation. I am setting _zapi property to an instance of ZabbixApiRequest instead of ZabbixAPI. Some modules just work with this with no changes some modules need to be adapted - this is the next step I'd be working on when we decide how to proceed. I'll take a look at your httpapi1 branch as I was working off master (it already has httpapi) -(((

rockaut commented 2 years ago

Cool. Very cool 😎

D3DeFi commented 2 years ago

Spent some time learning how connection plugins work and I mus say it's very cool stuff.

I have a question about introducing connection_type variable as proposed above. Why don't we implement backward compatibility following way:

If a user defines


    ansible_network_os: community.zabbix.zabbix

    ansible_connection: httpapi

Then httpapi code is used by all our modules. Additionally in this case a user can define API token which will be used instead of username/password:


    ansible_httpapi_session_key:

      auth: 2920b1179da3041e23e60e38b421cd95582fc6b8ae06763e4979580d0fa7c8be

If ansible_network_os and ansible_connection are not defined (connection is local) then zabbix-api is in use.

In my opinion this simple approach makes it very easy for users to test new approach. Then with time we can totally remove zabbix-api if needed.

Releasing 2.0.0 is a big milestone for us. Something like that I guess will not be repeated so soon in the future. We may lock ourselves to zabbix-api for a longer period then we think.

Maybe we can word it that zabbix-api is still possible opt-in, but will not be supported and is unmaintained by us. In such case we can also leave a legacy Base classes there. Depends, how much code we are talking about? Calls like zbx.host.get() will not be changed, right?

BGmot commented 2 years ago

Maybe we can word it that zabbix-api is still possible opt-in, but will not be supported and is unmaintained by us. In such case we can also leave a legacy Base classes there. Depends, how much code we are talking about? Calls like zbx.host.get() will not be changed, right?

Yes we are not trying to move away from zabbix-api (at least now) we just want to introduce new connection method with full playbooks code backward compatibility. I.e. I am trying to achieve this: if you run your playbook without any changes with our collection 2.0.0 it will still require zabbix-api. If you add this to your inventory:

ansible_network_os: community.zabbix.zabbix
ansible_connection: httpapi

Then httpapi will be used (with possibility to provide token. Is that what we want?

BGmot commented 2 years ago

I need an advise on what we should do with documentation and hard requirements for these all modules parameters server_url, login_user, login_password, they are not needed for httpapi. I propose to remove them from documentation and all modules integration tests but still accept them making them 'optional' this way all current playbooks (that have these parameters) will run with both zabbix-api and httpapi.

D3DeFi commented 2 years ago

Maybe we can word it that zabbix-api is still possible opt-in, but will not be supported and is unmaintained by us. In such case we can also leave a legacy Base classes there. Depends, how much code we are talking about? Calls like zbx.host.get() will not be changed, right?

Yes we are not trying to move away from zabbix-api (at least now) we just want to introduce new connection method with full playbooks code backward compatibility. I.e. I am trying to achieve this: if you run your playbook without any changes with our collection 2.0.0 it will still require zabbix-api. If you add this to your inventory:

ansible_network_os: community.zabbix.zabbix
ansible_connection: httpapi

Then httpapi will be used (with possibility to provide token. Is that what we want?

yes this sounds correct

D3DeFi commented 2 years ago

I need an advise on what we should do with documentation and hard requirements for these all modules parameters server_url, login_user, login_password, they are not needed for httpapi. I propose to remove them from documentation and all modules integration tests but still accept them making them 'optional' this way all current playbooks (that have these parameters) will run with both zabbix-api and httpapi.

change to required=False and adjust documentation for them that they are required if zabbix-api is present. We should ideally implement a check that controls this whenever modules decide to use zabbix-api and not httpapi.

D3DeFi commented 2 years ago

I need an advise on what we should do with documentation and hard requirements for these all modules parameters server_url, login_user, login_password, they are not needed for httpapi. I propose to remove them from documentation and all modules integration tests but still accept them making them 'optional' this way all current playbooks (that have these parameters) will run with both zabbix-api and httpapi.

change to required=False and adjust documentation for them that they are required if zabbix-api is present. We should ideally implement a check that controls this whenever modules decide to use zabbix-api and not httpapi.

If we want to still support zabbix-api, we should have some tests in place I think.

BGmot commented 2 years ago

Ok, I reached the point where everything works as expected and ansible-test integration -v --color --diff --python 3.9 --requirements --coverage runs with no errors in my local dev environment however github fails with very weird message

fatal: [testhost]: FAILED! => {"msg": "the connection plugin 'httpapi' was not found"}

https://github.com/BGmot/community.zabbix/runs/8293665394?check_suite_focus=true

If somebody could take a look I'd appreciate that. (My branch is here https://github.com/BGmot/community.zabbix/tree/bgmot_httpapi did not push it yet to upstream) Meanwhile I will try to dig as well.

BGmot commented 2 years ago

Ok we depend on ansible.netcommon collection which in turn depends on ansible.utils. I can copy ansible.netcommon code into our collection not sure whether it is right, I actually got rid of the one created by @rockaut which was almost identical to the one from ansible.netcommon. This is where I am not strong so please advise.

BGmot commented 2 years ago

Added ansibl.netcommon installation to test suite https://github.com/BGmot/community.zabbix/commit/9ef854d0784150c83fc9551fc707eb02e7d47acd but surprisingly it did not change anything.

schapmanCE commented 1 year ago

Just so I'm following, is the assumption then that moving to httpapi you're running plays against the target zabbix host? verses running against localhost and setting server_url to point to the zabbix host? I ask because I see you're deprecating server_url but there doesn't seem to be an replacement/alternative in the docs

BGmot commented 1 year ago

yes, httpapi is a "connection" plugin so works exactly as for example "ssh" one does - connects to the host from inventory (the play is running against) unless you use delegate_to or re-define ansible_host.

leroy0211 commented 1 year ago

I have been using the community.zabbix.zabbix_host_info module in my plays to determine if the current host is available in zabbix-server. If not it would add the host using the community.zabbix.zabbix_host

It seems that community.zabbix.zabbix_host_info's module is rewritten to another purpose, because I can not for the life of me use that module anymore to fetch host information.

This isn't a Backwards Compatible module, but functionally a different module (using the same name) without letting people know up front to use something else.