Add tls parameter to redis module

[jcharlytown]

SUMMARY

Add tls parameter to redis module

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

module redis

ADDITIONAL INFORMATION

None

[ansibullbot]

cc @slok
click here for bot help

[felixfontein]

Thanks for your contribution!

I wonder whether it wouldn't be better to directly use the redis module_utils and docs_fragment; that would add a few more features to this module.

The main reason against that is right now that this would add another dependency (certifi) to this module. But if the module_utils is changed so that certifi is only required when tls=true, this would be ok, since so far this module didn't support TLS at all.

But if we add the tls option manually, this module will behave different from the other redis modules, and it won't be possible to adjust the behvaior (and add the dependency) without a breaking change.

So going that route would be much better.

[jcharlytown]

@felixfontein , what you're saying is that it cannot/will not be merged in this form?

I did not follow this argument:

But if we add the tls option manually, this module will behave different from the other redis modules, and it won't be possible to adjust the behvaior (and add the dependency) without a breaking change.

The option I added is the same as for the other redis modules - except the default. Is there a way to use module_utils but overwrite the default? Otherwise this would also be a breaking change as well, correct?

[felixfontein]

@felixfontein , what you're saying is that it cannot/will not be merged in this form?

I didn't say that.

I did not follow this argument:

But if we add the tls option manually, this module will behave different from the other redis modules, and it won't be possible to adjust the behvaior (and add the dependency) without a breaking change.

The option I added is the same as for the other redis modules - except the default. Is there a way to use module_utils but overwrite the default? Otherwise this would also be a breaking change as well, correct?

Yes, you can modify redis_auth_argument_spec as follows:

def redis_auth_argument_spec(tls_default = True):
    ...
        tls=dict(type='bool',
                 default=tls_default),
    ...

Then you can use redis_auth_argument_spec(tls_default=False) in the module.

You should also modify fail_imports to

def fail_imports(module, needs_certifi=True):
    ...
    if not HAS_CERTIFI_PACKAGE and needs_certifi:
    ...

Then you can use fail_imports(module, needs_certifi=module.params['tls']) instead of fail_imports().

[ansibullbot]

cc @paginabianca
click here for bot help

[jcharlytown]

@felixfontein , I upgraded the PR as outlined in your comment. I have tested both redis-data and redis modules.

However, I am confident regarding the doc fragments I have created. Is there a way to parametrize doc fragments? If not, I think other options would be to duplicate the doc fragment, either into another fragment or directly into the module. Opinions?

[felixfontein]

The commit e205260 undid way too much. You need to keep the changes to the documentation in plugins/modules/database/misc/redis.py.

[jcharlytown]

@felixfontein , honestly, I do not know which direction you want me to take this. The doc fragment gives a conflicting default value for the TLS option. From my understand, without removing this option from the fragment, I won't be able to use it for the redis module. I see the following options:

1. Do not use the fragment for redis module, duplicating the documentation (what is currently implemented)
2. Remove the TLS option from the fragment, reusing the fragment for all modules, adding the TLS option explicitly to all modules
3. Factoring out the TLS option into separate fragments, avoiding duplication (what was implemented previously - I didn't like it either though)

Out of your comments I am unable to infer what you would llike to see. Also, I am totally fine with you adding a commit yourself to rectify what ever is wrong in your opinion.

[felixfontein]

The correct way is: use the original fragment, and overwrite this specific value from it in the module(s) where you need a different value.

[felixfontein]

If nobody complains, I'll merge by the end of this week.

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/9a74ace1e47816c82f145883daa1e140cff17ee6/pr-4207

Backported as #4343

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@jcharlytown thanks for your contribution!