ansible-community / community-topics

[Moved to Ansible Forum] Discussions for Ansible Community Meetings
https://docs.ansible.com/ansible/devel/community/steering/community_steering_committee.html#community-topics-triage
GNU General Public License v3.0
35 stars 9 forks source link

Collection requirements: expanding the section on best practices #33

Closed tadeboro closed 2 years ago

tadeboro commented 2 years ago

Summary

Right now, the development conventions section in collection requirements use general best practices as its base. And while those general rules do offer a good baseline, parts of the Ansible ecosystem developed their own set of best practices when developing Ansible content.

For example, resource modules have their own set of conventions that contradict the general best practices in certain places.

Maybe we should expand the best practices section and add some specialized subsections for things like network collections?

ptoal commented 2 years ago

I may be wrong but I've always looked at it as something like: _info modules perform Read operations and no other. A user should have complete confidence that running an _info module will never perform and Create, Update, Delete (CUD) operations. _info modules are for safely gathering information for the purpose of being used elsewhere (an assertion, another task, etc). A non-info module should be expected to make some CUD action unless run in check_mode and needs extra safety (like using check_mode) when testing or validating playbooks.

That is sort of to the point about what I'm getting at. The Network Resource modules have expanded the possible operations on a resource. When Ansible was started, I think the number of potential operations was limited to "read the current state" (_info) and "enforce the desired state". Over time, the number of operations has expanded, and the network resource modules expanded it even more for network-specific operations, for example: 'rendered' to generate the commands required to configure a network device to the desired state, without actually enforcing that state.

For me, the module paradigm makes more sense to be: "a module encapsulates the possible operations on an entity". Having separate _info, or _facts modules just for read-only operations feels counter-intuitive, even though I realize it is the established, and documented pattern.

ptoal commented 2 years ago

I may be wrong but I've always looked at it as something like: _info modules perform Read operations and no other. A user should have complete confidence that running an _info module will never perform and Create, Update, Delete (CUD) operations.

I don't think there's much gained, from a confidence perspective, that a _facts, or _info module only performs read-only operations, because that is ultimately a result of the access control mechanisms of the target resource. Ultimately, the credential used determines what a playbook can do, not the format of the modules. I think

 - foo:
     state: unchanged # (or whatever term is used for read-only inspection)
   register: foo_state

is just as intuitive as

  - foo_info:
    register: foo_state

Also, I know a whole bunch of Cisco IOS 'show' commands that can cause significant performance degradation of the host device. Theoretically, they are read-only operations, but they are far from safe to execute in production. Relying on the module naming paradigm to have confidence you're doing something safe is just false security.

All that said, I still don't have confidence in the "right" solution to this problem, because as felixfontein has pointed out, there have been conflicting implementations of the same functionality for years now. My feeling is to let both continue to exist, even though it causes a little confusion. We can address that confusion in the documentation, as this issue suggests. I look at this as evolution of the language, not tainting it.

trishnaguha commented 2 years ago

Since this issue has been "stuck" in limbo for two months and is blocking the inclusion/rejection of the trendmicro.deepsec Ansible Collection, I propose we answer the following question:

