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

providers: Move hostid from ipa to sdap #237

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@hvenev
Copy link
Contributor

commented Apr 18, 2017

This just makes sss_ssh_knownhostsproxy work. There is no support for hostgroups (although hostgroups in ipa should continue working).

I've been using this for a few days with the ldap and krb5 providers and I haven't noticed any regressions. I haven't tested ipa and ad but all tests seem to pass.

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2017

Can one of the admins verify this patch?

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

ok to test

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

Hi,

I'm sorry the review takes so long. We're swamped with fixing bugs at the moment and the Easter holidays didn't help either.

I think since the patch moves quite a bit of code to the generic layer, it could be a bit generic as well. The current approach that moves all the IPA code to LDAP provider has a side-effect of exposing pieces of interface that are only (to the best of my knowledge) available only in IPA to the LDAP provider. For example I'm not sure if any other LDAP schema exposes something like UUID or memberof for hosts.

So what do you think about not exposing the part that fetches the host groups outside the IPA provider? The sdap_host_info_send/recv request would only return the host. Then, in the IPA provider, there would be a ipa_host_info_send/recv request that would first call sdap_host_info and then proceed to fetch the hostgroups as well. This could also mean that the only host-related options that would be publicly exposed in the LDAP documentation would be the host class, name and the public key objectclass.

@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

UUIDs are exposed in LDAP for users and groups. Not exposing memberOf would require having completely different attribute maps in LDAP and IPA. I don't think this can be done easily without duplicating absolutely everything.

Separating ipa_host_info_send/recv from sdap_host_info_send/recv will either result in more duplication or significantly more complicated code.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

commented May 2, 2017

I'm adding the "Changes requested" label as per @jhrozek's review.

Although it's an ongoing discussion it helps us to have a cleaner idea on what's new on pull-requests land.

@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2017

Sorry, I didn't mean to send this.

@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

This seems to be working for ldap. Can someone test ipa?

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

retest this, please

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

Removing the "Changes requested" label as the patch has been updated (v2).

providers: Move hostid from ipa to sdap, v2
In the ldap provider, all option names are renamed to ldap_host_*. In
the ipa provider the names haven't been changed.

Host lookups for both ipa and ldap are handled in the ldap provider.
sss_ssh_knownhostsproxy works but hostgroups are still only available
in the ipa provider.

I've also added some documentation for the ldap provider.
@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2017

When this patch is applied to 1.15.3, all tests are passing and things seem to be working fine when using ldap.

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2017

@hvenev. sorry, the "retest this, please" is a command that will trigger our CI to retest the patch set. :-)

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2017

@fidencio tests very likely will not pass due to expired certificates in unit test. Which was fixed in 2ccfa95. Patches would need to rebased on current master (or at leas 1.15.3) otherwise it will fail in jenkins. And that would be the same for any older pull request.

@hvenev hvenev closed this Jul 28, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

@hvenev, please, why this PR got closed?

Please, I'll re-open it and rebase your patches on top of current git master.

@hvenev hvenev reopened this Jul 29, 2017

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2017

Can one of the admins verify this patch?

@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2017

Sorry, I must have pushed the wrong thing.

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

ok to test

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

@hvenev, thanks a lot for re-opening this PR. :-)

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2017

@hvenev,
Could you rebase patch + also fix some issues. You can see POC https://github.com/lslebodn/sssd/tree/sdap-hostid_rebased

@jhrozek were your comments addressed in latesd version? #237 (comment)

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2017

Yes, but I think we should also work on tests. I also haven't ran any automated tests at all.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

@hvenev do you plan to work on this PR.
I know you were waiting for a long time for review and I am sorry for that.

@hvenev

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

I no longer have a real environment where I'm using sssd so I don't think I'll be working on this.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

Yes, but I think we should also work on tests. I also haven't ran any automated tests at all.

I rebased PR + ran automated test and there are not any regressions.

I'll push PR with your RB.

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

master:

1 similar comment
@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

master:

@lslebodn lslebodn closed this Feb 9, 2018

@lslebodn lslebodn added the Pushed label Feb 9, 2018

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.