Do not build the local provider by default #611

fidencio · 2018-07-02T11:25:03Z

Folks,

This series is the first attempt to avoid building the local provider by default. With we only build it conditionally and the default is set to not build it. The problems:

integration tests depend on the local provider: For this, I've changed our configure line, specifying to enable the local provider;
some unit tests depend on local provider: I've changed some tests (please, take a careful look at those in order to be sure I'm not invalidating the tests) and worked them around so they're still valid with both scenarios (building or not building the local provider);

My personal preference would be to start using the files provider in our tests instead of using the "local" one. However, I've faced some issues related to the amount of users being find in some sysdb tests (ping me, we can discuss this either here or in the #sssd channel) (maybe because it also finds the local users/groups created on my machine?).

Again, this is a first attempt, let's discuss improvements needed :-)

pbrezina · 2018-07-02T12:19:06Z

My personal preference would be to start using the files provider in our tests instead of using the "local" one. However, I've faced some issues related to the amount of users being find in some sysdb tests (ping me, we can discuss this either here or in the #sssd channel) (maybe because it also finds the local users/groups created on my machine?).

Good idea. How did you configure the files provider? You can you passwd_files and group_files to set the files you want to read, also you can use SSS_FILES_PASSWD and SSS_FILES_GROUP environment variable to overwrite standard file names.

jhrozek · 2018-07-10T09:50:20Z

Thanks, this is a good start. We also want to remove the whole pysss.local interface and don't build the sss_* tools like sss_useradd and so on.

If you want to remove the usage of the local provider from tests, does it make sense to keep the code at all using the is_local_provider functions? Remember the code is not lost forever, we can always ressurect it from git history.

jhrozek · 2018-07-10T09:51:20Z

btw I'm not sold either way myself, on one hand the local provider might be handy for tests, on the other hand I haven't used it in a long time myself. So I would not be completely mad if it goes away.

jhrozek · 2018-07-10T09:51:48Z

src/tests/cmocka/test_sysdb_views.c

