HTTP: Added variable validation to the response_headers option

[hongzhidao]

This is to improve error messages for response headers configuration. Take the configuration as an example:

  {
      "response_headers": {
          "a": "$b"
      }
  }

Previously, when applying it the user would see this error message:

  failed to apply previous configuration

After this change, the user will see this improved error message:

  the previous configuration is invalid: Unknown variable "b" in the "a" value

[lcrilly]

I don't think the word "previous" is actually used in error messages, and it would be confusing to introduce it now.

Something like this would be far more useful. Now that we have route logging, it would be great to see the route URI show up in the config-apply error messages.

{
  "error": "Invalid configuration.",
  "detail": "Unknown variable $b in routes/0/action/response_headers/a."
}

[hongzhidao]

Hi @lcrilly,

"detail": "Unknown variable $b in routes/0/action/response_headers/a."

I agree to display describable error messages whenever possible, but it's another separate topic than the PR.

I don't think the word "previous" is actually used in error messages, and it would be confusing to introduce it now.

Take the below configuration as an example,

"listeners": {
        "*:8080": {
            "pass": 1
        }
    }

It shows an error like this.

the previous configuration is invalid: The "pass" value must be a string, but not an integer number.

It's a general handling internally and happens in the control process that validates configuration before applying it.

Previously, when applying it the user would see this error message: failed to apply previous configuration

This happened in the router process when it was applied.

After this change, the user will see this improved error message: the previous configuration is invalid: Unknown variable "b" in the "a" value

This is what the patch did, it's similar to this commit. https://github.com/nginx/unit/commit/49aee6760a26b14c06180e4f25088cb18e2037a7

[lcrilly]

In my experience "previous configuration is invalid" is shown when Unit starts and tries to apply the configuration in the state directory.

When applying configuration to a running instance I see

$ echo '{"a":"$b"}' | unitc /config/routes/0/action/response_headers
2024/03/19 10:18:45 [alert] 51893#7308836 failed to apply new conf
{
  "error": "Failed to apply new configuration."
}

Are you only addressing the startup case with this PR? That seems like an incomplete approach.

[hongzhidao]

I see what you mean.

Are you only addressing the startup case with this PR?

It's both for startup case and control API.

Sorry that I didn't test this case, I just followed the code and thought it was general. Here's the difference if we test with control API:

{
    "error": "Failed to apply new configuration."
}

vs

{
    "error": "Invalid configuration.",
    "detail": "Unknown variable \"b\" in the \"a\" value."
}

As you see, we should validate the configuration in the controller process so that it can show the exact error as much as possible.

[lcrilly]

Variables don't contain spaces so why would we put (escaped) quotes (\") around it? I also think the $ prefix would make it clearer.

[hongzhidao]

Variables don't contain spaces so why would we put (escaped) quotes (\") around it?

Yes, it doesn't.

I also think the $ prefix would make it clearer.

"detail": "Unknown variable \"b\" in the \"a\" value."

"detail": "Unknown variable \"$b\" in the \"a\" value."

TBH, both are fine for me. We could improve with another patch and it needs to be general for all variables.

[ac000]

Variables don't contain spaces so why would we put (escaped) quotes ( \" )
around it? I also think the $ prefix would make it clearer.

The "'s are escaped because JSON... (i.e the whole string is in quotes).

[ac000]

I think this patch is fine for what it's doing.

The other suggestions would likely be a larger more intrusive patch.

[hongzhidao]

The other suggestions would likely be a larger more intrusive patch.

Yep, they can be separated patches if needed.

The "'s are escaped because JSON... (i.e the whole string is in quotes).

JSON does provide the function of escaping strings in Unit configuration, but in this case of variables error message, it's assembled manually.

static nxt_int_t
nxt_conf_vldt_var(nxt_conf_validation_t *vldt, nxt_str_t *name,
    nxt_str_t *value)
{
    u_char  error[NXT_MAX_ERROR_STR];

    if (nxt_tstr_test(vldt->tstr_state, value, error) != NXT_OK) {
        return nxt_conf_vldt_error(vldt, "%s in the \"%V\" value.",
                                   error, name);
    }

    return NXT_OK;
}