Opened 8 years ago

Closed 8 years ago

#1627 closed defect (fixed)

Error: Server configuration failed: incomplete textual name

Reported by: jreed Owned by: muks
Priority: medium Milestone: Sprint-20120403
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 1 Internal?: no

Description

> config go /Auth/
/Auth> config go datasources
/Auth/datasources> config show
Auth/datasources        []      list    (default)
/Auth/datasources> config add 
/Auth/datasources> config show
Auth/datasources[0]/type        ""      string  (default)
Auth/datasources[0]/class       "IN"    string  (default)
Auth/datasources[0]/zones       []      list    (default)
/Auth/datasources> config go [0]
/Auth/datasources[0]> config show
Auth/datasources[0]/type        ""      string  (default)
Auth/datasources[0]/class       "IN"    string  (default)
Auth/datasources[0]/zones       []      list    (default)
/Auth/datasources[0]> config set type memory
/Auth/datasources[0]> config add zones
/Auth/datasources[0]> config show
Auth/datasources[0]/type        "memory"        string  (modified)
Auth/datasources[0]/class       "IN"    string  (default)
Auth/datasources[0]/zones/      list    (modified)
/Auth/datasources[0]> config commit
Error: Server configuration failed: incomplete textual name
Configuration not committed
/Auth/datasources[0]> 

This error is unclear. The same error is in the log.

What is incomplete?

Subtickets

Change History (18)

comment:1 Changed 8 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120403

comment:5 Changed 8 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:6 Changed 8 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

Up for review.

This generates:

> config add Auth/datasources[0]/zones
> config show Auth/datasources[0]
Auth/datasources[0]/type	"memory"	string	(modified)
Auth/datasources[0]/class	"IN"	string	(default)
Auth/datasources[0]/zones/	list	(modified)
> config show Auth/datasources[0]/zones
Auth/datasources[0]/zones[0]/origin	""	string	(modified)
Auth/datasources[0]/zones[0]/file	""	string	(modified)
> config show Auth/datasources[0]/zones[0]
Auth/datasources[0]/zones[0]/origin	""	string	(modified)
Auth/datasources[0]/zones[0]/file	""	string	(modified)
> config commit
Error: unable to parse zone's origin: incomplete textual name
Configuration not committed
> 

In the logs:

2012-03-26 17:47:11.501 ERROR [b10-auth.auth] AUTH_CONFIG_UPDATE_FAIL update of configuration failed: unable to parse zone's origin: incomplete textual name
2012-03-26 17:47:11.505 ERROR [b10-cmdctl.cmdctl] CMDCTL_COMMAND_ERROR error in command set_config to module ConfigManager: unable to parse zone's origin: incomplete textual name

comment:7 follow-up: Changed 8 years ago by jreed

If the problem is just a missing origin, how about just have "missing origin"? Is it parsing? Why "textual name"?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by muks

Replying to jreed:

If the problem is just a missing origin, how about just have "missing origin"? Is it parsing? Why "textual name"?

That error is from the name parser (origin is parsed by the name parser), so that's the general parse error message.

We now get:

Error: unable to parse zone's origin: incomplete textual name

The name parser can return various errors. The general format is:

Error: unable to parse zone's origin: <name-parser-error>

comment:9 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jinmei

Replying to muks:

Replying to jreed:

If the problem is just a missing origin, how about just have "missing origin"? Is it parsing? Why "textual name"?

That error is from the name parser (origin is parsed by the name parser), so that's the general parse error message.

