Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYNDNS: SSSD does not batch DDNS update requests #696

Closed
wants to merge 1 commit into from

Conversation

@thalman
Copy link
Contributor

commented Nov 15, 2018

SSSD includes a 'send' command in between each record modification
and does not batch DDNS update requests. This is problematic in
complex AD environments because those requests may not be processed
by the same server.

Now zone update is done in two steps - one per
protocol family. If dyndns_update_per_family is set
to false, update is performed in single step.

Resolves:
https://pagure.io/SSSD/sssd/issue/3829

Copy link
Member

left a comment

Just a few coding style nitpick, otherwise it looks good.

updateipv4,
updateipv6
);
} else {

This comment has been minimized.

Copy link
@pbrezina

pbrezina Dec 5, 2018

Member

This else can be removed.

This comment has been minimized.

Copy link
@thalman

thalman Dec 6, 2018

Author Contributor

done

"update add %s %d in PTR %s.\n",
ptr, ttl, hostname);
}
break;

This comment has been minimized.

Copy link
@pbrezina

pbrezina Dec 5, 2018

Member

It would read better if it is:

if (remove...) {
    updateipv4 = talloc...
    if (updateipv4 == NULL) {
        return NULL;
    }
}

updateipv4 = talloc...
if (updateipv4 == NULL) {
    return NULL;
}

Same for ipv6 case.

This comment has been minimized.

Copy link
@thalman

thalman Dec 6, 2018

Author Contributor

done

"%s"
"send\n",
updateipv4,
updateipv6);
} else {

This comment has been minimized.

Copy link
@pbrezina

pbrezina Dec 5, 2018

Member

No need for else here.

This comment has been minimized.

Copy link
@thalman

thalman Dec 6, 2018

Author Contributor

done

@pbrezina pbrezina self-assigned this Dec 5, 2018
@thalman thalman force-pushed the thalman:moje-dyndns branch 2 times, most recently from f9862a6 to f6abbe2 Dec 6, 2018
}

return update_msg;
if (update_per_family && updateipv4[0] && updateipv4[0]) {

This comment has been minimized.

Copy link
@pbrezina

pbrezina Dec 7, 2018

Member

The third condition should say updateipv6.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

There is one typo that needs to be fixed otherwise the patch looks good.

Code-style wise, we usually use an empty line after closing bracket, i.e.

  if (...) {
  }

  if (...) {
      ....
  }

  return xyz;

Since the patch need one more change, perhaps you could change this as well. Thank you.

SSSD includes a 'send' command in between each record modification
and does not batch DDNS update requests. This is problematic in
complex AD environments because those requests may not be processed
by the same server.

Now forward zone update is done in two steps - one per
protocol family. If dyndns_update_per_family is set
to false, update is performed in single step.

Resolves:
https://pagure.io/SSSD/sssd/issue/3829
@thalman thalman force-pushed the thalman:moje-dyndns branch from f6abbe2 to 16d355c Dec 7, 2018
@pbrezina

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

Ack. Thank you.

@pbrezina pbrezina added the Accepted label Dec 7, 2018
@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

@jhrozek jhrozek closed this Dec 10, 2018
@jhrozek jhrozek added the Pushed label Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.