Opened 8 years ago

Closed 8 years ago

#1414 closed defect (fixed)

Zonemgr/secondary_zones[]/class required

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20111220
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket: none
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 5.82 Internal?: no

Description

When I tried to add a new secondary zone for zonemgr, I found that
I needed to specify the zone's RR class explicitly, even if it has the
reasonable default of "IN":

I first added a new entry:

> config add Zonemgr/secondary_zones
> config show Zonemgr/secondary_zones
Zonemgr/secondary_zones[0]/class	"IN"	string	(modified)
Zonemgr/secondary_zones[0]/name	"nui.org"	string	(modified)
Zonemgr/secondary_zones[1]/class	"IN"	string	(default)
Zonemgr/secondary_zones[1]/name	""	string	(default)

so far it's okay.

Then I set secondary_zones[1]/name:

> config show Zonemgr/secondary_zones
Zonemgr/secondary_zones[0]/class	"IN"	string	(modified)
Zonemgr/secondary_zones[0]/name	"nui.org"	string	(modified)
Zonemgr/secondary_zones[1]/class	"IN"	string	(default)
Zonemgr/secondary_zones[1]/name	"dyn.kame.net"	string	(modified)

still okay.

Since secondary_zones[1]/class has the default and I'm okay with it,
I thought I could just commit it, but it failed:

> config commit
Error: 'class'
Configuration not committed

and the error message was quite helpless (except that it seems to
indicate something related to 'class' (but the word 'class' has
a broad meaning)).

If I explicitly specified the RR class, commit succeeded:

> config set Zonemgr/secondary_zones[1]/class IN
> config commit

This is counter intuitive at the best. At the very least the error
message should be more helpful.

Subtickets

Attachments (1)

