Opened 9 years ago

Closed 9 years ago

#811 closed enhancement (complete)

Put TSIG field into list of zones we should manage.

Reported by: stephen Owned by: jelte
Priority: very high Milestone: Sprint-20110531
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: tsig
Estimated Difficulty: 2.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description


Subtickets

Attachments (1)

xfrin-simplify.diff (2.1 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by shane

  • billable changed from 1 to 0
  • Estimated Difficulty changed from 0.0 to 2
  • Milestone changed from Year 3 Task Backlog to Sprint-20110419
  • Priority changed from major to critical

comment:2 Changed 9 years ago by shane

  • Priority changed from critical to blocker

comment:3 Changed 9 years ago by jelte

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

comment:4 Changed 9 years ago by shane

  • Defect Severity set to N/A
  • Feature Depending on Ticket set to tsig
  • Sub-Project set to DNS

comment:5 follow-up: Changed 9 years ago by jelte

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

Ready for review; the changes are in trac811_new (i started over after the botched attempt in zonemgr, as discussed on the call this week), last commit is cd79c2fae07f7b1a8d2e2f501488de7a2d11eac5

I have done a little bit more than strictly necessary for this ticket; since i was adding zone-specific config anyway, i thought i'd move master_addr and master_port in there as well.

Note that the usage this makes it is not final! There are several things we intend to change in the future, two notable things are:
tsig_key is currently a direct string representation of the entire key, and not a name or reference to a tsig key config item as done in a different ticket (unsure what the status of that is, I think we need a wrap-up ticket to tie the final loose ends for tsig)
the way it is configured now is that, similar to Auth/listen_on, you have a list containing dicts; in this case a list of 'zone info' dicts, with a name, master_addr, master_port, and tsig_key. When we have something like ACL's or peers, this will also change here to use that.

About the usage:

Xfrin now has a list config item called zones; which is intially empty. I think the easiest way to add config is to add an empty item first then set the values:

> config show Xfrin
Xfrin/transfers_in	10	integer	(default)
Xfrin/zones	[]	list	(default)
> config add Xfrin/zones
> config show Xfrin/zones
Xfrin/zones[0]/name	""	string	(default)
Xfrin/zones[0]/class	"IN"	string	(default)
Xfrin/zones[0]/master_addr	""	string	(default)
Xfrin/zones[0]/master_port	53	integer	(default)
Xfrin/zones[0]/tsig_key	null	string	
> config set Xfrin/zones[0]/name tjeb.nl
> config set Xfrin/zones[0]/master_addr 178.18.82.80
> config commit

Now you can force a retransfer with just the zone name:

> Xfrin retransfer tjeb.nl
"zone xfrin is started"

You can of course still override the configured settings by specifying them as arguments to retransfer. If the zone is not configured, and you do not specify any arguments, it will use the defaults (as it used to, 127.0.0.1/53, i am wondering if we shouldn't just remove that behaviour and error in this case)

About the code:

added a dict _zones to Xfrin, which stores the new ZoneInfo? objects (which contain the items as shown above), keyed on name and class.

For xfrout, I only added the specification for tsig_key, as it wasn't clear to me yet how to specify slaves, and since xfrout does not currently use tsig_key anyway.

comment:6 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

higher level points

  • we'll need a changelog entry for this.
  • are we going to manage the zone info in various places (zonemgr, xfrin, auth, and perhaps xfrout)? Or would there be "master" source of the information (in zonemgr?) and others just use a copy (of a subset of it)?
  • I tend to agree that we should reject the case of unspecified master address. at least the current default (127.0.0.1) doesn't make sense.
  • shouldn't we be able to specify multiple masters? is it a plan for future extension? (I'm okay with that)
  • I couldn't reproduce the step you showed:
    > config show Xfrin
    Xfrin/transfers_in	10	integer	(default)
    Xfrin/zones	[]	list	(default)
    > config add Xfrin/zones
    > config show Xfrin/zones
    Xfrin/zones[0]/name	""	string	(default)
    [...]
    

For me, "config add Xfrin/zones" didn't seem to make any effect (but I
was able to add zone info by config set Xfrin/zones[0]/..., so it was
not a big problem)