+            gid = 0;
+        } else {
+            uid = 1234 + i;
+            gid = 1234 + i;


I would even say we can drop the conditional and just use the else branch.

jhrozek · 2018-07-10T09:52:16Z

src/tests/sysdb-tests.c

+        gid = 0;
+    } else {
+        uid = 1234;
+        gid = 1234;


I would even drop the if branch and only use the else branch.

jhrozek · 2018-07-10T09:53:38Z

src/tests/sysdb-tests.c

@@ -1095,6 +1105,9 @@ START_TEST(test_user_group_by_name)
     * ldap provider differently with auto_private_groups.
     */
    test_ctx->domain->provider = discard_const_p(char, "ldap");
+    if (!local_provider_is_built()) {


On the other hand, this test might be handy. Since some functions differ in functionality with mpg enabled domain, I think we want to have a test. But I think just adding a unit test with mpg=true is simpler and more error proof than having an if-else branch based on how sssd is built.

jhrozek · 2018-07-10T09:54:28Z

src/tests/sysdb-tests.c

@@ -1827,6 +1844,7 @@ START_TEST (test_sysdb_remove_nonexistent_group)
 }
 END_TEST

+#ifdef BUILD_LOCAL_PROVIDER


OTOH, I think this ifdef is fine because the get_new_id function is only useful for the local provider.

jhrozek · 2018-07-10T09:56:05Z

About the sysdb tests, can you paste what issues you faced here? Or push a branch? I could see issues with not matching number of groups returned because the local provider is mpg-enabled by default, but I don't know what the issue with users could be.

pbrezina · 2018-07-10T16:10:18Z

Thanks, this is a good start. We also want to remove the whole pysss.local interface and don't build the sss_* tools like sss_useradd and so on.

If you want to remove the usage of the local provider from tests, does it make sense to keep the code at all using the is_local_provider functions? Remember the code is not lost forever, we can always ressurect it from git history.

btw I'm not sold either way myself, on one hand the local provider might be handy for tests, on the other hand I haven't used it in a long time myself. So I would not be completely mad if it goes away.

Definitely drop all the code related to local provider. It makes the code complicated since it is a special case. It is also very easy to forget it.

If it will be needed again, we can implement a simple backend for local provider just returning EOK from the dbus handlers. But I have literally never used it for testing ever. In addition, it does not really tests anything today since it never goes to backend and requires special casing in responders.

fidencio · 2018-07-18T15:25:07Z

@jhrozek, @pbrezina,

I haven't addressed any of the comments made, yet.
I've pushed the patchset containing the most of the changes theoretically needed.

We can discuss test by test in our phone meeting Tomorrow.

fidencio · 2018-07-31T12:04:53Z

It's been discussed on IRC a few times already and let me try to make it explicit here.
I do need some help in order to make the tests that are failing to pass. Those tests theoretically only test APIs but there seem to be some changes depending on whether they are or are not used "local" provider.

The help needed here and asked on IRC a few times before is in order to figure out the changes that have to be done.

fidencio · 2018-08-02T13:42:27Z

I've updated the patches, @pbrezina helped me with the most part of the issues I was facing.

There are still an issue in the ldap_id_cleanup() test as the test fails when ran by the first time but passes if ran again after a minute or two.

Summing up, there's still work to be done in this PR, thus i"m adding the "Changes requested" label.

jhrozek · 2018-08-03T08:22:55Z

Do you need help with triaging the failure?

fidencio · 2018-08-03T14:21:10Z

@jhrozek, that would be really appreciated.

What I've found so far is that when not building the local provider the ldap_id_cleanup test is bailing out because when trying to cleanup the groups the search done doesn't return any records from the cache.

I've checked that those records are present in the timestamp cache and if searched there we'd bea ble to move forward. However, the search is done in the cache itself.

So, what I'm trying to understand here is why the cache has not been updated (following the timestamp cache) when not using the local provider.

fidencio · 2018-08-06T17:29:21Z

So, thanks to both @jhrozek and @pbrezina the tests are now passing!

I've decided to re-order/re-organize the patches and all comments made should be addressed.

The series now contain:

A bunch of commits changing the tests to work with the "files" provider
- It includes a few fixes needed in order to make ldap_id_cleanup test to pass;
One commit making the build of the "local" provider conditional
One commit removing pysss.local interface.

jhrozek · 2018-08-12T20:23:00Z

I wish we had more time to nuke the code completely, but not building it by default is a good start. We can remove the stuff completely in 2.1

jhrozek · 2018-08-12T20:23:35Z

I'll ack once coverity finishes

jhrozek · 2018-08-13T09:28:42Z

Coverity and CI were OK

Also start to consider the "files" provider when cleaning up the files. This change will let us start to moving to "files" provider in our test suite. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Apart from the obvious change from "local" to "files" and from "LOCAL" to "FILES", we're also passing an on-the-fly created uid/gid to the sysdb_store_user() function in order to avoid calling sysdb_get_new_id(), which only should be used with the "local" provider. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Apart from the obvious change from "local" to "files" and from "LOCAL" to "FILES", we're also passing a static uid/gid to the sysdb_add_user() function in order to avoid calling sysdb_get_new_id(), which only should be used with the "local" provider. Another change doneis to explicitly set mpg to true as it was enabled by default when using the "local" provider. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Apart from the obvious change from "local" to "files" and from "LOCAL" to "FILES", we also had to change the cleanup function as this test suite doesn't rely on test_multidom_suite_cleanup(). Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Only a simple change from "local" to "files" was required. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

When trying to get rid of the "local" provider references in ldap_id_cleanup tests by switching to "files" provider, it's been noticed that the linearized dn wasn't coming sanitized, which would make the test to fail. While I'm not sure here's the right place to have it fixed, this is the simplest proposal I could come up with. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

The reason for specifically passing a ts_subfilter is because when using a provider that relies on a cache and on a timestamp cache, the search done each cache is different. The difference in the search is that on timestamp cache we add a (dateExpireTimestamp <= XXX), but it shouldn't be added to the cache search. This commit is needed in order to have the ldap_id_cleanup test running when switching from local provider (which has no timestamp cache) to the files provider (which exposed this issue). Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. The only changes requred were the obvious change from "local" to "files" and from "LOCAL" to "FILES". Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Let's just replace "LOCAL_SYSDB_FILE" for NULL as This test suite doesn't create local sysdb file. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

…ution_order_ Let's take advantage of the files provider and start to get rid of the local provider references in our code. Let's just replace "LOCAL_SYSDB_FILE" for NULL as This test suite doesn't create local sysdb file. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's take advantage of the files provider and start to get rid of the local provider references in our code. Let's just replace "LOCAL_SYSDB_FILE" for NULL as This test suite doesn't create local sysdb file. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

As all tests are now taking advantage of the "files" provider instead of the "local" one, let's just remove the last reference of LOCAL_SYSDB_FILE from our tests. Together with the reference, let's also remove the whole if-block as we're not relying on "local" provider anymore. Related: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

Let's "get rid" of the local provider and only build it conditionally. The local provider is only used by our integration tests and those will be ran in the CI enabling the local provider. If someone, for some reason, still needs to use it, "--enable-local-provider" has been added as a configure option and the provider can be built using that (as done in our integration tests). Resolves: https://pagure.io/SSSD/sssd/issue/3304 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

"local" -> "password" Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

The pysss.local interface has been de-emphasized in favour of the files domain. As there's no current consumer of this API, let's just remove it. Resolves: https://pagure.io/SSSD/sssd/issue/3493 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

jhrozek · 2018-08-13T15:46:07Z

retest this please

jhrozek · 2018-08-13T16:13:14Z

retest this please

jhrozek · 2018-08-13T16:33:20Z

master:
0e211b8
82d51b7
b8946c4
c075e28
15342eb
5a87af9
a24f0c2
2e8fe6a
728e4be
99b5bb5
a8a9e66
064ca0b
6ebcc59
35a200d
2243b34
7d48373

jhrozek reviewed Jul 10, 2018

View reviewed changes

fidencio force-pushed the wip/local_provider branch from 9d97810 to e70a864 Compare July 18, 2018 15:19

fidencio changed the title ~~Do not build the local provider by default~~ WIP: Do not build the local provider by default Jul 18, 2018

fidencio force-pushed the wip/local_provider branch from e70a864 to 2b0053d Compare August 2, 2018 13:36

fidencio added the Changes requested label Aug 2, 2018

fidencio force-pushed the wip/local_provider branch from 2b0053d to 19a7b09 Compare August 2, 2018 13:58

fidencio force-pushed the wip/local_provider branch from 19a7b09 to 618a2bf Compare August 3, 2018 09:45

fidencio force-pushed the wip/local_provider branch from 618a2bf to cd00107 Compare August 6, 2018 17:27

fidencio removed the Changes requested label Aug 6, 2018

fidencio changed the title ~~WIP: Do not build the local provider by default~~ Do not build the local provider by default Aug 7, 2018

fidencio added the postponed until sssd 2.0 label Aug 7, 2018

jhrozek approved these changes Aug 12, 2018

View reviewed changes

jhrozek added the Accepted label Aug 13, 2018

fidencio added 2 commits August 13, 2018 17:27

fidencio added 14 commits August 13, 2018 17:27

pysss: fix typo in comment

857ffc7

"local" -> "password" Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

pysss: remove pysss.local

e224db7

The pysss.local interface has been de-emphasized in favour of the files domain. As there's no current consumer of this API, let's just remove it. Resolves: https://pagure.io/SSSD/sssd/issue/3493 Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

fidencio force-pushed the wip/local_provider branch from cd00107 to e224db7 Compare August 13, 2018 15:29

jhrozek closed this Aug 13, 2018

jhrozek added the Pushed label Aug 13, 2018

pbrezina mentioned this pull request Apr 28, 2020

Remove the pysss.local interface pbrezina/sssd-test#3481

Closed

sssd-bot mentioned this pull request May 2, 2020

Remove the pysss.local interface #4519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not build the local provider by default #611

Do not build the local provider by default #611

fidencio commented Jul 2, 2018

pbrezina commented Jul 2, 2018

jhrozek commented Jul 10, 2018

jhrozek commented Jul 10, 2018

jhrozek Jul 10, 2018

jhrozek Jul 10, 2018

jhrozek Jul 10, 2018

jhrozek Jul 10, 2018

jhrozek commented Jul 10, 2018

pbrezina commented Jul 10, 2018

fidencio commented Jul 18, 2018

fidencio commented Jul 31, 2018

fidencio commented Aug 2, 2018

jhrozek commented Aug 3, 2018

fidencio commented Aug 3, 2018

fidencio commented Aug 6, 2018

jhrozek commented Aug 12, 2018

jhrozek commented Aug 12, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018

Do not build the local provider by default #611

Do not build the local provider by default #611

Conversation

fidencio commented Jul 2, 2018

pbrezina commented Jul 2, 2018

jhrozek commented Jul 10, 2018

jhrozek commented Jul 10, 2018

jhrozek Jul 10, 2018

Choose a reason for hiding this comment

jhrozek Jul 10, 2018

Choose a reason for hiding this comment

jhrozek Jul 10, 2018

Choose a reason for hiding this comment

jhrozek Jul 10, 2018

Choose a reason for hiding this comment

jhrozek commented Jul 10, 2018

pbrezina commented Jul 10, 2018

fidencio commented Jul 18, 2018

fidencio commented Jul 31, 2018

fidencio commented Aug 2, 2018

jhrozek commented Aug 3, 2018

fidencio commented Aug 3, 2018

fidencio commented Aug 6, 2018

jhrozek commented Aug 12, 2018

jhrozek commented Aug 12, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018

jhrozek commented Aug 13, 2018