Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get-IcingaCheckCommandConfig: provide vars.ifw_api_arguments in each command #633

Merged
merged 3 commits into from
Jul 24, 2023
Merged

Get-IcingaCheckCommandConfig: provide vars.ifw_api_arguments in each command #633

merged 3 commits into from
Jul 24, 2023

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Jun 6, 2023

to properly support just importing the ifw-api command. That command e.g. takes DSL/JSON arrays [], not PS ones @(). Similar with strings. Therefore set vars.ifw_api_arguments as ifw-api expects.

closes #629

TODO

⚠️ Things to consider while reviewing

To keep things simple, I've implied a few inconsistencies:

…command

to properly support just importing the ifw-api command.
That command e.g. takes DSL/JSON arrays [], not PS ones @(). Similar with strings.
Therefore set vars.ifw_api_arguments as ifw-api expects.
@Al2Klimov Al2Klimov marked this pull request as ready for review June 7, 2023 08:03
@julianbrost
Copy link

Simply using the original command arguments initially sounded like a nice plan, but as it turned out, many of the arguments would need special handling to provide different values depending on how the check is executed. So I think it absolutely makes sense to simply provide the arguments separately.

My main question for @LordHepipud: does this, i.e. not supporting any functions/lambdas but just macro strings in all of ifw_api_arguments (i.e. no set_if = {{ ... }}, no value = {{ ... }}), work for all possible scenarios that arguments are used (or planned to be used) in Icinga for Windows?

To keep things simple, I've implied a few inconsistencies:

  • Empty arrays aren't explicitly passed, but disappear
  • Empty strings -if you can even produce such in Director- don’t disappear, but are explicitly passed
  • SecureString is handled as String

Is any of this due to limitations of the C++ implementation?

  • No order anywhere, Icinga DSL maps sort themselves alphabetically anyway

For this one, I understand the implications, this probably means that you don't generate order attributes in ifw_api_arguments because at best, this could affect the order of entries in the JSON object, but those are typically interpreted as unordered key/value structures anyways (because of this, the current implementation in Icinga 2 provides no useful way to influence that order and implementing that would be a waste of time).

@Al2Klimov
Copy link
Member Author

does this, i.e. not supporting any functions/lambdas but just macro strings in all of ifw_api_arguments (i.e. no set_if = {{ ... }}, no value = {{ ... }}), work for all possible scenarios that arguments are used (or planned to be used) in Icinga for Windows?

Ok, these are actually two questions, with a big difference:

  • What's used right now: I've analysed the above if/elseif/... (just above mine). It uses the DSL functions just for rendering (which is different in ifw-api of course). The actual data sources are always "$foo.bar$", w/ or w/o macro(). Basically what I do.
  • What's planned will have no problems with data supplier DSL functions which are stored in custom vars. Shouldn't be that difficult.

To keep things simple, I've implied a few inconsistencies:

  • Empty arrays aren't explicitly passed, but disappear
  • Empty strings -if you can even produce such in Director- don’t disappear, but are explicitly passed
  • SecureString is handled as String

Is any of this due to limitations of the C++ implementation?

At least the array I think. But I'd like @LordHepipud to confirm that it's just fine or to reason that behaviour in the current renderers, so that I know what, what for and how to resolve.

  • No order anywhere, Icinga DSL maps sort themselves alphabetically anyway

For this one, I understand the implications, this probably means that you don't generate order attributes in ifw_api_arguments because at best, this could affect the order of entries in the JSON object

This couldn’t even that. I mean, I could just C&P the order... only to waste space and traffic.

@julianbrost
Copy link

To keep things simple, I've implied a few inconsistencies:

  • Empty arrays aren't explicitly passed, but disappear
  • Empty strings -if you can even produce such in Director- don’t disappear, but are explicitly passed
  • SecureString is handled as String

Is any of this due to limitations of the C++ implementation?

At least the array I think. But I'd like @LordHepipud to confirm that it's just fine or to reason that behaviour in the current renderers, so that I know what, what for and how to resolve.

Are the first two trying to say that this config

vars.ifw_api_arguments = {
  "-Array" = {"value" = "$empty_array$"}
  "-String" = {"value" = "$empty_string$"}
}
vars.empty_array = []
vars.empty_string = ""

