disussion #482 - It should be reviewed every parameter of each role to configure the right default behavior

[adonisgarciac]

Summary

Following the disscusion #482, I want to bring it to an issue.

The roles of this collection have omit or omit,true in their parameters and it is not the default behavior of that parameters. It doesn't follow the principle of configuration as code.

I mean, if you change in the GUI of controller some parameter and you don't set this parameter as code, actually you won't have this object in configuration as code in full.

In my opinion, if you want to have your object(s) as code, you have to have every parameter as code. If you don't have some of them, you will have a default value even though you have already configured a value for it through the GUI

This approach will do that this collection will be closer to a "desired state" behavior.

[adonisgarciac]

NOTE: It could break if it is using an AAP without the latest version because it will pass always every parameter and the target AAP (without latest version) could not have some of them.

Maybe it isn't a problem if it is using the collection of awx/ansible.controller/ansible.tower which fit with the version of awx/tower/controller. I believe that as the awx/ansible collection don't have that parameter, it won't send to the AWX/tower/controller.

[djdanielsson]

We will most likely keep omit for most things so I don't think it should be an issue but is worth noting

[adonisgarciac]

Why do you prefer to keep omit for most things? Do you prefer that users could change parameters from the GUI and not change them with the Configuration as Code if they have not defined this parameter?

I'm wondering how we want to address this behavior. Setting a default value will be more disruptive but it will be closer to a desired state because you will have to have everything as code, otherwise, a default value will be sent to the API and your manual changes done through the GUI will be lost.

Sorry if this is not good to be an issue, we could continue in the discussion again.

[Tompage1994]

The reason we went with default(omit) (forgetting the usage of (omit,true)) was because we wanted the module defaults to be used. The only sensible thing we can do otherwise is default to what the module defaults to but then if the module's default changes then we would need to reflect this (and its pretty hard to keep track of that).

Until I've seen a proposed example solution to this I'd be hesitant to go down this path.

[adonisgarciac]

(and its pretty hard to keep track of that).

Yes, this is the most problematic thing.

The reason we went with default(omit) (forgetting the usage of (omit,true)) was because we wanted the module defaults to be used

Ok, but now if someone change manually a parameter through the GUI, it will persist if you have not configured as code.

[adonisgarciac]

We could address it with a variable for who wants to configure a default behavior. I will show an example for JT.

Current definition in the role JT of this collection:

  job_template:
    name:                                 "{{ __controller_template_item.name | mandatory }}"
    new_name:                             "{{ __controller_template_item.new_name | default(omit, true) }}"
    copy_from:                            "{{ __controller_template_item.copy_from | default(omit, true) }}"
    description:                          "{{ __controller_template_item.description | default(omit, true) }}"
    execution_environment:                "{{ __controller_template_item.execution_environment.name | default(__controller_template_item.execution_environment | default(omit, true)) }}"
    job_type:                             "{{ __controller_template_item.job_type | default('run') }}"
    inventory:                            "{{ __controller_template_item.inventory.name | default(__controller_template_item.inventory | default(omit, true)) }}"
    labels:                               "{{ __controller_template_item.labels | default(__controller_template_item.related.labels | default([]) | map(attribute='name') | list) | default(omit) }}"
(. . .)

Proposal:

  job_template:
    name:                                 "{{ __controller_template_item.name | mandatory }}"
    new_name:                             "{{ __controller_template_item.new_name | default(default_jt_new_name) | default(omit) }}"
    copy_from:                            "{{ __controller_template_item.copy_from | default(default_jt_copy_from) | default(omit) }}"
    description:                          "{{ __controller_template_item.description | default(default_jt_description) | default(omit) }}"
    execution_environment:                "{{ __controller_template_item.execution_environment.name | default(__controller_template_item.execution_environment | default(default_jt_ee) | default(omit)) }}"
    job_type:                             "{{ __controller_template_item.job_type | default(default_jt_type) | default('run') }}"
    inventory:                            "{{ __controller_template_item.inventory | default(default_jt_inventory) | default(omit) }}"
    labels:                               "{{ __controller_template_item.labels | default(default_jt_labels) | default(omit) }}"
    (. . . )

I would also change default(omit,true) for default(omit) in cases that the parameter can be a string because if you have configured the parameter as an empty value and it is an allowed value, it would be sent to the controller/awx.

[adonisgarciac]