As far as I understand it, the real error in the configuration in
question is that origin was *unspecified*, not really an invalid form
of it (although "unspecified" could be considered a part of invalid
form). So I think a more helpful error is something that specifically
points it out, like this patch:

     BOOST_FOREACH(ConstElementPtr zone_config, zones_config->listValue()) {
         ConstElementPtr origin = zone_config->get("origin");
-        if (!origin) {
+        const string origin_txt = origin ? origin->stringValue() : "";
+        if (origin_txt.empty()) {
             isc_throw(AuthConfigError, "Missing zone origin");
         }

Then what we'd see on bindctl is:

> config show Auth/datasources[0]/zones
Auth/datasources[0]/zones[0]/origin	""	string	(modified)
Auth/datasources[0]/zones[0]/file	""	string	(modified)
> config commit
Error: Missing zone origin
Configuration not committed

(this could be even more helpful if it said in which configuration it
fails (e.g. "Missing zone origin for in-memory zone #0"), but that's
not exactly the point of this ticket).

Same for file, btw.

Ideally, the spec file should be more descriptive so it can be
rejected at that level, but as far as I know we need to provide some
default anyway (in this case, an empty string). So, technically, we
cannot distinguish intentionally specified empty string from
unspecified config, but in practice if an empty string is passed it's
most likely that the user forgets to fill in it.

If you agree on this, we could more localize the fixes (we don't have
to modify name.h, etc). But I'll comment on the changes in the branch
anyway:

  • as convention we generally add '[ticket #]' in the beginning of each commit log message. So, in this case it should be '[1627]'. If you don't have a particular reason not to follow this convention, I'd suggest you do the same for consistency.
  • there are some style issues. I've directly fixed them on the branch. Please check it. In particular, there were some "hard tabs (\t)", which we generally don't use.
  • clarifying the error message when the given name is invalid would itself be good, and since this branch isn't big so I'm okay with doing it here even if we agree the essential issue is not the validity of the name. However, I'd like to avoid adding try-catch for each of this level of things. If we use exception this way in the configuration checker, it will be full of such micro try-catch blocks as the syntax becomes more complicated. I'd rather keep some context while we parse the configuration, and just let an exception thrown from a lower layer (like the dns::Name class) be propagated at a higher level like configureAuthServer(). We can catch the exception there, convert it to AuthConfigError with the saved context information:
        } catch (const isc::Exception& ex) {
            isc_throw(AuthConfigError, "Server configuration failed at "
                      << printContext() << ": " << ex.what());
        }
    
    Then the result would look like:
    Error: Server configuration failed at parsing zone origin of in-memory zone #0: <error message from a lower layer>
    
  • Regarding the exceptions in the name class, while it might be beyond the scope of this ticket, one thing we should have done before is to add the text that triggers the exception, e.g.
                        isc_throw(EmptyLabel, "non terminating empty label in " <<
                                  namestring);
    
  • I think it a good idea to introduce some intermediate class hierarchy in for exception classes in libdns++ (in fact there should be a ticket for that, I think), but I'd like to do it based on some consistent design consideration, rather than by introducing an intermediate class in ad hoc manner as we see the need for it. e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError?

=> isc::dns::NameParserError? => isc::dns::EmptyLabel?

  • In name_unittest.cc many cases seem to be a result of copy-paste. I'd like to unify these cases like:
template <typename ExceptionType>
void
checkBadTextName(const string& txt) {
    // Check it results in the specified type of exception as well as
    // NameParserException.
    EXPECT_THROW(Name(txt, false), ExceptionType);
    EXPECT_THROW(Name(txt, false), NameParserException);
}
...
    checkBadTextName<EmptyLabel>(".a");

comment:10 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to muks

comment:11 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by muks

  • Owner changed from muks to jinmei

Hi jinmei

Thank you for the detailed review :)

Replying to jinmei:

As far as I understand it, the real error in the configuration in
question is that origin was *unspecified*, not really an invalid form
of it (although "unspecified" could be considered a part of invalid
form). So I think a more helpful error is something that specifically
points it out, like this patch:

     BOOST_FOREACH(ConstElementPtr zone_config, zones_config->listValue()) {
         ConstElementPtr origin = zone_config->get("origin");
-        if (!origin) {
+        const string origin_txt = origin ? origin->stringValue() : "";
+        if (origin_txt.empty()) {
             isc_throw(AuthConfigError, "Missing zone origin");
         }

Then what we'd see on bindctl is:

> config show Auth/datasources[0]/zones
Auth/datasources[0]/zones[0]/origin	""	string	(modified)
Auth/datasources[0]/zones[0]/file	""	string	(modified)
> config commit
Error: Missing zone origin
Configuration not committed

(this could be even more helpful if it said in which configuration it
fails (e.g. "Missing zone origin for in-memory zone #0"), but that's
not exactly the point of this ticket).

Same for file, btw.

You're right. I've changed both missing checks now. :)

Ideally, the spec file should be more descriptive so it can be
rejected at that level, but as far as I know we need to provide some
default anyway (in this case, an empty string). So, technically, we
cannot distinguish intentionally specified empty string from
unspecified config, but in practice if an empty string is passed it's
most likely that the user forgets to fill in it.

If you agree on this, we could more localize the fixes (we don't have
to modify name.h, etc). But I'll comment on the changes in the branch

I agree, but please elaborate further.

to modify name.h, etc). But I'll comment on the changes in the branch
anyway:

  • as convention we generally add '[ticket #]' in the beginning of each commit log message. So, in this case it should be '[1627]'. If you don't have a particular reason not to follow this convention, I'd suggest you do the same for consistency.

[number] is parsed by Trac as a link to a commit. #number is parsed by Trac
and linked to a bug. Instead of using #number, I used "bug #number" because
when editing a git commit log in an editor, lines that start with # are
assumed as comments and not included in the commit log message.

  • there are some style issues. I've directly fixed them on the branch. Please check it. In particular, there were some "hard tabs (\t)", which we generally don't use.

