Foreman Ansible Collection - Are SC Param overrides really "All or Nothing"?

[areyus]

Problem:
We are currently trying to transition most of our Foreman configuration to Ansible and are utilizing the theforeman.foreman Ansible collection for this.
What we currently try to achieve is:

• We want to manage Puppet smart-class parameters through Ansible
• Due to how our workflows are designed, we need to only manage certain overrides through Ansible
• The values we do manage need to be created or updated as needed
• The values we do not manage need to stay untouched

The last part is the one we struggle with. From what we found, overrides for smart-class parameters seem to be “All or Nothing”. If we do not specify any overrides, all overrides stay as they are. As soon as we start specifying overrides, all non-managed overrides get deleted.

Expected outcome:
A way to only manage a certain subset of overrides, either via a parameter to the smart_class_parameter module or via a own dedicated module.

Foreman and Proxy versions:
3.2.1 (I know this is dated, but should not be related)

Foreman and Proxy plugin versions:

• foreman-tasks 6.0.1
• foreman_expire_hosts 7.0.4
• foreman_hooks 0.3.17
• foreman_puppet 3.0.7
• foreman_remote_execution 6.0.0
• foreman_scc_manager 1.8.20
• foreman_snapshot_management 2.0.1
• foreman_templates 9.1.0
• foreman_webhooks 3.0.3
• katello 4.4.2.2
• puppetdb_foreman 5.0.0

Distribution and version:
RHEL 8

Other relevant data:
Is suspect this loop to be responsible for what we see.
If I overlooked some way this is already doable, please share with me how that can be achieved
If this is not doable right now: Is this intended behavior or just an unconsidered use case? What would be an acceptable way of implementing it? My first idea would be to just add on option making the above mentioned loop skipable if desired.

[evgeni]

Hi,

yes, the current behaviour is intentional (even if not optimal in all cases, as you point out).
Without the “remove not listed overrides” there would be no way to remove overrides that you previously created and now want gone (via ansible).

The original design idea is/was that you have a playbook that defines all the needed overrides (= green field) and if you want one gone you just drop it from the list and magic happens.

The correct solution for brown field setups (= where you want/can only manage certain overrides) would be to have an own smart_class_parameter_override module, where you can manage individual overrides instead of all of one parameter in the parameter module. But we don’t have such a module today.

[areyus]

First off, thanks for the clarification I that understood the current situation correctly.

Concerning the problem we are currently facing: I understand that current behavior is an interesting scenario for many use cases. In my opinion, it’s a rather bad situation to have it as the only option though.
I would assume most scenarios where people start to adopt Ansible management of their Foreman instance are brown field scenarios, plus the current behavior is most practical if you do not use FQDN based overrides a lot. By enforcing this behavior, you also forcefully remove the users’ ability to integrate Foreman with other tools on this end.

I do understand the need to remove overrides via Ansible, but that should be implementable rather easily. The backend code already can delete overrides (otherwise this problem would not be a topic at all), it’s just a question of how to expose it to the user. My initial thought was to just expose possibilities through the ForemanSmartClassParameterModule to choose between “delete unmanaged” true/false, and additionaly (after reading your thoughts) also exposing the desired state of the overrides.
I agree that having a dedicated smart_class_parameter_override module would probably be the better way, though I would at least feel semi-confident in my skills to implement the first option, while not really having that confidence for implementing a whole new module. (Once I manage to at least get the tests running on may machine to begin with, but that’s a different topic).

Would you be against exposing those options through ForemanSmartClassParameterModule in general or would this be an option worth looking into?

[evgeni]

Yeah, I think I would be against it, so I started working on that new module instead: add smart_class_parameter_override_value module by evgeni · Pull Request #1659 · theforeman/foreman-ansible-modules · GitHub

[areyus]

Alright, that’s a solution I#m even more happy with
Thanks for your effort, if you need someone to test the new module out let me know, otherwise I’m just looking forward to the release

[evgeni]

Oh I’ll definitely come back to you for testing whether this solves your workflows!

[evgeni]

add smart_class_parameter_override_value module by evgeni · Pull Request #1659 · theforeman/foreman-ansible-modules · GitHub should now be working for string-type parameters

still need to poke at non-string ones, but at least it’s something

[evgeni]

@areyus and now other types work fine too. would you mind having a look?

[areyus]

