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

schema: Enlarge Command Argument and Environment Key #792

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Aug 15, 2024

From the beginning, the Icinga DB schema allowed 64 characters for both the command arguments and environment variable names. In particular, this affects CheckCommand, EventCommand and NotificationCommand Icinga 2 objects.

But if a command with either an argument key or an environment variable that is longer than 64 characters was defined in Icinga 2, Icinga DB will try to insert it into the database and may end up crashing. Although it may seem large enough, it is sometimes exceeded.

After evaluating that there was no technical limitation, the limit was increased to 255 characters. This limit was chosen over the wider text type as it allows indexes in the future and requires less space.

For example, the following CheckCommand was not possible before:

object CheckCommand "icingadb-i791" {
  import "plugin-check-command"
  command = [ "/bin/true" ]
  env = {
    "THAT_ARE_64_AS_WOW_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" = "huhu"
  }
  arguments = {
    "java.class.that.was.used.as.an.argument.that.was.eighty.seven.characters.long.and.broke" = {
      value = "F"
    }
  }
}

Another thing was a type difference between the MySQL and PostgreSQL schemas. While the MySQL schema defined argument_key_override as varchar(64), in PostgreSQL it was a citext. So it was changed to varchar(255) in MySQL and kept as it was in PostgreSQL.

Closes #791.

From the beginning, the Icinga DB schema allowed 64 characters for both
the command arguments and environment variable names[0]. In particular,
this affects CheckCommand, EventCommand and NotificationCommand Icinga 2
objects.

But if a command with either an argument key or an environment variable
that is longer than 64 characters was defined in Icinga 2, Icinga DB
will try to insert it into the database and may end up crashing.
Although it may seem large enough, it is sometimes exceeded.

After evaluating that there was no technical limitation[1], the limit
was increased to 255 characters. This limit was chosen over the wider
text type as it allows indexes in the future and requires less space.

For example, the following CheckCommand was not possible before:

> object CheckCommand "icingadb-i791" {
>   import "plugin-check-command"
>   command = [ "/bin/true" ]
>   env = {
>     "THAT_ARE_64_AS_WOW_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" = "huhu"
>   }
>   arguments = {
>     "java.class.that.was.used.as.an.argument.that.was.eighty.seven.characters.long.and.broke" = {
>       value = "F"
>     }
>   }
> }

Another thing was a type difference between the MySQL and PostgreSQL
schemas. While the MySQL schema defined argument_key_override as
varchar(64), in PostgreSQL it was a citext. So it was changed to
varchar(255) in MySQL and kept as it was in PostgreSQL.

Closes #791.

[0]: 05d5e97
[1]: #791 (comment)
@oxzi oxzi force-pushed the command-args-envs-len-i791 branch from e122ceb to 091388d Compare August 15, 2024 16:22
@oxzi oxzi added this to the 1.3.0 milestone Aug 15, 2024
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and its detailed description 👍

@yhabteab yhabteab removed the request for review from julianbrost October 18, 2024 08:55
@yhabteab yhabteab merged commit 3484476 into main Oct 18, 2024
32 checks passed
@yhabteab yhabteab deleted the command-args-envs-len-i791 branch October 18, 2024 08:59
yhabteab added a commit that referenced this pull request Nov 15, 2024
PR #792 mistakenly added a `NOT NULL` clause to a nullable columns. This
PR corrects that by dropping that clause and adding a `DEFAULT NULL`
clause, making it consistent the actual schema file.
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.

When using a very large argument key icingadb will see a fatal error and quit
4 participants