community.zabbix icon indicating copy to clipboard operation
community.zabbix copied to clipboard

clarify and test backward compatibility with httpapi

Open rockaut opened this issue 2 years ago • 17 comments

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
  • Enhancement
COMPONENT NAME
  • connection.httpapi
  • httpapi.jsonrpc

rockaut avatar Dec 08 '21 11:12 rockaut

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)

BGmot avatar Aug 25 '22 15:08 BGmot

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.

D3DeFi avatar Aug 26 '22 08:08 D3DeFi

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.

BGmot avatar Aug 26 '22 12:08 BGmot

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.

D3DeFi avatar Aug 26 '22 12:08 D3DeFi

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.

BGmot avatar Aug 29 '22 12:08 BGmot

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

rockaut avatar Aug 29 '22 15:08 rockaut

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) -(((

BGmot avatar Aug 29 '22 15:08 BGmot

Cool. Very cool 😎

rockaut avatar Aug 29 '22 20:08 rockaut

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?

D3DeFi avatar Aug 31 '22 14:08 D3DeFi

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 avatar Aug 31 '22 14:08 BGmot

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.

BGmot avatar Sep 01 '22 12:09 BGmot

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 avatar Sep 06 '22 08:09 D3DeFi

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 avatar Sep 06 '22 08:09 D3DeFi

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.

D3DeFi avatar Sep 06 '22 08:09 D3DeFi

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 avatar Sep 11 '22 22:09 BGmot

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 avatar Sep 11 '22 22:09 BGmot

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

BGmot avatar Sep 12 '22 13:09 BGmot

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

schapmanCE avatar May 04 '23 19:05 schapmanCE

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.

BGmot avatar May 05 '23 01:05 BGmot

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.

leroy0211 avatar May 26 '23 15:05 leroy0211