would result in the following JSON (no -Array)?

{"-String":""}

I'm just trying to understand the implications if what you've stated as inconsistencies here and if any of this is something I should look into when reviewing the C++ part. By the way, I still have no idea about that SecureString thing so far, but I guess that's a pure PowerShell thing as there is no such thing in Icinga 2 or JSON.

  • No order anywhere, Icinga DSL maps sort themselves alphabetically anyway

For this one, I understand the implications, this probably means that you don't generate order attributes in ifw_api_arguments because at best, this could affect the order of entries in the JSON object

This couldn’t even that. I mean, I could just C&P the order... only to waste space and traffic.

I think you understood this differently than what I was trying to say. The only thing that Icinga 2 ifw-api could theoretically do if we would do massive changes to how we generate JSON, would be to allow you to change whether {"-a": "foo", "-b": "bar"} or {"-b": "bar", "-a": "foo"} is generated. Most JSON libraries will treat the elements in JSON objects as unordered, so implementing this would make no sense. So I agree that omitting order from ifw_api_arguments makes sense.

@Al2Klimov
Copy link
Member Author

Al2Klimov commented Jun 7, 2023

  • Empty arrays aren't explicitly passed, but disappear
  • Empty strings -if you can even produce such in Director- don’t disappear, but are explicitly passed
  • SecureString is handled as String

Is any of this due to limitations of the C++ implementation?

At least the array I think. But I'd like @LordHepipud to confirm that it's just fine or to reason that behaviour in the current renderers, so that I know what, what for and how to resolve.

Are the first two trying to say that this config

vars.ifw_api_arguments = {
  "-Array" = {"value" = "$empty_array$"}
  "-String" = {"value" = "$empty_string$"}
}
vars.empty_array = []
vars.empty_string = ""

would result in the following JSON (no -Array)?

{"-String":""}

Yes, I think so. I thought so. But now I'm not sure anymore. Maybe -due to a bug on the C++ side- it ends up with "-Array":true. I have to double check...

@julianbrost
Copy link

Another thing for the "Things to consider while reviewing" list: arrays containing just a single item are passed as a string instead of an array (see also Icinga/icinga2#9062 (comment)). For the one single thing I tested, this worked though.

@Al2Klimov
Copy link
Member Author

  • SecureString is handled as String

I see this was clever, but not smart: Icinga/icinga2#9062 (comment)

@LordHepipud Any suggestions how this one shall pass security strings?

@LordHepipud
Copy link
Collaborator

The SecureString handling is fine. The issue, that the new code in this case is rendering the arguments with -.

For example: -SqlPassword

It should only be SqlPassword, without the leading -

I updated the code on my test system, generated the new config and everything works as expected now.

@Al2Klimov
Copy link
Member Author

Only SecureString args? Not all types?

@LordHepipud
Copy link
Collaborator

LordHepipud commented Jun 30, 2023

All arguments are meant to not have the leading - while using the API. While it is working, the internal validator expects them without the leading -.

For the config, I will enforce the generation without - and update Icinga for Windows to be able to handle both identical.

@LordHepipud LordHepipud added this to the v1.11.0 milestone Jun 30, 2023
@julianbrost
Copy link

The SecureString handling is fine. The issue, that the new code in this case is rendering the arguments with -.

For example: -SqlPassword

It should only be SqlPassword, without the leading -

I updated the code on my test system, generated the new config and everything works as expected now.

FYI: the example in the documentation includes the -:

{ '-Core': 0 }

And the single quotes actually make this invalid JSON and GitHub renders it in a nice red (the PS daemon accepts it though).


## Icinga Communication to API

With Icinga 2.14.0 and later, you can enable the Icinga Agent to natively communicate with the Icinga for Windows API, allowing checks being executed without having to start a PowerShell.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LordHepipud LordHepipud self-assigned this Jul 24, 2023
@LordHepipud LordHepipud merged commit 57b21ef into Icinga:master Jul 24, 2023
@LordHepipud
Copy link
Collaborator

Thanks!

@Al2Klimov Al2Klimov deleted the ifw_api_arguments branch July 25, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants