Skip to content
Snippets Groups Projects

Zone api: zone update

Merged Zone api: zone update
zone_api-zone_update into master
Merged Dominik Taborsky requested to merge zone_api-zone_update into master

This is the initial merge request for zone-api. Changes are quite simple, mainly moving code from one place to the other and adding a bunch of functions to deal with it. So far the only advantage is a bit simpler code in the DDNS code and simpler API for it. The insides are not so nice yet as it requires further work on the changesets. Other than that this is merge-able as it is.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
src/knot/updates/zone-update.c
339 // Apply DNSSEC changeset
340 ret = apply_changeset_directly(new_contents, sec_ch);
341 if (ret != KNOT_EOK) {
342 return ret;
343 }
344
345 // Merge changesets
346 ret = changeset_merge(&update->change, sec_ch);
347 if (ret != KNOT_EOK) {
348 update_cleanup(sec_ch);
349 return ret;
350 }
351
352 // Plan next zone resign.
353 const time_t resign_time = zone_events_get_time(update->zone, ZONE_EVENT_DNSSEC);
354 if (refresh_at < resign_time) {
  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/updates/zone-update.c
    313 zone_contents_t *new_contents,
    314 changeset_t *sec_ch)
    315 {
    316 assert(update != NULL);
    317
    318 /*
    319 * Check if the UPDATE changed DNSKEYs or NSEC3PARAM.
    320 * If so, we have to sign the whole zone.
    321 */
    322 int ret = KNOT_EOK;
    323 uint32_t refresh_at = 0;
    324 const bool full_sign = changeset_empty(&update->change) ||
    325 dnskey_nsec3param_changed(update);
    326 if (full_sign) {
    327 ret = knot_dnssec_zone_sign(new_contents, sec_ch,
    328 ZONE_SIGN_KEEP_SOA_SERIAL,
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/updates/zone-update.c
    340 ret = apply_changeset_directly(new_contents, sec_ch);
    341 if (ret != KNOT_EOK) {
    342 return ret;
    343 }
    344
    345 // Merge changesets
    346 ret = changeset_merge(&update->change, sec_ch);
    347 if (ret != KNOT_EOK) {
    348 update_cleanup(sec_ch);
    349 return ret;
    350 }
    351
    352 // Plan next zone resign.
    353 const time_t resign_time = zone_events_get_time(update->zone, ZONE_EVENT_DNSSEC);
    354 if (refresh_at < resign_time) {
    355 zone_events_schedule_at(update->zone, ZONE_EVENT_DNSSEC, refresh_at);
    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      I don't like this kind of stuff. Deep calls of completely independent code within a function which does all the stuff while it's name suggest it will just sign the content. Nothing was said about scheduling. This is difficult to debug, difficult to understand, and impossible to test.

    • Dominik Taborsky
      Dominik Taborsky @dtaborsky ·
      Author Contributor

      When should this happen then? I think it is logical to schedule resign when you sign for the first time.

    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      The result of the signing (among other things) should be zone/changeset expiration time.

      If this was true, all the scheduling could be in one place without events magically rescheduling themselves.

  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/updates/zone-update.c
    373 "than current, serial %u -> %u",
    374 old_serial, new_serial);
    375 }
    376
    377 knot_soa_serial_set(&soa_cpy->rrs, new_serial);
    378 update->change.soa_to = soa_cpy;
    379
    380 return KNOT_EOK;
    381 }
    382
    383 int zone_update_commit(zone_update_t *update)
    384 {
    385 int ret = KNOT_EOK;
    386 zone_contents_t *new_contents = NULL;
    387
    388 if (update->flags & UPDATE_INCREMENTAL) {
  • Jan Včelák
  • Jan Včelák
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/updates/zone-update.h
    34 36 typedef struct {
    35 const zone_contents_t *zone; /*!< Zone being updated. */
    36 changeset_t *change; /*!< Changes we want to apply. */
    37 mm_ctx_t mm; /*!< Memory context used for intermediate nodes. */
    37 zone_t *zone; /*!< Zone being updated. */
    38 zone_contents_t *new_cont; /*!< New zone contents for full updates. */
    39 changeset_t change; /*!< Changes we want to apply. */
    40 uint8_t flags; /*!< Zone update flags. */
    41 mm_ctx_t mm; /*!< Memory context used for intermediate nodes. */
    38 42 } zone_update_t;
    39 43
    44 typedef enum {
    45 UPDATE_FULL = 1 << 0, /*!< Replace the old zone by a complete new one. */
    46 UPDATE_INCREMENTAL = 1 << 1, /*!< Apply changes to the old zone. */
    47 UPDATE_SIGN = 1 << 2, /*!< Sign the resulting zone. */
    48 UPDATE_DIFF = 1 << 3, /*!< In the case of full update, create a diff for journal. */
  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/updates/zone-update.h
    48 UPDATE_DIFF = 1 << 3, /*!< In the case of full update, create a diff for journal. */
    49 } zone_update_flags_t;
    50
    40 51 /*!
    41 52 * \brief Inits given zone update structure, new memory context is created.
    42 53 *
    43 54 * \param update Zone update structure to init.
    44 55 * \param zone Init with this zone.
    45 * \param change Init with this changeset. \todo will not be present in zone API
    56 * \param flags Flags to control the behavior of the update.
    57 *
    58 * \return KNOT_E*
    46 59 */
    47 void zone_update_init(zone_update_t *update, const zone_contents_t *zone,
    48 changeset_t *change);
    60 int zone_update_init(zone_update_t *update, zone_t *zone, zone_update_flags_t flags);
    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      This will be difficult to unit test because it's not clear what stuff in the zone structure this module uses. Zone content was an acceptable dependency. But from zone, you can fetch almost anything.

      I think the cause is the integrated signing, which should be somewhere else.

    • Dominik Taborsky
      Dominik Taborsky @dtaborsky ·
      Author Contributor

      The cause seems to be the resign planning and adding changes to journal. But from what I gathered, zone update was meant to take care of signing and journal so that the rest of the code doesn't have to. I don't see a problem here, I think this is intentional.

    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      No, we are not writing spaghetti intentionally. :pig:

    • Dominik Taborsky
      Dominik Taborsky @dtaborsky ·
      Author Contributor

      Your response is useless, sorry.

  • Jan Včelák
    Jan Včelák @jvcelak started a thread on the diff
    • src/knot/nameserver/update.c
    202 137 {
    203 138 assert(requests);
    204 139
    205 // Create DDNS change.
    206 changeset_t ddns_ch;
    207 int ret = changeset_init(&ddns_ch, zone->name);
    140 // Init zone update structure
    141 zone_update_t up;
    142 int ret = zone_update_init(&up, zone, UPDATE_INCREMENTAL | UPDATE_SIGN);
    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      Does it work with unsigned zones?

    • Dominik Taborsky
      Dominik Taborsky @dtaborsky ·
      Author Contributor

      You mean without specifying the UPDATE_SIGN flag? Yes, the signing is ignored in that case. It also takes into account the settings in conf db.

    • Jan Včelák
      Jan Včelák @jvcelak ·
      Contributor

      This code resides in answer processing. We know the zone and we know the zone configuration. Because this is the right place to know.

      So why the hell should zone_update_init() magically ignore UPDATE_SIGN flag based on the configuration, which is something the update should not stick it's nose into?

    • Dominik Taborsky
      Dominik Taborsky @dtaborsky ·
      Author Contributor

      The meaning of that flag is not "sign the zone", but "sign the zone if applicable". I get what you mean, but your approach would mean duplicated code whenever we might want to sign the zone.

      Also I could repeat all of your arguments and use them to support my (JK's) case.

  • Jan Včelák
    Jan Včelák @jvcelak ·
    Contributor

    @dtaborsky The changes look good. But some of the spaghetti code drives me crazy. Please, take a look at the comments and fix at least the simple problems. We can refactor this later, but whatever goes to master will be more difficult to change and won't get that much attention.

  • Dominik Taborsky Added 1 commit:

    Added 1 commit:

    • e7bb4693 - zone-api: minor review fixes.
  • Dominik Taborsky
    Dominik Taborsky @dtaborsky ·
    Author Contributor

    I am aware the code isn't perfect, but I've tried to make it as compact and simple as possible. The point of this first MR is to move working code to place where it should be, change function signatures as needed and prepare for followup work on other parts of Knot.

  • Dominik Taborsky Added 1 commit:

    Added 1 commit:

    • 3ad6883e - zone-api: test valgrind error in jenkins/clang-analyzer.
  • Dominik Taborsky Added 2 commits:

    Added 2 commits:

    • 788c26b3 - zone-api: minor review fixes.
    • 33cf8b64 - zone-api: commit cleanup, changesets merging update data.
  • Dominik Taborsky
    Dominik Taborsky @dtaborsky started a thread on the diff
    • src/knot/updates/changesets.c
    257 257 knot_rrset_free(&ch1->soa_to, NULL);
    258 258 ch1->soa_to = soa_copy;
    259 259
    260 ptrnode_t *n;
    261 WALK_LIST(n, ch2->old_data) {
    262 ptrlist_add(&ch1->old_data, (void *)n->d, NULL);
    263 };
    264 WALK_LIST(n, ch2->new_data) {
    265 ptrlist_add(&ch1->new_data, (void *)n->d, NULL);
    266 };
  • Jan Včelák
    Jan Včelák @jvcelak ·
    Contributor

    @dtaborsky Please, read the comments. Promise me that you will keep despaghettizing the code. And we can move on, merge this code, and look forward to the following merge requests.

  • Dominik Taborsky Added 1 commit:

    Added 1 commit:

    • 0e67ac5d - zone-api: get rid of RCU in zone_update, move contents switch.
  • Jan Včelák Status changed to merged

    Status changed to merged

    • Please register or sign in to reply