xfrin.py

  • I'd avoid using hardcoded magic values ('IN') in the main source code like this:
            self.class_str = config_data.get('class') or 'IN'
    
    I'd at least like them to be defined as a module specific constant at the head of the source file. further, I wonder whether this default should be in the spec file and xfrin should simply refer to it?
  • why do we bother with trailing dot like this?
            # add the root dot if the user forgot
            if len(self.name) > 0 and self.name[-1] != '.':
                self.name += '.'
    
    Wouldn't it be better to store it as Name objects? On a related note, what if the specified zone is not a valid domain name (e.g. "badname..example")? If we convert it to Name at an earlier stage, we can detect the error at that point.

Probably the same point applies to tsig key, too.

  • _add_zone_info: should we worry about overriding existing info? (probably not as this is called in the context of "replace everything", but just checking)
  • config_handler doesn't provide the "strong exception guarantee". for example, if we add a few zones successfully and then find a syntax error for another zone, an incomplete zone list will remain in xfrin. Should we care about such a case?

xfrin_test.py

  • in test_command_handler_notify_known_zone, I'd check if the configured address/port is used instead of those specified in the command (when they are different)
  • we'll probably need test cases for invalid zone names (and perhaps invalid tsig key spec too)
  • I'd use 192.0.2.X instead of 1.1.1.1; likewise I'd use test.example.com instead of test com.

xfrin.spec

  • can't zones[]/class,master_port be optional? their defaults seem to be quite typical.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:9 in reply to: ↑ 7 ; follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

higher level points

  • we'll need a changelog entry for this.

oh yes, i propose:
[func] Xfrin configuration now contains a master server setting per zone,
and the 'general' master_addr has been removed. Both xfrin and xfrout
also have a tsig_key setting (per zone).

  • are we going to manage the zone info in various places (zonemgr, xfrin, auth, and perhaps xfrout)? Or would there be "master" source of the information (in zonemgr?) and others just use a copy (of a subset of it)?

Initially i thought the latter, and i started out modifying zonemgr. But
then it got unclear (as discussed in the call last week), and I restarted
doing it on a module-that-actually-needs-the-data basis (i.e. you set
masters in xfrin, slaves in xfrout, etc). Zonemgr is strange imo, and i
am half thinking that in the direction it's currently going we might as
well put all its logic in xfrin anyway. There is another change i want
that makes this a bit more 'usable', that is to not have a List as the
'zones' config, but a direct dict (where the keys are the zone names), so
that essentially, you are not configuring the 'zone' as such in xfrin and
xfrout (and zonemgr), but you configure xfrin-specific settings *for* the
zone, if you catch my drift.

Side note, though the current bindctl modifies internal config values
directly, afaik we are still thinking about having an abstraction between
the UI and the actual settings (so that settings live where they are used,
but on the configuration side they may be grouped on a conceptual basis),
but that's further away I guess. And I do not have a clear view on how
this would work with extensibility yet.

  • I tend to agree that we should reject the case of unspecified master address. at least the current default (127.0.0.1) doesn't make sense.

Yes, and there may not be any settings that make sense (well, perhaps the
soa value, but you don't have that yet). It now errors if retransfer is
given without a master argument and the zone is not known in the
configuration.

  • shouldn't we be able to specify multiple masters? is it a plan for future extension? (I'm okay with that)

yes. But since that would also require some more advanced
fallback-to-next master logic, I omitted it for this task.

  • I couldn't reproduce the step you showed:
    > config show Xfrin
    Xfrin/transfers_in	10	integer	(default)
    Xfrin/zones	[]	list	(default)
    > config add Xfrin/zones
    > config show Xfrin/zones
    Xfrin/zones[0]/name	""	string	(default)
    [...]
    

For me, "config add Xfrin/zones" didn't seem to make any effect (but I
was able to add zone info by config set Xfrin/zones[0]/..., so it was
not a big problem)

really? for me it fails if i try to set values for an item that was not
added yet. Is the above a transcript from your session or a copy of my
example?

xfrin.py

Ack, done

  • I'd avoid using hardcoded magic values ('IN') in the main source code like this:
            self.class_str = config_data.get('class') or 'IN'
    
    I'd at least like them to be defined as a module specific constant at the head of the source file. further, I wonder whether this default should be in the spec file and xfrin should simply refer to it?