*nod* :)

  • clarifying the error message when the given name is invalid would itself be good, and since this branch isn't big so I'm okay with doing it here even if we agree the essential issue is not the validity of the name. However, I'd like to avoid adding try-catch for each of this level of things. If we use exception this way in the configuration checker, it will be full of such micro try-catch blocks as the syntax becomes more complicated. I'd rather keep some context while we parse the configuration, and just let an exception thrown from a lower layer (like the dns::Name class) be propagated at a higher level like configureAuthServer(). We can catch the exception there, convert it to AuthConfigError with the saved context information:
        } catch (const isc::Exception& ex) {
            isc_throw(AuthConfigError, "Server configuration failed at "
                      << printContext() << ": " << ex.what());
        }
    
    Then the result would look like:
    Error: Server configuration failed at parsing zone origin of in-memory zone #0: <error message from a lower layer>
    

I agree on avoiding inner try-catch blocks, but am not sure if maintaining a context would be less work than prepending the context and rethrowing, when something is thrown.

  • Regarding the exceptions in the name class, while it might be beyond the scope of this ticket, one thing we should have done before is to add the text that triggers the exception, e.g.
                        isc_throw(EmptyLabel, "non terminating empty label in " <<
                                  namestring);
    

*nod*. This is implemented now, and in some places checks for an empty string too where it can happen.

  • I think it a good idea to introduce some intermediate class hierarchy in for exception classes in libdns++ (in fact there should be a ticket for that, I think), but I'd like to do it based on some consistent design consideration, rather than by introducing an intermediate class in ad hoc manner as we see the need for it. e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError?

=> isc::dns::NameParserError? => isc::dns::EmptyLabel?

This is perhaps good for another ticket too. I had a reason to add the NameParserException? parent for this bug. :) We can change it however we want.

  • In name_unittest.cc many cases seem to be a result of copy-paste. I'd like to unify these cases like:
template <typename ExceptionType>
void
checkBadTextName(const string& txt) {
    // Check it results in the specified type of exception as well as
    // NameParserException.
    EXPECT_THROW(Name(txt, false), ExceptionType);
    EXPECT_THROW(Name(txt, false), NameParserException);
}
...
    checkBadTextName<EmptyLabel>(".a");

Ouch! It should've been written this way. :) I have modified it to use this now. (Also updated a testcase for a different bug to use a helper function similarly.)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by jinmei

Replying to muks:

Ideally, the spec file should be more descriptive so it can be
rejected at that level, but as far as I know we need to provide some
default anyway (in this case, an empty string). So, technically, we
cannot distinguish intentionally specified empty string from
unspecified config, but in practice if an empty string is passed it's
most likely that the user forgets to fill in it.

If you agree on this, we could more localize the fixes (we don't have
to modify name.h, etc). But I'll comment on the changes in the branch

I agree, but please elaborate further.

??? I don't understand this. By "But I'll comment on the changes in
the branch anyway", I meant: I could have stopped there and confirmed
whether or not we agree on where the issue was and made comments on
the actual changes only if not (because if we agreed, the minimum
necessary change could just be a few line of changes only within
auth_config.cc); but I actually also made comments on the rest of the
changes in the branch rather than waiting. Do you need something
more/else?

  • as convention we generally add '[ticket #]' in the beginning of each commit log message. So, in this case it should be '[1627]'. If you don't have a particular reason not to follow this convention, I'd suggest you do the same for consistency.

[number] is parsed by Trac as a link to a commit. #number is parsed by Trac
and linked to a bug. Instead of using #number, I used "bug #number" because
when editing a git commit log in an editor, lines that start with # are
assumed as comments and not included in the commit log message.

Ah, okay. In that case using '#' may make more sense. In either case
I think we should use a consistent style, so maybe we should discuss
this at the mailing list.

I agree on avoiding inner try-catch blocks, but am not sure if maintaining a context would be less work than prepending the context and rethrowing, when something is thrown.

I have a feeling that keeping a context will be useful so we can
produce useful exception what() (which in this context will be used as
a response message to the operator) in a consistent manner, even if we
keep the micro try-catch. But at the moment that would probably be
over generalization. So, I'm okay with leaving the block for now, but
I'd suggest adding some comments like:

        // Note: we don't want to have such small try-catch block for each
        // specific error.  We may eventually want to introduce some unified
        // error handling framework as we have more configuration parameters.
        // See #1627 for the relevant discussion.
        InMemoryZoneFinder* imzf = NULL;

so we can revisit the point later.

  • I think it a good idea to introduce some intermediate class hierarchy in for exception classes in libdns++ (in fact there should be a ticket for that, I think), but I'd like to do it based on some consistent design consideration, rather than by introducing an intermediate class in ad hoc manner as we see the need for it. e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError?