From the looks of it, this looks good
I was at first a little confused about state being missing from both the parameter list and the documentation (it is present in smart_class_parameters, for example), but since you are using it in the tests and other modules are also lacking the explicit mention of state, I assume this is a cosmetic thing and the existance of state is always implied.
I am currently trying to spin up some tests and will get back to you once I can tell if this implementation solves our use-cases

[evgeni]

the state parameter comes (and its docs) come in via shared code, so you don’t see it in the PR.

for s_c_p it is explicitly mentioned for some other reason

if you have the file on your system, look at ansible-doc theforeman.foreman.smart_class_parameter_override_value which should include all the things

[areyus]

It took me a while to set up some tests (I’m still pretty new to Ansible), but after running a few tests against our dev environment, it looks good overall. The only issue I found so far is some sort of type confusion, though I don’t know if this is related to the module or a general Ansible problem. Also, I only found this due to some faulty data in our Foreman instance in the first place (we are currently transitioning a lot of the life data to ansible to enable reproducible Foreman setups).

We have an sc-param that is supposed to take a package name string as argument. Someone now entered a string that looks like a list into this field, like ["package-name"]. It is still a string though.

The data for the Ansible Task to set this value looks like this:

match: hostgroup=hgname
omit: false
value: '["package-name"]'

When I run this in check-mode with diffing enabled, I get this maked as a change:

  "changed": true,
  "diff": {
    "before": {
      "override_values": [
        {
          "id": 1325,
          "match": "hostgroup=hgname",
          "value": "[\"package-name\"]",
          "omit": false,
          "smart_class_parameter_id": 2196
        }
      ]
    },
    "after": {
      "override_values": [
        {
          "id": 1325,
          "match": "hostgroup=hgname",
          "value": [
            "package-name"
          ],
          "omit": false,
          "smart_class_parameter_id": 2196
        }
      ]
    }
  },

So from the looks of it, somewhere on the way the string that contains the syntax for a list got converted to a list and the module would try to write that to Foreman (hopefully failing, since the parameter is declared to be a string).
While in my case this is obviously due to faulty data, I guess there might be use-cases where people need this? Not a blocker for me though, just wanted to bring it up.

[evgeni]

What is the parameter_type of the parameter you’re setting that override on? Is it string or is it array?
https://foreman.example.com/foreman_puppet/api/smart_class_parameters/2196 should have that data.

Also, can you try with the old smart_class_parameter module, I’d expect it to generate the same diff.

[evgeni]

Okay, I can repro this with a parameter_type=string entry. Nice catch!

Patch pushed (and yes, it would also have happened with the old module)

[areyus]

Thanks for the fix, I can confirm the new version does not produce type-confusions anymore
I am still getting a change because now the nested double quotes get converted to single quotes, though I expect that to be a lesser problem.

[evgeni]

Depends on how “the other side” is trying to parse it.

["package-name"] is valid JSON, ['package-name'] isn’t.

[areyus]

That’s a valid point. Currently I see this:

• Before: “[\“package-name\”]”
• After: “[‘package-name’]”
• Data from the data file we pass to ansible: '["package-name"]'

So in case one has a sc-param defined as string where the other end expects a json list, this might in fact cause problems. Probably an edge-case, since it requires you have set up your data type wrong in the first place, but maybe worth fixing.

[evgeni]

I am not sure how much “guessing” I want to do in the conversion. Especially, if it already is a string, encoding is as json is a bad idea (as that would add quotes).

I’ll think a bit about this, but as you already say, this is quite a corner case (and we can fix it later, if we deem that necessary)

[evgeni]

actually, I found a way that seems sane enough, pushed, check it out!

Edit: CI says I broke things, so much for “sane enough”

[areyus]

@evgeni

Edit: CI says I broke things, so much for “sane enough”

I can confirm your changed fixed what happened on our end. No more unintended changes on my test playbook.
The problem with the failed CI job seems to be that the JSON values in the test data are also strings and because of that, are picked up by and not isinstance(value, six.string_types) which causes the code to handle them as strings and not as json.
Though, I can’t figure out why it broke in the first place and stopped breaking now. Both ways should lead to the exact same block of code for strings

parameter_string = value

Also in my tests (with pure python though, not any ansible magic involved), calling str('["something"]'') actually results in the expected '["something"]', so my confusion is actually even greater in regards to why this bit of code keeps doing weird conversions.
The only way I found to actually break it was by calling json.dumps() on the string.