Hi! What do you think of my latest proposal?

[Tompage1994]

Sorry, didn't spot that previous comment until now.

I'm not sure I like the idea of introducing a whole bunch of new variables for defaults when the module defaults these all anyway to sensible values. I get the issue of not correcting back to default values of things changed in the GUI but if you're that concerned about that you wouldn't allow changes in the GUI anyway. I can also see a scenario where people would want the GUI changes to be kept for values they haven't defined. Particularly in dev environments etc. So I don't think I like enforcing this change at all.

[adonisgarciac]

In my suggestion it will not be forcing to the default value, it will only happen if you define a default value.

I know that the code will become more complicated but it will give to the user the opportunity to define a default behavior.

[jpcarmona]

Hi! we agree with the solution proposed by @adonisgarciac , could this new implementation be approved?

[adonisgarciac]

Hi @Tompage1994 @djdanielsson @sean-m-sullivan @ivarmu @silvinux what do you think about my suggestion? I'd like to give users the possibility to set default values in order to achieve a desired Configuration as Code.

The code that I'm proposing, will only change the behavior in case the user has set a default value in their variables. If not, the behavior will be the same. With this change, users will have the opportunity to be sure that any manual change in the GUI, will be removed if this is their desire.

In the other hand, this code will also fix the not expected behavior of (omit,true).

[ivarmu]

In my opinion, the CasC must be the only source for the final configuration state, so all the contents modified outside the CasC should be replaced by the ones indicated by the code. Otherwise, you could have a lot of uncontrolled changes outside your CasC and have troubles when you run your CasC on another AAP instance, as for example, your production environment.

Making an analogue, ArgoCD is not allowing any change at the Kubernetes objects by default, and let's you specify what dou you want to allow explicitly, ignoring the changes at the object types and parameter path as you especified in your application or applicationset. I think our CasC should be near that concept as well, and the proposal by @adonisgarciac is a very good starting point.

[silvinux]

I partially agree with @adonisgarciac, we should give the user a way of controlling that behavior but not in every parameter of each module because we already have parameters that could leverage variables to handle this from the user side. For instance, a variable that could be set in the playbook that run all tasks and that variable could be used in the list of that object:

$ cat playbook.yaml
---
- name: Filetree read test
  gather_facts: false
  vars:
    jt_use_fact_cache: false
  roles:
    - infra.controller_configuration.filetree_read
    - infra.controller_configuration.dispatch
...

$ cat job_template_list.yaml
--- 
job_template:
  - name: jt_1
    use_fact_cache: "{{ jt_use_fact_cache }}"
...

What I think it would be useful is to have a global variable for all boolean or integer parameters. I'm not sure but I think all of them are set by default to false or '0'. This should partially mitigate the issue that the customer is facing, forcing to apply the default boolean parameters when creating a Job Template for example.

For strings, it doesn't make any sense to create default variables for them because they are not common and can have any value depending on the object. For example, the description, which should not be the same in any Job Template.

[adonisgarciac]

Thanks @silvinux for your comment. I believe we are thinking in different stages.

What you are proposing is to have some default values from variables which are reading in the playbook in order to give these default values to the "developers" which will create the Configuration as Code in git. And this, as you said, does not make sense for most of parameters.

However, what I mean is to have the possibility to set default values in the collection itself. This configuration will give to the admins the option to force some values to some parameters in case "developers" have not set this parameters in the CasC. I'm going to try to explain better with an example.

First of all, imagine this code in the collection (my previous proposal):

  job_template:
    name:                                 "{{ __controller_template_item.name | mandatory }}"
    new_name:                             "{{ __controller_template_item.new_name | default(default_jt_new_name) | default(omit) }}"
    copy_from:                            "{{ __controller_template_item.copy_from | default(default_jt_copy_from) | default(omit) }}"
    description:                          "{{ __controller_template_item.description | default(default_jt_description) | default(omit) }}"
    execution_environment:                "{{ __controller_template_item.execution_environment.name | default(__controller_template_item.execution_environment | default(default_jt_ee) | default(omit)) }}"
    job_type:                             "{{ __controller_template_item.job_type | default(default_jt_type) | default('run') }}"
    inventory:                            "{{ __controller_template_item.inventory | default(default_jt_inventory) | default(omit) }}"

Now, admins have these default variables set:

$ cat playbook.yaml
---
- name: Filetree read test
  gather_facts: false
  vars:
    default_jt_inventory: "my_default_inventory"
    default_jt_description: "my_default_description"
  roles:
    - infra.controller_configuration.filetree_read
    - infra.controller_configuration.dispatch
...

If some "developers", some organization admin, etc.. try to add new JT in CasC without inventory or description, the CasC "engine" will use "my_default_inventory" and "my_default_description". I know that inventory and description are not good examples but it is only an example to understand the behavior.

This would be useful for many user cases. First if admins want to have default behavior for some parameters. Second and probably most important, in case admins want to remove every change which was made by the GUI. Because now, if someone change some parameter in the GUI and this parameter is not configured in the CasC, the GUI change will persist.

Reminder, the proposal only will apply the default variable if exist and it is declared by the admins. If the default variable does not exist, the default value will be (omit), like currently.

[ivarmu]

I think that the purpose of this conversation is, in fact, to let all the things defined as code to be translated into the AAP, that's for example... Today I'm creating a new Job Template with "this is my first description" as the description; a week after that, I realise that the Job Template name is self-describing enough and the description is confusing the JT's users, so I decide to set the description to "" (an empty string). Right now, that change won't be applied, as the default(omit[,true]) is skipping the possibility to apply that empty value.

To summarize, in my opinion, the default values should be defined at the role defaults' location, like they are already, and it's default values should allow the roles to avoid the usage of default(omit,[true]), as the variables should ever exist and have a valid value. If the role user provides an invalid value, is not the responsibility of the role to control this, so AAP will throw an error and returned to the user as expected. That way, the users can establish their own default values (overriding the role defaults) and the main intent is covered, as any valid value will be applied to the AAP configuration.

[adonisgarciac]

Hi @ivarmu the problem in your suggestion is if some user/org wants to have the possibility to manage some parameters by GUI. I know that this approach wants to manage everything as code but I'd like to give users as much flexibility as possible and it will be a big change of behavior.

The problem that you are hitting could be fixed changing (omit,true) for (omit) in the parameters that this change is required.

[adonisgarciac]

After some conversation with @ivarmu and @silvinux. another solution is to change the behavior of the collection and apply always the default value. I mean, change default(omit) for default(default_value_of_parameter) to ensure Configuration as Code. Example for JT description: default(omit,true) -> default("")

It makes sense because if you are managing some objects as code, you should not change this object by GUI.

One problem here is that the version of this collection has to be related to version of AAP/AWX but we are already doing it. This is due to new/change parameters in new versions of product.

@sean-m-sullivan @Tompage1994 @djdanielsson what do you think about this?

[djdanielsson]

I have been thinking about this some, this would break all old versions and we cannot maintain multiple copies of this collection to support the multiple AAPs in reality so I do not think this is a good solution. I think we keep omit but remove the true so a blank "" can be passed and wipe out current settings. if someone cares to set a value it should be in code, if they allow some stuff to be done in the UI then that is on them. this latest proposal will be a huge pain to maintain and I do not think the value is there.

[sean-m-sullivan]

Discussion today in the meeting. WE should do this, but with an if/else to do the parameter default vs default omit, And set at role level like secure logging turns on/off. will work up an example role around this.

[adonisgarciac]

@sean-m-sullivan great job!

But I have a question, I don't understand why did not you use if/else. Something like this seems to work:

description: "{{ __application_item.description | default(('' if controller_configuration_applications_enforce_defaults else omit), true) }}"

[sean-m-sullivan]

I was trying outside of the default, not inside the default, .......

[adonisgarciac]

If my proposal works properly, could be better to use if/else solution instead of using a custom plugin. Do you agree?

[sean-m-sullivan]

Just tested, it works, looking at changing.

[sean-m-sullivan]

After futher testing, default is being wonky and ignoring the default set to false when using the new method, we are going back to the custom plugin, and I'll be raising an issue with ansible itself.

[sean-m-sullivan]

And following up, we may have to fix the awx.awx collection on some fields as they are passed correctly, but not pushed to the api correctly.

[sean-m-sullivan]

Adding this to the issue
ansible/awx#14128

We will need to update the roles to conform with this as it moves the source of truth for defaults from the roles, to the API/Modules.

[sean-m-sullivan]

I am closing this issue on our end. we have it working as best we can atm, changes needed to the api and awx collection to pursue this further, Its a good idea, but I think out of scope for the collection any further.