Opened 7 years ago

Closed 7 years ago

#2925 closed defect (fixed)

The also_notify port should default to 53

Reported by: vorner Owned by: muks
Priority: medium Milestone: Sprint-20130806
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Low
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 1 Add Hours to Ticket: 0
Total Hours: 0.52 Internal?: no

Description (last modified by vorner)

If I add the also-notify element, the port defaults to 0, while it probably should to 53, while not many people will use a different port:

> config add Xfrout/also_notify
> config show Xfrout/also_notify
Xfrout/also_notify[0]/address	"..."	string	(modified)
Xfrout/also_notify[0]/port	53	integer	(modified)
Xfrout/also_notify[1]/address	""	string	(default)
Xfrout/also_notify[1]/port	0	integer	(default

Subtickets

Change History (14)

comment:1 Changed 7 years ago by vorner

  • Description modified (diff)

comment:2 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 1

comment:3 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:4 Changed 7 years ago by muks

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

comment:5 Changed 7 years ago by muks

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Up for review

comment:6 Changed 7 years ago by muks

I forgot.. this also needs a ChangeLog entry. I'll add one soon.

comment:7 Changed 7 years ago by muks

Here is the ChangeLog entry for this ticket:

+XXX.   [func]          muks
+       The default b10-xfrout also_notify port has been changed from
+       0 to 53.
+       (Trac# 2925, git ...)
+

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

I don't think this is working:

2013-07-29 10:02:25.643 INFO  [b10-xfrout.xfrout/19170] XFROUT_NEW_CONFIG_DONE Update xfrout configuration done
Traceback (most recent call last):
  File "/home/vorner/testing/bind10/libexec/bind10/b10-xfrout", line 1167, in <module>
    xfrout_server = XfroutServer()
  File "/home/vorner/testing/bind10/libexec/bind10/b10-xfrout", line 1030, in __init__
    self._start_notifier()
  File "/home/vorner/testing/bind10/libexec/bind10/b10-xfrout", line 1049, in _start_notifier
    self._notifier.add_slave(slave['address'], slave['port'])
KeyError: 'port'

This is when I configure the also_notify, do not set the port (so its left at 53), add a zone and then restart XfrOut? (unfortunately, there are problems in XfrOut? that don't let it work without restart for now).

comment:10 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 0.25

comment:11 in reply to: ↑ 9 Changed 7 years ago by muks

Replying to vorner:

   File "/home/vorner/testing/bind10/libexec/bind10/b10-xfrout", line 1049, in _start_notifier
     self._notifier.add_slave(slave['address'], slave['port'])
 KeyError: 'port'

I am able to reproduce this issue now. Checking.

comment:12 Changed 7 years ago by muks

  • Owner changed from muks to vorner

The config did not have the 'port' key if it was left as default. This is strange that the config library doesn't return the key as set to the default value if it doesn't exist.

This has been fixed now by checking for the keys explicitly. The bad part is that the default values are specified in yet another place. Back to review.

comment:13 Changed 7 years ago by vorner

  • Owner changed from vorner to muks
  • Total Hours changed from 0.25 to 0.52

Well, that's known misfeature of our configuration system. It is one of the thing that needs to be improved by the configuration rewrite.

I think it is OK to be merged now. In fact, it could be slightly simplified (with the dict's get method), but I think it doesn't matter.

comment:14 Changed 7 years ago by muks

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

Merged to master branch in commit 8acbf043daf590a9f2ad003e715cd4ffb0b3f979:

* 51e0b74 [2925] Check for address and port keys in config before using them
* 99e8782 [2925] Change default also_notify port to 53

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.