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

Unify and simplify LDAP service discovery #2144

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@tiran
Copy link
Member

tiran commented Jul 12, 2018

Move LDAP service discovery and service definitions from
ipaserver.install to ipaserver. Simplify and unify different
implementations in favor of a single implementation.

Signed-off-by: Christian Heimes cheimes@redhat.com

Remark: The first iterations of this PR had active service discovery. This turns out to be too slow and unstable.

@tiran tiran requested a review from frasertweedale Jul 12, 2018

@tiran tiran force-pushed the tiran:active_server branch from 3bd0a87 to dc3824c Jul 12, 2018

@Tiboris Tiboris added the WIP label Jul 12, 2018

@Tiboris Tiboris changed the title [WIP] Find active servers Find active servers Jul 12, 2018

@tiran tiran force-pushed the tiran:active_server branch 3 times, most recently from 2574b79 to d983e58 Jul 12, 2018

@rcritten rcritten added the re-run label Aug 16, 2018

@@ -1555,7 +1566,7 @@ def install(installer):
# Enable configured services and update DNS SRV records
service.enable_services(config.host_name)
api.Command.dns_update_system_records()
ca_servers = service.find_providing_servers('CA', api.Backend.ldap2, api)
ca_servers = find_providing_servers('CA', api.Backend.ldap2, api)

This comment has been minimized.

Copy link
@rcritten

rcritten Aug 16, 2018

Contributor

Does it make more sense to make find_providing_servers() public or use the server_role mechanism to see if there is more than one CA installed?

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Aug 16, 2018

A couple of the host tests are raising an Internal Error but this seems to be on the right track.

@tiran tiran force-pushed the tiran:active_server branch from d983e58 to b058382 Aug 23, 2018

@tiran tiran removed the needs rebase label Aug 23, 2018

@tiran tiran force-pushed the tiran:active_server branch from b058382 to 24d6689 Sep 26, 2018

@pvoborni

This comment has been minimized.

Copy link
Member

pvoborni commented Oct 5, 2018

Current server picking mechanism which is used also in vault plugin produces a bug https://pagure.io/freeipa/issue/7691 . Comment https://pagure.io/freeipa/issue/7691#comment-535233 analyses the situation. In short, by default, standard users don't have read permissions for service information in etc=masters subtree. Thus method like find_active_server will return None later causing the probably bad behavior of get_client in kra backend.

It makes sense to think about the context of this bug also in this refactoring, though the bug might be fixed separately.

Also, just by looking at the code, it seems that this refactoring aims to partly resolve: https://fedorahosted.org/freeipa/ticket/4557

@tiran tiran force-pushed the tiran:active_server branch 2 times, most recently from 3a05265 to a5152ba Nov 13, 2018

@tiran tiran changed the title Find active servers Unify and simplify LDAP service discovery Nov 13, 2018

@tiran tiran added needs review ipa-next and removed WIP labels Nov 13, 2018

@tiran tiran force-pushed the tiran:active_server branch from a5152ba to 9cb9f48 Nov 13, 2018

@tiran tiran force-pushed the tiran:active_server branch 3 times, most recently from 6b04b8c to e81d976 Nov 13, 2018

@tiran

This comment has been minimized.

Copy link
Member Author

tiran commented Nov 14, 2018

RunPytest Failed to publish artifacts from ci-large-26, rerun

@tiran tiran added the re-run label Nov 14, 2018

@freeipa-pr-ci freeipa-pr-ci removed the re-run label Nov 14, 2018

@fcami

This comment has been minimized.

Copy link
Contributor

fcami commented Nov 14, 2018

Full review done & this looks good to me (without actually running the code).

@tiran @rcritten indeed maybe find_providing_server(s) would be best in serverroles (serverroles.py) alongside server_role_search and friends?

Proposed changes (error messages and comments) live in:
https://github.com/fcami/freeipa/tree/active_server

@fcami

This comment has been minimized.

Copy link
Contributor

fcami commented Nov 14, 2018

@tiran @rcritten - replying to myself I'd vote for merging this as-is and open another issue to investigate whether moving find_providing_server(s) to serverroles is worthwhile.

@tiran could you remove 'LDAP' from the commit message, as your work is about any kind of service discovery?

host_name = host_name.lower()
try:
servers.remove(host_name)
except ValueError:

This comment has been minimized.

Copy link
@rcritten

rcritten Nov 14, 2018

Contributor

If a server is in the preferred list but isn't in the available list then it will be silently dropped. Should this be handled in some way?

This comment has been minimized.

Copy link
@tiran

tiran Nov 14, 2018

Author Member

The case may occur if a server has an explicit ca_host in its default.conf but the machine has been decommissioned. It's an uncommon case. I'd rather not fail here if IPA can recover automatically. How about log.warn?

This comment has been minimized.

Copy link
@rcritten

rcritten Nov 14, 2018

Contributor

Yes, that'd be fine.

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Nov 14, 2018

I thought about the suggested move to servroles.py as well but it doesn't quite fit there. I'm not super-keen on a new file in ipaserver but I don't know where else it might go other than ipaserver/install.

@tiran

This comment has been minimized.

Copy link
Member Author

tiran commented Nov 14, 2018

@fcami It's service discovery from LDAP, not of LDAP. For most services, IPA uses DNS SRV based discovery. LDAP service discovery is only used in some special cases, e.g. because IPA doesn't have SRV records for CA and KRA.

@rcritten yeah, ipaserver.install feels wrong. Modules are cheap and I prefer to put related code into one module rather than creating another 3k misc, stuff, or util module.

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Nov 14, 2018

I'm ok with putting it into ipaserver. I'm trying to be cautious about moving code around in case others are using the internal API (yeah, unsupported, etc but it does break things from time to time).

@fcami

This comment has been minimized.

Copy link
Contributor

fcami commented Nov 15, 2018

@tiran - oh right, I read it wrong, thanks for the explanation.

@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Nov 15, 2018

This change LGTM for master.

Unify and simplify LDAP service discovery
Move LDAP service discovery and service definitions from
ipaserver.install to ipaserver. Simplify and unify different
implementations in favor of a single implementation.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@rcritten

This comment has been minimized.

Copy link
Contributor

rcritten commented Nov 20, 2018

Works in my testing, ack.

@rcritten rcritten added ack and removed needs review labels Nov 20, 2018

@tiran

This comment has been minimized.

Copy link
Member Author

tiran commented Nov 21, 2018

master:

  • 8decef3 Unify and simplify LDAP service discovery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.