zonemgr.diff (5.3 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by jreed

Duplicate of #897 (and #960)?

comment:2 Changed 8 years ago by shane

  • Feature Depending on Ticket set to none

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jelte

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

comment:6 Changed 8 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 2

the method that handles the configuration update needs access to the ccsession (for the get_default_value() so we don't have to hardcode a value here).

Ideally, we should change the basic 'new_config' object we pass around (globally), so that is has the value, and its spec (including its default value). However, that would be way too big a change for this ticket. So for now I've only added the moduleccsession to the update config method (and added a minimal fake one in the tests), and left the rest of the code as much intact as possible.

Oh I've also improved the error if no zone name was given (i.e. if you do config add secondary_zones; config commit). It was a similar confusing error caused by trying to directly access a nonexistent key in a dict).

comment:7 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:8 follow-up: Changed 8 years ago by jinmei

zonemgr

  • not really for this branch, but I think we should rather check the given name is a valid domain name (same for the RR class, btw) by converting it into a Name object, then dump it to text (lower-cased with ending period):
                        # Be tolerant to sclerotic users who forget the final dot
                        if name[-1] != '.':
                            name = name + '.'
    
    xfrout does this, btw. See _get_transfer_acl(). It can be deferred to a separate ticket, though. I mentioned it simply because I noticed it while I reading the patch.

zonemgr_test

  • I'd integrate FakeConfig? and FakeCCSession (and "MyCCSession" defined and used somewhere else in the test). See the attached diff for concrete ideas. The point is:
    • eliminate hardcoding as much as possible
    • integrate functionality FakeConfig? and MyCCSession into FakeCCSession, thus eliminating the need for the former(s) completely.
  • FakeCCSession constructor: this doesn't seem to be necessary:
            self.config = FakeConfig()
    
    (in the sample patch I removed this line, too)
  • (this is not really for this branch) While thinking about this further refactoring, I realized this part didn't seem to make sense:
            # This one does not exist
            config['secondary_zones'] = \
                zone_list_from_name_classes(["example.net", "CH"])
            self.zone_refresh.update_config_data(config, self.cc_session)
            self.assertFalse(("example.net.", "CH") in
                             self.zone_refresh._zonemgr_refresh_info)
            # Simply skip loading soa for the zone, the other configs should be updated successful
            self.assertFalse(("example.net.", "IN") in
                             self.zone_refresh._zonemgr_refresh_info)
    
    (this is after refactoring, but I believe it preserves the test semantics) First, I suspect ["example.net", "CH"] should have been [("example.net", "CH")]. But then the subsequent test failed. I couldn't even be sure what exactly these sequences tried to test, so I simply kept the original semantics even though it didn't make sense to me.

Others

  • I've made a couple of minor editorial cleanups.
  • I believe we need a changelog entry for this.

Changed 8 years ago by jinmei

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:10 in reply to: ↑ 8 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

zonemgr

  • not really for this branch, but I think we should rather check the given name is a valid domain name (same for the RR class, btw) by converting it into a Name object, then dump it to text (lower-cased with ending period):
                        # Be tolerant to sclerotic users who forget the final dot
                        if name[-1] != '.':
                            name = name + '.'
    
    xfrout does this, btw. See _get_transfer_acl(). It can be deferred to a separate ticket, though. I mentioned it simply because I noticed it while I reading the patch.

Might as well fix it now, added to-and-from conversion, with exception catch to provide a slightly better error (and to convert to one exception). Added a few tests as well (but not extensively for every possible bad dname)

zonemgr_test

  • I'd integrate FakeConfig? and FakeCCSession (and "MyCCSession" defined and used somewhere else in the test). See the attached diff for concrete ideas. The point is:
    • eliminate hardcoding as much as possible
    • integrate functionality FakeConfig? and MyCCSession into FakeCCSession, thus eliminating the need for the former(s) completely.

I applied your patch as-is. It did require one extra addition; it introduces SPECFILE_LOCATION, which is fine, but then we must also set B10_FROM_BUILD in the Makefile test call, so that it calculates the correct path for it. Added that too.

  • (this is not really for this branch) While thinking about this further refactoring, I realized this part didn't seem to make sense:
            # This one does not exist
            config['secondary_zones'] = \
                zone_list_from_name_classes(["example.net", "CH"])
            self.zone_refresh.update_config_data(config, self.cc_session)
            self.assertFalse(("example.net.", "CH") in
                             self.zone_refresh._zonemgr_refresh_info)
            # Simply skip loading soa for the zone, the other configs should be updated successful
            self.assertFalse(("example.net.", "IN") in
                             self.zone_refresh._zonemgr_refresh_info)
    
    (this is after refactoring, but I believe it preserves the test semantics) First, I suspect ["example.net", "CH"] should have been [("example.net", "CH")]. But then the subsequent test failed. I couldn't even be sure what exactly these sequences tried to test, so I simply kept the original semantics even though it didn't make sense to me.

I have no idea whether it was intentional, but what it did was add to one-letter names with one-letter classes, and then making sure it didn't set the original one. Yeah that's weird, and most probably wrong.

Again, might as well fix it now; I've changed the test; what it now does is set the 'normal' name example.net, but class CH (and use the weirdly named but conveniently defined ZONE_NAME_CLASS1_CH for that), then checks whether the zonemgr did not make IN of it.

Whatever the test was supposed to do, it probably changed :p

Removed the duplicate test after 'skip soa'. No idea what that was for.

Others

  • I've made a couple of minor editorial cleanups.

thanks

  • I believe we need a changelog entry for this.

[bug] jelte
Fixed a bug where adding Zonemgr/secondary_zones without explicitely setting the class value of the added zone resulted in a cryptic error in bindctl ("Error: class"). It will now correctly default to IN if not set. This also adds better checks on the name value, and a better error when it is bad.

comment:11 Changed 8 years ago by jelte

  • Total Hours changed from 2 to 4

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

Replying to jelte:

xfrout does this, btw. See _get_transfer_acl(). It can be deferred
to a separate ticket, though. I mentioned it simply because I
noticed it while I reading the patch.

Might as well fix it now, added to-and-from conversion, with exception catch to provide a slightly better error (and to convert to one exception). Added a few tests as well (but not extensively for every possible bad dname)

Looks okay, but to be even more perfect, I guess we need to do:

  • perform syntax check on the class, too (reject "BADCLASS", etc)
  • confirm whether the comparison is case-insensitive ('example.COM/in' should be treated as 'example.com./IN'.)

BTW, I believe checking only one bad name is okay at this level. I
also think it's okay to use the catch-all Exception (ideally we'd like
to introduce module level exception and/or all isc-exceptions, but
currently we don't have them, so Exception would be the best
compromise)

I applied your patch as-is. It did require one extra addition; it introduces SPECFILE_LOCATION, which is fine, but then we must also set B10_FROM_BUILD in the Makefile test call, so that it calculates the correct path for it. Added that too.

Ah, right, good catch (about B10_FROM_BUILD).

I have no idea whether it was intentional, but what it did was add to one-letter names with one-letter classes, and then making sure it didn't set the original one. Yeah that's weird, and most probably wrong.

Again, might as well fix it now; I've changed the test; what it now does is set the 'normal' name example.net, but class CH (and use the weirdly named but conveniently defined ZONE_NAME_CLASS1_CH for that), then checks whether the zonemgr did not make IN of it.

Whatever the test was supposed to do, it probably changed :p

Removed the duplicate test after 'skip soa'. No idea what that was for.

Okay.

  • I believe we need a changelog entry for this.

[bug] jelte
Fixed a bug where adding Zonemgr/secondary_zones without explicitely setting the class value of the added zone resulted in a cryptic error in bindctl ("Error: class"). It will now correctly default to IN if not set. This also adds better checks on the name value, and a better error when it is bad.

This looks fine.

comment:13 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

Replying to jinmei:

Might as well fix it now, added to-and-from conversion, with exception catch to provide a slightly better error (and to convert to one exception). Added a few tests as well (but not extensively for every possible bad dname)

Looks okay, but to be even more perfect, I guess we need to do:

  • perform syntax check on the class, too (reject "BADCLASS", etc)
  • confirm whether the comparison is case-insensitive ('example.COM/in' should be treated as 'example.com./IN'.)

ok, done

updated changelog (added "and class")

[bug] jelte
Fixed a bug where adding Zonemgr/secondary_zones without explicitely setting the class value of the added zone resulted in a cryptic error in bindctl ("Error: class"). It will now correctly default to IN if not set. This also adds better checks on the name and class values, and better errors if they are bad.

comment:15 Changed 8 years ago by jelte

BTW, Jeremy noted that in the class check, the new exception was caught in an object but not used, Fixed that too

comment:16 in reply to: ↑ 14 Changed 8 years ago by jinmei

Replying to jelte:

I made one additional change to the case-(in)sensitiveness test and
pushed it. I hope it's acceptable.

Other than this it looks okay. Please merge.

comment:17 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:18 Changed 8 years ago by jinmei

  • Total Hours changed from 4 to 5.82

comment:19 Changed 8 years ago by jelte

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

Thanks, merged (with your last addition). Closing ticket

Note: See TracTickets for help on using tickets.