Do we want to accept resource modules (where resource modules are defined as in #33 (comment)) as one of the standard ways for structuring and writing Ansible modules that want to become part of the community package?

If the answer is yes, we can formally accept the definition of what the resource modules are and we are done.

If the answer is no, we should answer the following question:

Do we want to accept resource modules into the community package?

The answers to this question I came up with/stole from the meeting discussions are:

  1. Yes.
  2. Yes, but only for network modules where the pattern is already established.
  3. Yes, but only for network and security modules, because the pattern seems to fit the use cases well there.
  4. No.

At this point, we should know if resource modules can be part of the Ansible package or not. This should unblock the current inclusion decisions (and possibly open the discussion about what to do with existing resource modules that were grandfathered into the Ansible package).

@abadger @felixfontein @gundalow @Andersson007 @acozine @ssbarnea @jillr @cidrblock @jamescassell @thaumos Can you please comment on this at your earliest convenience? Thank you all in advance!

My vote is Yes Option 1. The Resource modules have been widely accepted by the users, customers, field, partners because of the value that it provides. The proposals, issues, PRs, blog posts related to RMs are all available in public since the beginning. Nothing was done in an isolated manner (If someone is not able to participate in IRC meetings because of TZ difference/other reasons - that should not be called out as "working in silo/isolation"). The Resource modules have been developed for years by the team of expert engineers, not to forget community/partner contributions around it. and they should continue to exist to support our users.

thaumos commented 2 years ago

I am for resource modules for being included.

Strategically, this is a path forward that we need to work towards.

I am not for these discussions that are binary. There is a lot of merit to this as a type. Let's have a better discussion around how to leverage this than make it a yes or no.

craig-br commented 2 years ago

Reasoning: I talk to hundreds of customers, community members, potential customers, network automators, system administrators, etc from a different skill-sets, different domains (e.g. heavy linux users, people who are NOT network engineers) and we have tons of folks asking us "can you do this for other use-cases besides network?" Literally the network use-case is becoming more mature than other use-cases because of resource modules. People want turn-key declarative models for configuring things because not everyone can spit out a 50 line jinja2 template from memory.

My vote: Option 1

Developers should be given the choice to follow the resource module pattern based on what users need.

I agree with @IPvSean . The feedback we receive from the field and customers is that the potential use-cases based on resource modules are suited to what they need, especially in the security space. Giving users a pre-defined, declarative model to use makes adoption easier, which is aligned with Ansible's principles.

https://github.com/ansible/ansible#design-principles Have an extremely simple setup process with a minimal learning curve.

timway commented 2 years ago

Just to chime in; I find the resource module approach to cover a wide basis - they seem like they provide a mechanism to develop functionality that is desired by the community. This is very visible in the network space as it was never really feasible to template out a configuration for an entire device because reliably applying it would require rebooting into the configuration. Not exactly desired most of the time nor is it like rebooting a service on a single box at a time.

Could it have been accomplished in a different pattern, possibly. I say keep it and if other areas find it a useful way to express things then so be it. Option 1 for me.

felixfontein commented 2 years ago

@gundalow @acozine @ssbarnea @jamescassell your votes are still missing in this discussion.

ssbarnea commented 2 years ago

I am also for Option 1. As a side note, this thread grew so much is hard to follow it.

Others remarked the problem of guidelines/practices not all being in a single place. I do agree that we should unify them. For example when I tell people to follow collection repository layout for all their ansible content, they often ask "why? I do not have a collection and we do not have plans to publish one". That is a serious issue and likely more people don't understand that the standard layout is not only for collection. Until we refactor our documentations to clarify these things, we will still face confused users.

gundalow commented 2 years ago

Firstly, I'm really happy that people have taken the time to get involved in the discussion, that's the whole reason we have community-topics. There is lot's of great feedback in here, thank you.

I think as various people have said this is a difficult one, and likely not a binary decision.

To recap where we are right now:

Personally, I think the resource modules (as they exist today) are good as:

I'd like to suggest that we accept Resource Modules are they are today, and therefore accept trendmicro.deepsec (assuming everything else is good).

Moving forward, I think given the expertise of this group, would it make sense to:

acozine commented 2 years ago

I'm abstaining. I agree with the argument for separating read-only operations from change operations. I also agree that resource modules have been in use for a long time and have become an accepted alternative. Personally when I'm writing playbooks I prefer each module to do one thing and do it well, and I prefer more restrictive state options, but I'm not ready to ban the resource module approach on that basis.

tadeboro commented 2 years ago

We discussed this issue in the community meeting and came to the following conclusion:

20:46 <tadeboro> With gundalow's +1, we get to 3x-1, 6x+1, 1x0 to accepting resource modules as an official way of writing modules, and that is enough to pass.
20:46 <felixfontein> ok, so it looks like we have 6 x "allow resource modules everywhere", 3 x "restrict them to all of network/security (or even less)", and 1 x no strong opinion either way
20:46 <felixfontein> good that we ended up on the same numbers :)

So as the IRC log excerpt says, we accepted the resource module pattern as an official way of writing modules.

Thank you all for voicing your opinions!