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

feat: add nginx upstream server module #65

Merged
merged 19 commits into from
Jun 20, 2024

Conversation

atammy-narmi
Copy link
Contributor

Relates: #64

@atammy-narmi
Copy link
Contributor Author

friendly ping

@ansibleguy
Copy link
Owner

Sorry for the delay - got pretty sick.

  1. plugins/module_utils/main/nginx_upstream_server.py
    As the description is already set to required in the modules/nginx_upstream_server.py this additional check is not needed. You may be able to remove the whole check method as it will fall-back to the default module behavior.

  2. plugins/modules/nginx_upstream_server.py
    Please add aliases=['name'] to the description parameter as this is a basic convention of ansible-modules.

  3. tests/nginx_upstream_server.yml
    Remove the ipsec_psk tests.
    If not done yet - try to run the tests on your OPNSense box as seen in the developer docs

@ansibleguy ansibleguy self-assigned this Apr 24, 2024
@ansibleguy ansibleguy added the enhancement New feature or request label Apr 24, 2024
@atammy-narmi
Copy link
Contributor Author

Ooh, sorry to hear that. Hope you're better now.
Thanks for the review, will do the updates this week.

@ansibleguy
Copy link
Owner

Thanks. I hope it is understandable how to run the tests.
You could also skip the tests, but I like them as they make the maintenance easier

@ansibleguy
Copy link
Owner

@atammy-narmi Please contact me if there is something unclear to you.

@atammy-narmi
Copy link
Contributor Author

Sorry, irl work has been more pressing so this was on backburner. will try to work some more soon.

@atammy-narmi
Copy link
Contributor Author

OK, I've added documentation and testing and the nginx_general module. I'm still unable to get everything to work correctly.

@ansibleguy
Copy link
Owner

Sorry, irl work has been more pressing so this was on backburner. will try to work some more soon.

Understand that. I too currently am pretty held-up in some projects

@ansibleguy
Copy link
Owner

ansibleguy commented Jun 20, 2024

The nginx_general error was only a test-issue. The module seems to work (:

My debug workflow:

  • Disable the failed_when of the failing task
  • Add debug: true to the failing task (is a valid parameter for all of the modules of this collection - shows you some details like performed API calls)
  • Run the tests: bash scripts/test_single.sh "$TEST_FIREWALL" "$TEST_API_KEY" $(pwd) nginx_general 1
  • Analyse the debug output and fix the task or module ;b

@ansibleguy
Copy link
Owner

ansibleguy commented Jun 20, 2024

The service actions like restart and reload seem to be supported by the API: https://docs.opnsense.org/development/api/plugins/nginx.html#id3

You can see those API calls using the developer console:
image

This API can be manually triggered using the service module

But if you set debug: true on a single task - you can see that the reconfigure call is done automatically IF some config has changed:

- name: Enabling - should work
  ansibleguy.opnsense.nginx_general:
    enabled: true
    debug: true
[WARNING]: REQUEST: GET | URL: https://172.17.1.52/api/nginx/settings/get
[WARNING]: RESPONSE: '{'status_code': 200, '_request': <Request('GET', 'https://172.17.1.52/api/nginx/settings/get')>, '_num_bytes_downloaded': 840, '_elapsed': datetime.timedelta(microseconds=270299), '_content': b'{"nginx":{"gene
ral":{"enabled":"0","ban_ttl":"0"},"webgui":{"limitnetworks":"0"},"http":{"workerprocesses":"1","workerconnections":"1024","sendfile":"0","keepalive_timeout":"60","reset_timedout":"0","default_type":"","server_names_hash_bucket_siz
e":"","server_names_hash_max_size":"","ban_response":{"403":{"value":"403 Forbidden","selected":1},"444":{"value":"444 Terminate Connection","selected":0}},"headers_more_enable":""},"userlist":[],"credential":[],"upstream":[],"upst
ream_server":[],"location":[],"custom_policy":[],"naxsi_rule":[],"http_server":[],"stream_server":[],"sni_hostname_upstream_map":[],"sni_hostname_upstream_map_item":[],"ip_acl":[],"ip_acl_item":[],"http_rewrite":[],"security_header
":[],"limit_zone":[],"errorpage":[],"tls_fingerprint":[],"limit_request_connection":[],"ban":[],"cache_path":[],"syslog_target":[]}}'}'
[WARNING]: Field changed: 'enabled' 'False' != 'True'
[WARNING]: {'before': {'uuid': None, 'enabled': False, 'ban_ttl': 0}, 'after': {'uuid': None, 'enabled': True, 'ban_ttl': 0}}
[WARNING]: REQUEST: POST | URL: https://<IP>/api/nginx/settings/set | HEADERS: '{'Content-Type': 'application/json'}' | DATA: '{"nginx": {"general": {"enabled": 1, "ban_ttl": 0}}}'
[WARNING]: RESPONSE: '{'status_code': 200, '_request': <Request('POST', 'https://<IP>/api/nginx/settings/set')>, '_num_bytes_downloaded': 18, '_elapsed': datetime.timedelta(microseconds=127178), '_content':
b'{"result":"saved"}'}'
[WARNING]: REQUEST: POST | URL: https://<IP>/api/nginx/service/reconfigure | HEADERS: '{}'
[WARNING]: RESPONSE: '{'status_code': 200, '_request': <Request('POST', 'https://<IP>/api/nginx/service/reconfigure')>, '_num_bytes_downloaded': 15, '_elapsed': datetime.timedelta(microseconds=379274), '_content':
b'{"status":"ok"}'}'

This API call is related to these module variables:

API_CONT_REL = 'service'
API_CMD_REL = 'reconfigure'
# https://<IP>/api/nginx/<API_CONT_REL>/<API_CMD_REL>

@ansibleguy
Copy link
Owner

BTW: Don't worry @superstes is my default user - just forgot to change the git config (;

Copy link
Owner

@ansibleguy ansibleguy left a comment

Choose a reason for hiding this comment

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

lgtm


def check(self) -> None:
if self.p['state'] == 'present':
if is_unset(self.p['description']):
Copy link
Owner

Choose a reason for hiding this comment

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

@atammy-narmi As the description is already set to required in the modules/nginx_upstream_server.py this additional check is not needed.


def run_module():
module_args = dict(
description=dict(type='str', required=True),
Copy link
Owner

Choose a reason for hiding this comment

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

@atammy-narmi Please add aliases=['name'] to the description parameter as this is a basic convention of ansible-modules.

@ansibleguy ansibleguy merged commit 944dc71 into ansibleguy:latest Jun 20, 2024
2 checks passed
@atammy-narmi
Copy link
Contributor Author

Great, thanks a bunch. That will help a lot with testing!

@atammy-narmi atammy-narmi deleted the nginx/upstream_server branch June 20, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants