Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#1643 closed defect (fixed)

TSIG configuration syntax should be as consistent as possible for auth and xfrout

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20120306
Component: configuration Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 11.23 Internal?: no

Description

See this thread at bind10-users:
https://lists.isc.org/pipermail/bind10-users/2012-January/000164.html

I propose we use the same syntax as that used in the global
configuration, i.e., tsig_keys for xfrout. We'll also need to update
documentation (at least bind10-guide, possibly also the man page)

Subtickets

Attachments (1)

python-tests-coverage.sh (1.1 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by jinmei

There is another suggestion on configuration interface consistency
in that bind10-users thread:
https://lists.isc.org/pipermail/bind10-users/2012-January/000165.html

Since the original issue should be quite easy to address, it probably
makes sense to address the other issue in this ticket, too.

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 5

comment:3 Changed 8 years ago by jelte

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

comment:4 Changed 8 years ago by vorner

  • Owner set to vorner
  • Status changed from new to accepted

comment:5 Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

It should be ready for review. The branch contains some framework changes, namely:

  • Ability to add a remote config by its name instead of a spec file.
  • Port of C++ tsig keyring, keeping up to date to the global keyring (it was not wrapped, because we have a separate incompatible implementation of the config session in python).
  • The actual changes to xfrout.

If needed be, the steps could be reviewed separately (with borders between them at c540444d6220133977db22de4c0adc116afd24a6 and 73506bcbd64a043f7e66c193a15b2d09a0a47bf0, the last 2 commits are fixes when trying it out and documentation).

I guess it should have a changelog, more because user needs to remove old configuration option from xfrout:

[func]*		vorner
Xfrout now uses the global TSIG keyring, instead of its own. This means the
keys need to be set only once (in tsig_keys/keys). However, the old
configuration of Xfrout/tsig_kes need to be removed for Xfrout to work.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

The branch generally looks okay. I've made a few minor editorial
changes including some trivial corrections in code comments. I have
some other small things to note, which follow:

general

Ideally, I'd like to confirm actual zone transfer using TSIG via
system tests. Since configuration involves multiple processes, and
also we now have additional indirection to the global key ring,
I'm afraid it's more likely to have a system-level bug that cannot be
detected via unittests. This could be a separate deferred ticket
though.

ccsession.py

  • Please explain a bit more rationale about this change. It's not crystal clear. (maybe we need to add the implication to the add_remote_config() description)
        * The config callback should be called after the module is ready.
    
  • not really for this branch, but _add_remote_config_internal seems to ignore some error cases:
    • non-0 rcode or value is None
    • when validate_config() fails
  • add_remote_config_by_name: 'env' doesn't seem to be used and can be _:
                answer, env = self._session.group_recvmsg(False, seq)
    
    (not a big deal though)

ccsession_test.py

  • _internal_remote_module_with_custom_config: this comment (still)

refers to add_remote_config even if the method is generalized:

        # override the default config value for "item1".  add_remote_config()
  • error cases do not seem to be tested like this one:
                    if module_spec.get_module_name() != module_name:
                        raise ModuleCCSessionError("Module name mismatch: " +
                                                   module_name + " and " +
                                                   module_spec.get_module_name())
    
    (you may also want to run pycoverage)

tsig_keyring.py

  • is it okay not to provide any logging?
  • Updater._keyring could be "declared" as private, i.e., 'keyring'? Same for _update().
  • for naming consistency, "keyring()" may be better named "get_keyring()"?
  • did you actually mean "duplicate", not "duplicity"?
        Raised when a key can not be added. This usually means there's a
        duplicity.
    
    in case you mean it: according to my dictionary "duplicity" is uncountable, so it should be "there's duplicity". Same comment applies to the comment in test_update_bad.
  • do you mean "i.e." instead of "eg."? ('eg. = for example' doesn't make much sense to me in this context)
            The parameters are there just to match the signature which
            the callback should have (eg. they are ignored).
    

tsig_keyring_test.py

  • is it an okay behavior to do init_keyring() after deinit? If so, should we test that case too?
  • I'd test either/both of loading/updating a keyring that has multiple keys. I'd also test whether we can make the keyring empty on update.

xfrout_test.py

  • Shouldn't we check if the global keyring is actually used in xfrout?

bind10-guide

  • do you mean 'para', instead of 'param'?
        <param>Both Xfrout and Auth will use the system wide keyring to check
        TSIGs in the incomming messages and to sign responses.</param>
    

changelog

  • s/tsig_kes/tsig_key_ring/
    However, the old
    configuration of Xfrout/tsig_kes need to be removed for Xfrout to
    work.
    

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:9 in reply to: ↑ 7 ; follow-ups: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Ideally, I'd like to confirm actual zone transfer using TSIG via
system tests. Since configuration involves multiple processes, and
also we now have additional indirection to the global key ring,
I'm afraid it's more likely to have a system-level bug that cannot be
detected via unittests. This could be a separate deferred ticket
though.

OK, I'll add the ticket when merging this.

  • Please explain a bit more rationale about this change. It's not crystal clear. (maybe we need to add the implication to the add_remote_config() description)
        * The config callback should be called after the module is ready.
    

The _add_remote_config_internal did call the callback if there were non-default values. However, it did before setting the internal structures, so it complain the remote config is not set up yet when used from inside the callback. So I just switched the order there. There's no implication to the outside, it was just a bug caught by system tests, so I fixed it.

  • not really for this branch, but _add_remote_config_internal seems to ignore some error cases:
    • non-0 rcode or value is None

Being none is allowed, if there's no config set yet. But I do check the others now.

}}}

  • error cases do not seem to be tested like this one:
                    if module_spec.get_module_name() != module_name:
                        raise ModuleCCSessionError("Module name mismatch: " +
                                                   module_name + " and " +
                                                   module_spec.get_module_name())
    
    (you may also want to run pycoverage)

I tried running pycoverage, but it fails for me with this error:

Running test: edns_python_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/dns/python/tests/edns_python_test.py", line 18, in <module>
    from pydnspp import *
ImportError: dynamic module does not define init function (initpydnspp)
make[7]: *** [check-local] Error 1

This seems to happen reliably with all wrapper modules, and I need to look into it sometime. I didn't want to hold this ticket because of it, though.

changelog

  • s/tsig_kes/tsig_key_ring/
    However, the old
    configuration of Xfrout/tsig_kes need to be removed for Xfrout to
    work.
    

Hmm, yes, you're right. I'll update it when merging.

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

Replying to vorner:

I think you can merge the branch.

Some followup and minor additional comments.

  • I made a few minor editorial cleanups.
  • the additional checks for _add_remote_config_internal look good, but these cases don't seem to be tested:
                if rcode == 0:
                    if value != None:
                        if module_spec.validate_config(False, value):
                            module_cfg.set_local_config(value)
                            call_callback = True
                        else:
                            raise ModuleCCSessionError("Bad config data for " +
                                                       module_name + ": " +
                                                       str(value))
                else:
                    raise ModuleCCSessionError("Failure requesting remote " +
                                               "configuration data for " +
                                               module_name)
    
  • I just noticed you could avoid one more temporary variable in tsig_keyring.py:
            (data, _) = self.__session.get_remote_config_value('tsig_keys',
                                                                     'keys')
    

I tried running pycoverage, but it fails for me with this error:

Running test: edns_python_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/dns/python/tests/edns_python_test.py", line 18, in <module>
    from pydnspp import *
ImportError: dynamic module does not define init function (initpydnspp)
make[7]: *** [check-local] Error 1

This seems to happen reliably with all wrapper modules, and I need to look into it sometime. I didn't want to hold this ticket because of it, though.

I don't think we should fix edns_python_test within this ticket, but
you don't have to run all coverage tests to see the coverage for the
specific branch (apparently you seemed to do that and fail). I use
the attached script and run it as follows:

% cd bind10-1643/src/lib/python/isc/config/tests
% python-tests-coverage.sh -b 1643 ccsession_test.py

In case you're interested, I've copied the results at:
http://bind10.isc.org/~jinmei/python-coverage/

The coverage of ccsession.py seems very bad, but it looks like missing
many parts that should actually be covered by the tests. Maybe the
use of a mock class confuses the coverage tool.

Changed 8 years ago by jinmei

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:12 Changed 8 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 11.23

Thank you, closing.

comment:13 in reply to: ↑ 9 Changed 7 years ago by jreed

Replying to vorner:

(you may also want to run pycoverage)

I tried running pycoverage, but it fails for me with this error:

Running test: edns_python_test.py
Traceback (most recent call last):
  File "/home/vorner/work/bind10/src/lib/dns/python/tests/edns_python_test.py", line 18, in <module>
    from pydnspp import *
ImportError: dynamic module does not define init function (initpydnspp)
make[7]: *** [check-local] Error 1

This seems to happen reliably with all wrapper modules, and I need to look into it sometime. I didn't want to hold this ticket because of it, though.

I think this is caused by using python coverage from a 2.x version instead of 3.x version. I had same problem until I deinstalled py27-coverage-3.5.2 and then installed py31-coverage-3.5.2 .

Note: See TracTickets for help on using tickets.