19:02:14 <sdoran> #startmeeting Ansible Core meeting https://github.com/ansible/community/issues/500
19:02:14 <zodbot> Meeting started Tue Nov  5 19:02:14 2019 UTC.
19:02:14 <zodbot> This meeting is logged and archived in a public location.
19:02:14 <zodbot> The chair is sdoran. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:02:14 <zodbot> Useful Commands: #action #agreed #halp #info #idea #link #topic.
19:02:14 <zodbot> The meeting name has been set to 'ansible_core_meeting_https://github.com/ansible/community/issues/500'
19:02:21 <sdoran> #chair sivel
19:02:21 <zodbot> Current chairs: sdoran sivel
19:02:45 <felixfontein> hi!
19:03:07 <drybjed> hello
19:03:43 <sdoran> sivel are we good on the `TaskExecutor` unsafe loop behavior change?
19:04:00 <sivel> yeah, I checked it off, there is a PR in review that will merge soon
19:04:06 <sdoran> 👍
19:04:10 <sivel> I got feedback ahead of waiting for this meeting
19:04:38 <bcoca> gave pr quick look, lgtm
19:04:56 <bcoca> i really hate we have custom calling of lokups in with_ that way
19:05:02 <sdoran> #topic Concensus regarding a custom params option in LDAP modules
19:05:23 <sdoran> https://github.com/ansible/community/issues/495#issuecomment-549267833
19:05:23 <drybjed> oh hey, my thing! so, any LDAP users around?
19:05:27 * cyberpear holds tight onto `with_` loops
19:05:32 <sdoran> drybjed: You have the floor.
19:05:46 <bcoca> params -10k ... same reasons as we did this in jenkins plugin
19:05:47 <felixfontein> I'm using the ldap modules (only since recentyly though)
19:06:32 <bcoca> its a cve waiting to happen allow option to override other options
19:06:47 <cyberpear> isn't this similar to the task-level `args` option?
19:07:06 <felixfontein> cyberpear: that one is way more safe than this one
19:07:14 <bcoca> no, since those still follow the same processing, a 'params' option bypasses it all, we had this problem with jenkins plugin already
19:07:19 <cyberpear> can that one be used instead of this?
19:07:25 <drybjed> yes, I would think so, but modules have to handle the 'args' specifically right?
19:07:33 <bcoca> args is fine, since it doesnt bypass anything
19:07:48 <bcoca> args is handled by controller and module just gets 'normal options
19:08:10 <cyberpear> `args` is nothing special to the module... priority is `module_defaults`, then regular module args, then `args` keyword (lowest to highest)
19:08:32 <bcoca> cyberpear: but not to the module .. those are ALL handled at controller
19:08:38 <bcoca> module only ever sees 'option=value'
19:08:47 <cyberpear> right... sounds like the proposed params wants to run on the managed node
19:08:49 <felixfontein> is there any reason for `params` when `args` can be used? (except if you want to destroy stuff :) )
19:08:57 <drybjed> in any case, I would like to have both ldap_entry and ldap_attrs modules use the same interface for parameters, so they would probably have to be modified together
19:09:27 <felixfontein> I guess `params` of `ldap_entry` should be deprecated then, and `ldap_attrs` should never get that option
19:09:36 <bcoca> agreed, this is not a problem with ldap_attrs trying to keep same interface, its a bug in ldap_entry that this exists in the first place
19:09:47 <drybjed> no, if args are processed on the controller that's fine for me, all I want is to be able to specify connection options via inventory variables, without modifying the role/playbook
19:10:04 <sdoran> Yeah, this should be removed from `ldap_entry` ASAP.
19:10:31 * bcoca really needs to implement tidbits/auth_plugins/domains
19:10:33 <felixfontein> I guess there's a four Ansible version deprecation loop before it can be removed? or can it be removed faster than that?
19:10:36 <drybjed> so if from user's point of view switching to 'args' is the same as 'params' currently, that's fine by me
19:11:10 <bcoca> not for security issues, if this params really allows overriding 'no_log' flagged options and then bypasses no_log itself ... its security issue
19:11:23 <sivel> I believe in the case of the jenkins module, we just ripped it out
19:11:29 <sivel> no deprecation
19:11:36 <felixfontein> bcoca: I think it can't do that, since no_log is processed long before module.params['params'] is merged into module.params
19:12:30 <sivel> fwiw, ldap_entry is "preview" status, which means it doesn't need to maintain a stable interface between releases
19:12:54 <bcoca> https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/net_tools/ldap/ldap_entry.py#L221
19:12:58 <felixfontein> most preview modules still try to stick to the four version deprecation cycle
19:13:05 <bcoca> ^ yeah, THAT should have never made it iin
19:13:12 <sdoran> A module parameter that wants to contain complex data should use proper suboptions.
19:13:16 <felixfontein> but instant change (for 2.10) is fine for me. or do you already want to change 2.8.x and 2.9.x as well?
19:13:18 <bcoca> again, not for security issues, that allows you to disclose passwords
19:13:23 <sivel> just saying it doesn't have to, and we would not enforce a deprecation
19:13:49 <sdoran> It's really concerning how it accepts data that modifies existing module parameters on the task. That's just crazy.
19:14:38 <bcoca> params: {'bind_pw': secret } <= bind_pw is no longer bound by no_log
19:14:53 <sdoran> So the problem is two-fold: 1) A parameter that accepts arbitrary data and 2) The module uses that arbitrary data to override module parameters for the task. 🤯
19:14:55 <bcoca> we already rejected this EXACT same thing in jenkins module
19:15:08 <sivel> I think both modules might have the same author
19:15:11 <bcoca> 1) is not normally a problem, 2) is ALWAYS a problem
19:15:24 <sdoran> sivel: I was thinking that but wasn't going to say it. ;)
19:15:40 <felixfontein> I've never used or looked at the jenkins module
19:15:42 <bcoca> sigh .. probably .. i should have checked all his modules after the jenkins issue
19:16:27 <sdoran> I think we have reached a consensus: this is bad, don't do that.
19:16:28 <cyberpear> (how will we enforce good practices when all these move to collections?)
19:16:34 <sdoran> drybjed: Do you need any further input?
19:16:50 <sivel> they were written by the same author fwiw
19:16:52 <drybjed> so what would be the plan then? rip out params from ldap_entry, implement args, and then the same can be done in ldap_attrs before it gets merged?
19:17:01 <felixfontein> I can create a PR which removes that option for ldap_entry
19:17:10 <cyberpear> drybjed: no need to implement `args`, it's already bulit-in to ansible
19:17:14 <felixfontein> drybjed: there's no need to implement `args`, that's alreay handled by ansible
19:17:16 <bcoca> cyberpear CVEs will then be per collection ...
19:17:30 <drybjed> cyberpear: oh, I see. I'll have to try that out then, good to know
19:17:39 <drybjed> i thought it was just for command/shell modules
19:17:42 <bcoca> drybjed: args is generic and does not need implementation
19:17:54 <bcoca> its a 'task keyword'
19:18:29 <drybjed> so it should work now, ok. felixfontein I'll wait for your PR to make the necessary changes in ldap_attrs
19:18:48 <drybjed> thanks everybody for covering that issue :)
19:18:51 <bcoca> `        'bind_pw': dict(default='', no_log=True),`  .. not sure if that wont make any empty string censored
19:19:11 <felixfontein> drybjed: you can just remove `params`, that's everything that needs to be done for ldap_attrs (except maybe adding an example using `args`) ;)
19:19:22 <bcoca> drybjed: thanks for you patience, and finding the issue ... had not realized we still had this in core
19:19:41 <drybjed> bcoca: who knows where else it lurks... :)
19:19:52 <drybjed> felixfontein: ok
19:20:17 * bcoca checks all modules with same author
19:20:48 <sdoran> #action `params` to be removed from `ldap_entry` module by felixfontein
19:21:12 <sdoran> #topic Open Floor
19:21:31 <bcoca> sdoran ,  felixfontein we might need to handle this as CVE .. since it is a disclosure issue
19:21:36 <felixfontein> I guess I'll also remove it from `ldap_attr` then, even though this module will get deprecated in drybjed's PR
19:21:57 <bcoca> yep, remove from all occurrences and probably backport 3 versions
19:22:00 <felixfontein> bcoca: sdoran: I'll CC you on the PR, feel free to do whatever you want with it
19:22:09 <sdoran> shipit
19:22:25 <drybjed> felixfontein: can I also get a CC to track it?
19:22:40 <felixfontein> drybjed: sure!
19:22:41 <bcoca> felixfontein: dont make PR yet, if cve, we try to keep fixes hidden till they pass tests
19:22:58 * bcoca checked yum_repository and no 'params' ...
19:23:05 <felixfontein> bcoca: ok. I'll create a patchset then. what should I do then? (I guess not push it to GH...)
19:23:31 <bcoca> pm to sdoran, we'll see who handles this or even if it rises up to cve
19:23:45 <bcoca> gotta get all kinds of people involved
19:23:53 <felixfontein> ok, thanks!
19:24:48 <cyberpear> #topic trivial filters for jinja compatibility: https://github.com/ansible/ansible/pull/64342
19:25:33 <cyberpear> s/filters/tests/
19:26:02 <sdoran> #topic trivial tests for Jinja compatibility: https://github.com/ansible/ansible/pull/64342
19:28:00 <felixfontein> I like that, even though it only affected me when writing integration tests so far :)
19:28:50 <bcoca> at this point i think we might need to just vendor the 'latest jinja2' to keep things from being more insane
19:29:07 <bcoca> i dont like masking built ins
19:29:09 <sdoran> I think we looked at this in triage this morning.
19:29:16 <bcoca> also not sure how this fits in collections
19:29:21 <agaffney> copying tests/filters is a slippery slope
19:29:27 <bcoca> sdoran: yeah, i gave you a whole 10min speach you could not hear
19:29:34 <sdoran> 😁
19:29:39 <bcoca> agaffney: we are 1/2 down the slope
19:29:44 <cyberpear> ansible 2.8 assumes this is present, as it provide "contains" test that is explicitly "opposite of build-in 'in' test"
19:30:02 <cyberpear> thankfully, these are pretty trivial
19:30:22 <sivel> per the jinja devs, no one should be using anything less than 2.10 at this point :D
19:30:24 <bcoca> but i dont have a good solution other than 'version detecting' and injecting these conditionally then
19:30:30 <agaffney> do we also copy map/select/selectattr from jinja 2.7? what happens when the upstream version gets more functionality?
19:30:35 <bcoca> sivel: i'll go tell rhel6 users
19:30:43 <cyberpear> https://github.com/ansible/ansible/pull/45798
19:30:58 <bcoca> agaffney: we end up with issues like the unique filter
19:31:02 <cyberpear> agaffney: were those missing in jinja 2.7?
19:31:04 <bcoca> why i think its a bad slope to continue on
19:31:05 * cyberpear uses them all the time
19:31:06 <sivel> > Given that relatively few users are using <2.10 now, I think it's ok to assume people have this now
19:31:07 <bcoca> cyberpear: yes
19:31:21 <sivel> Response to an old docs issue I had open for jinja
19:31:22 <agaffney> cyberpear: those were new in jinja 2.7
19:31:31 <cyberpear> okay, that's the version RHEL 7 ships
19:31:52 <cyberpear> "relatively few users" excludes all RHEL 7 users, which I'd assume is a big user base... (But I have no numbers here)
19:31:52 <bcoca> that it appears in new stuff does not mean it is suported in existing stuff
19:32:09 <agaffney> EL6 had jinja 2.6, and only via EPEL
19:32:12 <bcoca> the 'saving grace' is that templating happens on controller, which is normally on the newer side
19:32:35 <bcoca> and that the old stuff, is generally the deployed servers
19:33:23 <agaffney> at a certain point, when is the shipped version of a dependent library on a random distro "not our problem"?
19:33:37 <bcoca> id be more inclined to accept thi  if wrapped in `if jinja2.version < 2.10:`
19:33:49 * cyberpear wonders if RH would just ship jinja 2.10 on RHEL 7
19:33:53 <sivel> agaffney: arbitrarily decided on a per case basis ;D
19:33:59 <cyberpear> bcoca: I'm happy to add that
19:34:00 <agaffney> ansible itself doesn't require any specific jinja version or any if these tests/filters
19:34:03 <bcoca> agaffney: that is what we normally respond with 'upgrade jinja2 to get that' .. but it is a bad user experience
19:35:23 <bcoca> cyberpear: as for what RH/ubuntu/htfbad does .. we should still mostly work the same, that is why im inclined to accept these, but im really remiss since it is kind of patching jinaj2 and as agaffney stated ... what happens when they change X in future?
19:35:53 <bcoca> that is why making it version dependant seems better, then any variation is mostly due to jinja2 itself making a change  ... see unique PR for similar fun
19:36:01 <agaffney> vendoring jinja seems a better path than copying a bunch of tests/filters, but that comes with its own dragons (which are mostly the same dragons)
19:36:04 <cyberpear> the "dict is dict" change did  (just now) make its way into the jinja-2.7 on RHEL 7, so maybe there's hope, but then there's all the other distros...
19:39:15 <bcoca> and people running RHEL5 ...
19:39:57 <cyberpear> RHEL 5 only works w/ ansible-2.3, so probably not our problem...
19:40:12 <sdoran> So is there any consensus on this PR?
19:40:12 <bcoca> i wish ...
19:40:21 <sdoran> Seems like it'd be ok to add but wrapped in a version test.
19:40:25 <bcoca> +1 IF it is jinja2 version dependent
19:40:48 <cyberpear> #action cyberpear to add version wrapping to jinja compat tests
19:40:53 * bcoca makes note for collection migration
19:40:56 <agaffney> yeah, only provide if jinja itself doesn't provide them
19:41:59 <felixfontein> +1 if it has a jinja2 version test
19:42:01 <cyberpear> I like it... it auto removes itself when the user has newer jinja
19:42:20 <sdoran> #agreed PR #64342 is ok provided it is only used if needed based on inspecting the Jinja version
19:43:15 <sdoran> #action cyberpear to add version wrapping to jinja compat tests
19:43:19 <sdoran> #topic open floor
19:45:38 <sdoran> I will end the meeting in one minute if there is nothing further to discuss.
19:45:45 * sdoran really needs to go eat lunch
19:47:04 <felixfontein> enjoy!
19:47:35 <sdoran> Thank you everyone for participating today.
19:47:38 <sdoran> #endmeeting