=> isc::dns::NameParserError? => isc::dns::EmptyLabel?

This is perhaps good for another ticket too. I had a reason to add the NameParserException? parent for this bug. :) We can change it however we want.

I'll create a separate ticket for the redesign work. On thinking how
it would probably go, I now think NameParserError is okay.

Comments on the revised code:

  • I made a couple of more style fixes.
  • one test now fails:
    config_unittest.cc:335: Failure
    Expected: parser->build(Element::fromJSON( "[{\"type\": \"memory\"," "  \"zones\": [{\"origin\": \"example..com\"," "               \"file\": \"example.zone\"}]}]")) throws an exception of type EmptyLabel.
      Actual: it throws a different type.
    
  • also, I think we should add test cases for empty/missing zone or file name.
  • this could be simplified:
                imzf = new InMemoryZoneFinder(rrclass_,
                                              Name(origin->stringValue()));
    
    to:
                imzf = new InMemoryZoneFinder(rrclass_, Name(origin_txt));
    

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to muks

comment:14 in reply to: ↑ 12 Changed 8 years ago by muks

  • Owner changed from muks to jinmei

Replying to jinmei:

I agree on avoiding inner try-catch blocks, but am not sure if maintaining a context would be less work than prepending the context and rethrowing, when something is thrown.

I have a feeling that keeping a context will be useful so we can
produce useful exception what() (which in this context will be used as
a response message to the operator) in a consistent manner, even if we
keep the micro try-catch. But at the moment that would probably be
over generalization. So, I'm okay with leaving the block for now, but
I'd suggest adding some comments like:

        // Note: we don't want to have such small try-catch block for each
        // specific error.  We may eventually want to introduce some unified
        // error handling framework as we have more configuration parameters.
        // See #1627 for the relevant discussion.
        InMemoryZoneFinder* imzf = NULL;

so we can revisit the point later.

This comment has been added.

  • I think it a good idea to introduce some intermediate class hierarchy in for exception classes in libdns++ (in fact there should be a ticket for that, I think), but I'd like to do it based on some consistent design consideration, rather than by introducing an intermediate class in ad hoc manner as we see the need for it. e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError?

=> isc::dns::NameParserError? => isc::dns::EmptyLabel?

This is perhaps good for another ticket too. I had a reason to add the NameParserException? parent for this bug. :) We can change it however we want.

I'll create a separate ticket for the redesign work. On thinking how
it would probably go, I now think NameParserError is okay.

Cool :)

Comments on the revised code:

  • I made a couple of more style fixes.
  • one test now fails:
    config_unittest.cc:335: Failure
    Expected: parser->build(Element::fromJSON( "[{\"type\": \"memory\"," "  \"zones\": [{\"origin\": \"example..com\"," "               \"file\": \"example.zone\"}]}]")) throws an exception of type EmptyLabel.
      Actual: it throws a different type.
    

Oh yeah.. a different kind of exception now is thrown (as the NameParserException? is now caught). I must have missed running make check. Fixed. :)

  • also, I think we should add test cases for empty/missing zone or file name.

Added.

  • this could be simplified:
                imzf = new InMemoryZoneFinder(rrclass_,
                                              Name(origin->stringValue()));
    
    to:
                imzf = new InMemoryZoneFinder(rrclass_, Name(origin_txt));
    

Done for both origin and file.

Returning for re-review.

comment:15 Changed 8 years ago by jinmei

It now looks okay. Please merge.

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to muks

comment:17 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 1

comment:18 Changed 8 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Pushed to master:

* 1a4d0ae [1627] Add comment about use of try-catch blocks
* f95e202 [1627] Add a couple more testcases
* 6ac392c [master] Fix testcase name
* 376c01f [1627] Fix type of exception thrown in testcase
* 9d192aa [1627] Rewrite code to use local variables
* 88a8012 [1627] (editorial) folded long lines
* 77eddb3 [1627] (editorial) folded long lines
* 252fc35 [1627] Add the namestring that triggers the exception to the message
* 1a46994 [1627] Unify testcases for NameParserException checks
* c80eae9 [1627] Check for empty origin and file in config, and throw with more relevant error msg
* ae9eae2 [1627] fixed some style issues: untabify, position of '*', fold long lines.
* 48cd72d bug #1627: Update error message
* 06280cc bug #1627: Generate better error messages when origin is not specified
* af354fd bug #1627: Rewrite code to use NameParserException
* 48cfa00 bug #1627: Add common base class for name parser exceptions

Closing ticket. Thank you for the reviews jinmei.

Note: See TracTickets for help on using tickets.