Oh yes you are right we should take it from spec.
Changed this. Added a ConfigData?.get_default_value() so modules don't
have to poke around in specification internals.

  • why do we bother with trailing dot like this?
            # add the root dot if the user forgot
            if len(self.name) > 0 and self.name[-1] != '.':
                self.name += '.'
    
    Wouldn't it be better to store it as Name objects? On a related note, what if the specified zone is not a valid domain name (e.g. "badname..example")? If we convert it to Name at an earlier stage, we can detect the error at that point.

Probably the same point applies to tsig key, too.

Good points, done. Class too btw.

  • _add_zone_info: should we worry about overriding existing info? (probably not as this is called in the context of "replace everything", but just checking)

While internally it would simply replace it indeed, it could lead to
unexpected behaviour, so I added a check for it.

  • config_handler doesn't provide the "strong exception guarantee". for example, if we add a few zones successfully and then find a syntax error for another zone, an incomplete zone list will remain in xfrin. Should we care about such a case?

yeah, i changed it to restore the previous values now

xfrin_test.py

  • in test_command_handler_notify_known_zone, I'd check if the configured address/port is used instead of those specified in the command (when they are different)

Added this check for 'retransfer'. In 'notify' the value is actually not
used yet (and with reason; we should only use the address/port from
whatever we got the notify from if it matches one of the values we have in
our configuration. As long as we can only set one value, it makes no
sense to use the one given. In fact I believe it would be a security hole
if we did. Added a few extram comments about this to xfrin.py.in).

  • we'll probably need test cases for invalid zone names (and perhaps invalid tsig key spec too)

ack, done, although i did not add test for every possible bad zone name.

  • I'd use 192.0.2.X instead of 1.1.1.1; likewise I'd use test.example.com instead of test com.

ohyeah, i just copied the values but changed them now, also added some
checks to see that the config is actually taken over.

xfrin.spec

  • can't zones[]/class,master_port be optional? their defaults seem to be quite typical.

Kind of depends on what we mean with 'optional' here; in my view, 'optional' does not mean that the user does not need to set it explicitely; it means that it can be omitted entirely (with different behaviour than when it is not omitted); for instance, tsig_key is optional, if not set, no TSIG is used. But we always need a port number for a master, so port number is not optional. We don't want the user to have to set it explicitely, so if this is not done it will default to 53.

comment:10 in reply to: ↑ 9 Changed 9 years ago by jinmei

Replying to jelte:

I'll first reply to selected parts of your response. For some of the
others, see comments on the revised code. For other parts that are
not covered either in the explicit response or in the code comment, I
have no further comment.

Replying to jinmei:

  • are we going to manage the zone info in various places (zonemgr, xfrin, auth, and perhaps xfrout)? Or would there be "master" source of the information (in zonemgr?) and others just use a copy (of a subset of it)?

Initially i thought the latter, and i started out modifying zonemgr. But
then it got unclear (as discussed in the call last week), and I restarted
doing it on a module-that-actually-needs-the-data basis (i.e. you set
masters in xfrin, slaves in xfrout, etc). Zonemgr is strange imo, and i
am half thinking that in the direction it's currently going we might as
well put all its logic in xfrin anyway. There is another change i want
that makes this a bit more 'usable', that is to not have a List as the
'zones' config, but a direct dict (where the keys are the zone names), so
that essentially, you are not configuring the 'zone' as such in xfrin and
xfrout (and zonemgr), but you configure xfrin-specific settings *for* the
zone, if you catch my drift.

I think I vaguely understand the idea. In any case I'm okay with the
current way. As we have more concrete examples we'll have a more
clear idea about how we should manage the information.

  • I couldn't reproduce the step you showed:

[...]

For me, "config add Xfrin/zones" didn't seem to make any effect (but I
was able to add zone info by config set Xfrin/zones[0]/..., so it was
not a big problem)

really? for me it fails if i try to set values for an item that was not
added yet. Is the above a transcript from your session or a copy of my
example?

Maybe it was my misoperation. I tried it again and this time I got
the same result.

xfrin_test.py

  • in test_command_handler_notify_known_zone, I'd check if the configured address/port is used instead of those specified in the command (when they are different)

Added this check for 'retransfer'. In 'notify' the value is actually not
used yet (and with reason; we should only use the address/port from
whatever we got the notify from if it matches one of the values we have in
our configuration. As long as we can only set one value, it makes no
sense to use the one given. In fact I believe it would be a security hole
if we did. Added a few extram comments about this to xfrin.py.in).

Yes, I understand the security implication (it was me who first
noticed that in a previous xfrin implementation and disabled that:-).
My original comment was intended to say "we should use configured
address/port (for security reasons) and we should test to confirm
that".

Now, specific comments on the revised branch:

About default config/command parameters

(This part is long and may be beyond the scope of this ticket. I'm
okay with deferring the discussion/development for this to a separate
ticket)

After looking at it more closely, I'm now feeling it's not clear who
is responsible for applying the default or for checking the existence
of mandatory parameters. It first occurred to me "why do we need to
write down these checks or apply the default when something is
unspecified within xfrin? Isn't it a job of ccsession?" Then I
noticed various related issues:

  • the python version of ModuleCCSession class doesn't validate incoming commands unlike the C++ version. This might be because some commands (like "notify") are "internal" and aren't listed in the spec file, but IMO we should centralize the command syntax information (and if we don't want to hide something from general UI, add a property field for that purpose in the spec file) rather than having the application act differently.
  • ModuleCCSession actually doesn't support completing config/command for (mandatory) parameters that has the default when they are unspecified. The latest version provides get_default_value(), but the caller still has to do the job of applying the default when something is missing.
  • bindctl doesn't seem to use the spec default while it checks missing mandatory parameter by itself (but this would be far beyond the scope of this ticket)
  • It's not clear what it means if a parameter is "optional" but has (meaningful) default. In the case of xfrin, they are "zone_class" and "port" for the retransfer command, and are essentially mandatory parameters, and xfrin uses hardocoded defaults (which happen to be consistent with the ones in the spec file). For "master", xfrin uses the address configured in zone when it's missing, and it's actually optional, but the default value will never make sense (it's an invalid string as an IPv6/v4 address).

I think a better solution is:

  • add syntax validation to ModuleCCSession.check_command_without_recvmsg(). if necessary, extend the spec file so that it will include "hidden" commands, too (as noted above).
  • add an ability to ModuleCCSession to complete config/command params when some mandatory params with defaults are missing. Maybe it can be part of check_command_without_recvmsg() and/or it can provide a separate API, say, ModuleCCSession.complete_command(args), which would apply all defaults for unspecified mandatory parameters.
  • the application generally relies on the check/completion of ModuleCCSession. In particular, the application doesn't care about how to apply default values when something that has a default is missing. but in terms of the defense in depth the app may at least want to check if a mandatory parameter is really provided.

xfrin_test.py

  • MockCC.get_default_value: maybe it's better to use TEST_RRCLASS_STR instead of hardcoded 'IN'
  • MockXfrin?.xfrin_start: wouldn't it be better to pass the given check_soa to the super class rather than resetting it to True?
            return Xfrin.xfrin_start(self, zone_name, rrclass, db_file,
                                     master_addrinfo, tsig_key_str,
                                     check_soa=True)
    
    (i.e., removing '=True')
  • MockXfrin?.xfrin_start: the following attributes don't seem to be used: xfrin_started_zone_name, xfrin_started_rrclass, xfrin_started_tsig_key_str
  • test_command_handler_retransfer_short_command2: this is now equivalent to test_command_handler_retransfer_short_command1 and redundant. Previously it added a trailing dot to the zone name to see if the behavior is the same. You may still want to do this check somewhere, but since this test now expects a failure (due to a different reason) I think such a test should go to somewhere else. as a result this test case should probably be just removed.
  • test_command_handler_retransfer_quota: better to use TEST_MASTER_IPV4_ADDRESS than hardcoding '127.0.0.1'
  • test_command_handler_notify_known_zone: the line number in the comment doesn't make sense to me. Such hardcoded line numbers are quit susceptible to further changes, so I'd suggest using a bit stabler tag (e.g. a method name)
            # would at this point not make sense, see the TODO in
            # xfrin.py.in:542)
    
  • I think it would be better to check a case of invalid tsig string. It should be a trivial copy-and-modify of existing tests, so I wrote it myself and pushed it.

xfrin.py

(These are actually specific instances of the general issue I
mentioned at the beginning, so depending on how we handle it we may
defer them to a separate ticket)

  • _check_zone_class: do we need to consider the case where

zone_class_str is None? If it's a non optional config parameter,
this seems to be treated as an error (just like the case where the
zone name or master address isn't specified).

  • Likewise, do we need to have DEFAULT_MASTER_PORT in Xfrin and use it as a last resort? Can't we basically rely on the validity check of the config module and simply treat it as an error if it's unexpectedly unspecified?

ZoneInfo?

  • is it okay to omit module_cc (in which case None is used)? no one seems to benefit from omitting it, and if it's omitted and 'class' isn't specified in config_data, it will cause disruption.
  • is there any reason for storing tsigkey as string? This way we need to convert it to a TSIGKey object every time we use it.
  • similarly, do we need to maintain self.master_addr_str and self.master_port_str separately from self.master_{addr, port}?

Xfrin

  • _get_zone_info: it seems the if-else clause could be simplified:
            return self._zones.get(key)
    
  • is there any reason we cannot directly use pairs of (Name, RRClass) for the key of _zones{}? It seems to be an unnecessary overhead to convert them to string every time.


  • do we need the indirection of _get_all_zone_info()? it seems to me we can simply use _zones in config_handler() (both are in the same class), which is (currently) the only user of this method. Same for _set_all_zone_info().
  • _parse_zone_name_and_class and _parse_master_and_port do a redundant job of extracting and validating RRclass.
  • _parse_zone_name_and_class: as commented above, it would be better

to treat the case of '!rrclass' as an error. (note: this is
another instance of the "general issues")

  • _parse_zone_name_and_class: I also noticed you use 'not XXX' to mean "not specified", rather than "is None". This happens when the given string is actually "", rather than "unspecified". But since these will be rejected via the Name()/RRClass() constructors, we could actually unify the check (assuming we trust the default RRClass provided by the config module + xfrin spec).
  • _parse_master_and_port(): same point for the port: can't we treat the case of 'port is None' as an error?

config_data.py

we probably need a test for get_default_value inside the config module.
but note also that we may want to introduce a different interface to
handle config/command defaults (see the discussion at the beginning).

ChangeLog?

  • I think we need to add "*" because it changes the UI quite a lot.
  • Some short example of how to add a zone may be helpful.
  • I didn't understand the "xfrout" part: "Both xfrin and xfrout also have a tsig_key setting (per zone)."

Other minor points

  • As we briefly discussed on jabber, I noticed the copyright year was changed from 2009 to 2011. There doesn't seem to be consensus about it or it's not even clear whether we should have a consensus, so I'm just making a note about this and leave it to you.
  • I made some minor editorial changes and pushed them. I believe the intent is clear enough so I don't explain details.

comment:11 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 Changed 9 years ago by jinmei

One more thing. I didn't notice there were changes to xfrout, too.
From a quick look at them, the same discussion seems to apply to that code.

comment:13 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

skipping the initial part, all good points (and i had plans myself for
some of them as well, especially relating to the unspecified commands,
which was actually a surprise to me and we should certainly not have that,
but outside the scope of this ticket indeed, will defer this to list or
ticket).

xfrin_test.py

  • MockCC.get_default_value: maybe it's better to use TEST_RRCLASS_STR instead of hardcoded 'IN'

ack

  • MockXfrin?.xfrin_start: wouldn't it be better to pass the given check_soa to the super class rather than resetting it to True?
            return Xfrin.xfrin_start(self, zone_name, rrclass, db_file,
                                     master_addrinfo, tsig_key_str,
                                     check_soa=True)
    
    (i.e., removing '=True')

woops, of course.

  • MockXfrin?.xfrin_start: the following attributes don't seem to be used: xfrin_started_zone_name, xfrin_started_rrclass, xfrin_started_tsig_key_str

oh yeah i wasn't sure we needed them in our set of tests, removed them

  • test_command_handler_retransfer_short_command2: this is now equivalent to test_command_handler_retransfer_short_command1 and redundant. Previously it added a trailing dot to the zone name to see if the behavior is the same. You may still want to do this check somewhere, but since this test now expects a failure (due to a different reason) I think such a test should go to somewhere else. as a result this test case should probably be just removed.

yes, removed short_command2, renamed 3 to 2, and added a copy of 3 that
does the same test without the trailing dot.

  • test_command_handler_retransfer_quota: better to use TEST_MASTER_IPV4_ADDRESS than hardcoding '127.0.0.1'

ack

  • test_command_handler_notify_known_zone: the line number in the comment doesn't make sense to me. Such hardcoded line numbers are quit susceptible to further changes, so I'd suggest using a bit stabler tag (e.g. a method name)
            # would at this point not make sense, see the TODO in
            # xfrin.py.in:542)
    

ok

  • I think it would be better to check a case of invalid tsig string. It should be a trivial copy-and-modify of existing tests, so I wrote it myself and pushed it.

ok thanks

xfrin.py

(These are actually specific instances of the general issue I
mentioned at the beginning, so depending on how we handle it we may
defer them to a separate ticket)

  • _check_zone_class: do we need to consider the case where

zone_class_str is None? If it's a non optional config parameter,
this seems to be treated as an error (just like the case where the
zone name or master address isn't specified).

  • Likewise, do we need to have DEFAULT_MASTER_PORT in Xfrin and use it as a last resort? Can't we basically rely on the validity check of the config module and simply treat it as an error if it's unexpectedly unspecified?

We do, at the very least, we do for now. Not for config (since it is
indeed non-optional), but for command parsing (where the zone class is
optional, and we don't have that same defaults stuff (yet)).

I've added a module constand DEFAULT_ZONE_CLASS, and added a comment to
both these constants why we need them (now), and a TODO to have a solution
similar to what we have for config options.

ZoneInfo?

  • is it okay to omit module_cc (in which case None is used)? no one seems to benefit from omitting it, and if it's omitted and 'class' isn't specified in config_data, it will cause disruption.

ack, removed default None

  • is there any reason for storing tsigkey as string? This way we need to convert it to a TSIGKey object every time we use it.
  • similarly, do we need to maintain self.master_addr_str and self.master_port_str separately from self.master_{addr, port}?

These values are now no longer stored as strings. I refactored the
ZoneInfo? class a bit, as the initializer got way too long )it now calls a
set of set_foo(foo_str) functions that do any checking/default getting.

name_str and class_str are still stored separately, added TODO for those
(but it's a bit tied into the rest of the (original) code)

Xfrin

  • _get_zone_info: it seems the if-else clause could be simplified:
            return self._zones.get(key)
    

ack. local variable key is then also not necessary anymore

  • is there any reason we cannot directly use pairs of (Name, RRClass) for the key of _zones{}? It seems to be an unnecessary overhead to convert them to string every time.


fundamentally, there isn't. However, our python wrappers would need to be
extended to do this. I propose we make a ticket for that too (i don't
think it's too hard, just out of scope)

  • do we need the indirection of _get_all_zone_info()? it seems to me we can simply use _zones in config_handler() (both are in the same class), which is (currently) the only user of this method. Same for _set_all_zone_info().

no, we don't. It was mostly to keep operations on _zones on the same
level. Removed them. Also noticed that 'the other' value could still be
updated even if the config as a whole was rejected, so it now reverts that
too (if we add more options that may fail, we will probably want to
generalize this, either through a backup_config object, a pending_config
object, and/or a set of calls (not to mention that we also need a bigger
three-phase-commit type thing))

  • _parse_zone_name_and_class and _parse_master_and_port do a redundant job of extracting and validating RRclass.

it was not entirely the same, it raised a different exception (due to
different usage contexts), however, this made me think
XfrinConfigException? was a bad name, as it was really only about zone
information data, so I changed the exception to XfrinZoneInfoException?
('something is wrong with the given zone information'). And the _parse
function now calls the same check function.

  • _parse_zone_name_and_class: as commented above, it would be better

to treat the case of '!rrclass' as an error. (note: this is
another instance of the "general issues")

also see earlier

  • _parse_zone_name_and_class: I also noticed you use 'not XXX' to mean "not specified", rather than "is None". This happens when the given string is actually "", rather than "unspecified". But since these will be rejected via the Name()/RRClass() constructors,

according to git, that's your code, btw :)
changed it to use is None

we could actually unify the check (assuming we trust the default
RRClass provided by the config module + xfrin spec).

  • _parse_master_and_port(): same point for the port: can't we treat the case of 'port is None' as an error?

yes. But both depend on the defaults thingy for commands, so right now, no.

btw, with the values no longer stored in zoneinfo as strings, i changed
this method to only parse them when given as a command argument (and
otherwise directly use the zoneinfo members), and removed build_addr_info.

It still needs to convert back to string at the end, but i figured i
refactored enough as a side effect for this ticket, and am considering
ways to use ZoneInfo? for command arguments as well (and then pass that to
the actual xfrin functions instead of these ad-hoc tuples), but ha no
complete idea for this yet.

config_data.py

we probably need a test for get_default_value inside the config module.
but note also that we may want to introduce a different interface to
handle config/command defaults (see the discussion at the beginning).

ChangeLog?

  • I think we need to add "*" because it changes the UI quite a lot.
  • Some short example of how to add a zone may be helpful.

ok

  • I didn't understand the "xfrout" part: "Both xfrin and xfrout also have a tsig_key setting (per zone)."

well I added tsig_key to xfrout, but it did not have any other tsig
related code yet, so it was mostly just the addition in the spec, as i did
not want to add too much code that was not used yet. Updated it to at
least convert the tsig key.

it also had no config-error checking whatsoever so i added something kind
of similar to what we have in xfrin.

The code here is not 'as done' as in xfrin, and what there is shows quite
a few similarities, so we may want to refactor it later. Having said
that, I do expect them to diverge, and i would like to first know how
before we do that :)

Other minor points

  • As we briefly discussed on jabber, I noticed the copyright year was changed from 2009 to 2011. There doesn't seem to be consensus about it or it's not even clear whether we should have a consensus, so I'm just making a note about this and leave it to you.

made it into 2009-2011 (and 2010-2011), don't really care too much, but we
may want to have a guideline on this

  • I made some minor editorial changes and pushed them. I believe the intent is clear enough so I don't explain details.

ok thanks

comment:14 in reply to: ↑ 13 Changed 9 years ago by jinmei

Replying to jelte:

Xfrin

  • is there any reason we cannot directly use pairs of (Name, RRClass) for the key of _zones{}? It seems to be an unnecessary overhead to convert them to string every time.

fundamentally, there isn't. However, our python wrappers would need to be
extended to do this. I propose we make a ticket for that too (i don't
think it's too hard, just out of scope)

I can live with that. But I just realized there are some minor case
problems if we keep using strings as keys: case-sensitiveness.
Consider we store a zone named "example.com", and then a user tries to
trigger a retransfer specifying "EXAMPLE.COM". Not actually checked,
but I suspect it will fail.

One quick-hack fix would be to convert the name to down-cased one
within _check_zone_name:

def _check_zone_name(zone_name_str):
    """Checks if the given zone name is a valid domain name, and returns
    it as a Name object. Raises an XfrinException if it is not."""
    try:
	# Later we'll compare zone names as string via to_text(), so
	# to avoid any unexpected comparison result due to case issues
	# we down case the name here.
        return Name(zone_name_str, True)

(and I guess we need a test case for it)

Or, if we declare this is quite minor and less likely to happen in
practice (and defer everything to the later ticket), I'm also okay
with that.

  • _parse_zone_name_and_class and _parse_master_and_port do a redundant job of extracting and validating RRclass.

it was not entirely the same, it raised a different exception (due to
different usage contexts), however, this made me think

Hmm, okay, but if the string is invalid the first exception will be
raised so the end result should be identical (right)?

XfrinConfigException? was a bad name, as it was really only about zone
information data, so I changed the exception to XfrinZoneInfoException?
('something is wrong with the given zone information'). And the _parse
function now calls the same check function.

And, with that change, I think we can really remove the redundancy.
I'm attaching a proposed patch.

  • _parse_zone_name_and_class: I also noticed you use 'not XXX' to mean "not specified", rather than "is None". This happens when the given string is actually "", rather than "unspecified". But since these will be rejected via the Name()/RRClass() constructors,

according to git, that's your code, btw :)

Oh, is it? I admit I myself sometimes am confused about None and not:-)

Comments on the revised code follow:

xfrin_test.py

  • in test_command_handler_zones(), zonesN should better be renamed to "config1" or "xfrin_config1", etc, because now they are not only for zones.

xfrin.py.in

  • XfrinZoneInfoException?: "when a command does not have the required or bad arguments" doesn't seem to parse reasonably. "when a command does not have a required argument or has bad arguments"?
  • in set_master_port, should we worry about if the default spec value is out of range for a valid port number (e.g. >= 216)? same for set_zone_class. (Sorry I should have pointed these before.) I'm okay with ignoring it as an unlikely scenario (at least for now).
  • Zoneinfo.set_name: is this TODO still active? From a quick check it seems we can safely remove this line:
            self.name_str = name_str
    

Other points

  • xfrout tests failed for me. We could fix it, but based on the discussion at the sprint planning, I'd rather suggest deferring the whole xfrout part to a separate ticket and concentrate on finishing xfrin. at the moment I've not closely looked at the xfrout code.
  • my previous comment about missing test for get_default_value() doesn't seem to be addressed.

Changed 9 years ago by jinmei

comment:15 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:16 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

Xfrin

<snip>

One quick-hack fix would be to convert the name to down-cased one
within _check_zone_name:

(and I guess we need a test case for it)

Or, if we declare this is quite minor and less likely to happen in
practice (and defer everything to the later ticket), I'm also okay
with that.

easy and minor enough to fix now (this is where the earlier smallish refactors start to pay off :))

XfrinConfigException? was a bad name, as it was really only about zone
information data, so I changed the exception to XfrinZoneInfoException?
('something is wrong with the given zone information'). And the _parse
function now calls the same check function.

And, with that change, I think we can really remove the redundancy.
I'm attaching a proposed patch.

ok, thanks, applied it

  • _parse_zone_name_and_class: I also noticed you use 'not XXX' to mean "not specified", rather than "is None". This happens when the given string is actually "", rather than "unspecified". But since these will be rejected via the Name()/RRClass() constructors,

according to git, that's your code, btw :)

Oh, is it? I admit I myself sometimes am confused about None and not:-)

i shan't say i never make the mistake (and i certainly read over it
easily), just happens that i was explaining this difference to another dev
last week too (so the 'you' got me a bit there ;))

Comments on the revised code follow:

xfrin_test.py

  • in test_command_handler_zones(), zonesN should better be renamed to "config1" or "xfrin_config1", etc, because now they are not only for zones.

ok

xfrin.py.in

  • XfrinZoneInfoException?: "when a command does not have the required or bad arguments" doesn't seem to parse reasonably. "when a command does not have a required argument or has bad arguments"?

ok

  • in set_master_port, should we worry about if the default spec value is out of range for a valid port number (e.g. >= 216)? same for set_zone_class. (Sorry I should have pointed these before.) I'm okay with ignoring it as an unlikely scenario (at least for now).

hmm. let's defer that too, not that i'm lazy (well ok I am, but i may have a reason here:), but this makes me think that maybe we do need a set of generalized check-calls like is_valid_port_number, etc. Preferably ones that do not raise, but do convert to specific type if necessary (and do provide the error if they can't).

  • Zoneinfo.set_name: is this TODO still active? From a quick check it seems we can safely remove this line:
            self.name_str = name_str
    

oh hey indeed, missed that one

Other points

  • xfrout tests failed for me. We could fix it, but based on the discussion at the sprint planning, I'd rather suggest deferring the whole xfrout part to a separate ticket and concentrate on finishing xfrin. at the moment I've not closely looked at the xfrout code.

ok, i reverted the changes there (btw i suspect your failed test was a missing dependency that caused the configure-time-created test file to not be recreated)

  • my previous comment about missing test for get_default_value() doesn't seem to be addressed.

woops, sorry, added

comment:17 in reply to: ↑ 16 Changed 9 years ago by jinmei

Replying to jelte:

I've added one small test case to test_get_default_value and pushed it
to the repo.

Other than this it looks okay. Please merge (and please also create
tickets for the deferred things).

comment:18 Changed 9 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:19 Changed 9 years ago by jelte

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

branch merged, tickets created (#941, #942, #943, and #944), closing ticket

Note: See TracTickets for help on using tickets.