BIND 10 #1627: Error: Server configuration failed: incomplete textual name

[BIND 10 Development]

#1627: Error: Server configuration failed: incomplete textual name
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  UnAssigned
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  bind-  |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  2
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 muks]:
 > Replying to [comment:7 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:

 {{{#!diff
      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:
 {{{#!c++
     } 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.
 {{{#!c++
                     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:

 {{{#!c++
 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");
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1627#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development