StateApplyResult: enable arrays for name field

[sandonovsuse]

Provides more flexibility at small cost in situations where mapping JSON responses to Java objects, where name field contains an Array of a single String element, fails.

[hustodemon]

@sandonovsuse Is this meant to be the final solution to be backported to the upstream? Or just a quick hotfix?

[sandonovsuse]

@sandonovsuse Is this meant to be the final solution to be backported to the upstream? Or just a quick hotfix?

I was going for a quick fix for this specific bug, trying to avoid adding extra complexity or processing cycles given the fact it's used in a web app. Extedning support to other kinds of response types could be a part of an additional effort at a later time perhaps?

[lucidd]

i already briefly talking with @hustodemon about this but for a proper long term fix its important to find out when and why the name can be an array and if it can be the case that there is more the one element in the array. By gut feeling is that this might just be a bug in salt since returning a 1 element array instead of a string makes little sense.

As for the fix toString on an array will return something like [Ljava.lang.String;@5cbc508c so not what we could use. I did a quick look into the usage of name in uyuni and its used quite a bit to determine what to do so its important that if we have to take the array into account we also need to be able to use it.

We can make name private Xor<String[], String> name; and then either adjust getName to

    public String getName() {
        return name.fold(
                arr -> Arrays.stream(arr).collect(Collectors.joining(",")),
                str -> str
        );
    }

which would make string and single element arrays look the same while multi element arrays would look like name1,name2,name3 so still usable. Or we can change getName to return Xor<String[], String> so every place in uyuni that uses it need to think about how it would handle the array case properly.

Before doing anything we should find out if this is intentional in salt or not since based on that we may just be able to have a temporary fix in uyuni directly until its fixed in salt.

[meaksh]

I was looking into this issue and I think I understand what is going on here and why sometimes we have this returning list and not the expected string.

This behavior has to do with the changes introduced to module.run state syntax few years ago that allows module.run state to receive a list of module instead of single one. This is detailed at https://docs.saltproject.io/en/latest/ref/states/all/salt.states.module.html

This change on the behavior is not enabled yet on Salt as the default behavior, and it was scheduled for a future Salt release, but the switch has been posponed release after release and latest news we got from Sage (not longer in Saltstack) was that they have actually no plans to do the switch at all since it was causing lot of pain for customers that would need to adapt the SLS files. So, in theory they won't be deprecating the old syntax and probable the next syntax will be optional to use, but we have not clear information about this and official Salt documentation still mentioned this new syntax will be introduced in the future.

That is btw why we introduced our compatiblity wrapper mgrcompat.module_run time ago already, to allow our SLS files to be compatible regardless the syntax that is enabled for the minion.

Looking into this bug, I see the customer explicitetly enabled the new syntax in their minion configuration, and this is what causes the issue:

    # Ensure 'module.run' in states will work with future releases
    use_superseded:
      - module.run

This is making Salt to use the newer syntax and therefore produce an output that is not the one that Java would expected.

For example, with the newer syntax, you can have a SLS that would look like the following, where you can execute more than one module in the same state.

test_state_3:
  module.run:
    - cmd.run:
      - cmd: date
    - test.arg:
      - key: val

And that would produce the following output for the "state.apply":

...
        "module_|-test_state_3_|-test_state_3_|-run": {
            "name": [
                "cmd.run",
                "test.arg"
            ],
            "changes": {
                "cmd.run": "Mon Jan 10 14:37:22 CET 2022",
                "test.arg": {
                    "args": [],
                    "kwargs": {
                        "key": "val"
                    }
                }
            },
            "comment": "cmd.run: Mon Jan 10 14:37:22 CET 2022, test.arg: Success",
            "result": true,
            "__sls__": "pepepe",
            "__run_num__": 2,
            "start_time": "14:37:22.389267",
            "duration": 10.978,
            "__id__": "test_state_3"
        }
    }
...

If customer wants to be sure their SLS are not breaking in a future release they can simply use the mgrcompat.module_run wrapper instead. We added some notes about this in the release notes for 4.0 and 4.1 when we released this compatibility wrapper, https://www.suse.com/releasenotes/x86_64/SUSE-MANAGER/4.1/index.html#_salt_module_run_compatibility_state, but maybe it is worth to add it to the official SUSE Manager documentation.

So, that explains why sometimes name is a string (current default behavior in Salt) or a list (future syntax explicitely enabled)

IMHO according to Salt official documentation, enabling the future syntax is possible so it is not really a bug in Salt, so maybe we should makesalt-netapi-client to take care of this possible situation.

Hth!

[lucidd]

ok so if this is gonna be the future syntax for module run then we should definitely handle it properly. Since this is only a module.run specific thing and we are changing the type representing all state apply results we also need to make sure the "default" single entry case is still easy to work with. Making name private Xor<String[], String> name; will already be a good step and we can probably just add a few special getters for usability.

[nadvornik]

It seems that name can be also null for failed actions. See https://bugzilla.suse.com/show_bug.cgi?id=1197449#c1

[nadvornik]

Proposed fix for bsc1197449 https://build.suse.de/request/show/269208

[renner]

Closing this PR in favor of @lucidd's #300