feat(ingestion) ldap: make ldap attrs keys configurable #4682

atulsaurav · 2022-04-17T15:32:52Z

Code changes to consume attrs_mapping from ldap recipe
Doc changes outlining how to use different configurations in recipe

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

…ject#4599) * Code changes to consume attrs_mapping from ldap recipe * Doc changes outlining how to use different configurations in recipe

github-actions · 2022-04-17T16:01:00Z

Unit Test Results (build & test)

385 tests ±0 385 ✔️ +6 10m 7s ⏱️ -33s
  91 suites ±0     0 💤 ±0
  91 files ±0     0 ❌ - 6

Results for commit ec943a4. ± Comparison against base commit 0949613.

♻️ This comment has been updated with latest results.

github-actions · 2022-04-17T16:10:44Z

Unit Test Results (metadata ingestion)

      8 files ±0       8 suites ±0 1h 17m 40s ⏱️ - 2m 24s
  565 tests ±0   562 ✔️ ±0   3 💤 ±0 0 ❌ ±0
1 068 runs ±0 1 025 ✔️ ±0 43 💤 ±0 0 ❌ ±0

Results for commit ec943a4. ± Comparison against base commit 0949613.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-04-19T18:05:47Z

As discussed at office hours.. Let's move forward by duplicating the LDAP source and taking the caching approach you suggested:

Fetch all users, extract the attribute that should be used in urn (from config)
Create map from DN -> attribute for urn
Fetch all groups, link to members, build GroupMembership aspect for each user
Write back to DataHub

John

jjoyce0510 · 2022-05-03T16:14:02Z

metadata-ingestion/source_docs/ldap.md

@@ -34,6 +34,24 @@ source:
    # Options
    base_dn: "dc=example,dc=org"

+    # Optional attribute mapping to allow ldap config differences across orgs
+    attrs_mapping:


@atulsaurav im wondering if this should really map "DataHub" concepts to specific attributes?

Like this:

urn_id:
email
firstName
lastName
displayName

Instead of mapping LDAP -> LDAP concepts

Yes, good idea! Let me make changes and send you an update later in the evening. Thanks!

jjoyce0510 · 2022-05-03T16:14:21Z

metadata-ingestion/source_docs/ldap.md


 The `drop_missing_first_last_name` should be set to true if you've got many "headless" user LDAP accounts
 for devices or services should be excluded when they do not contain a first and last name. This will only
 impact the ingestion of LDAP users, while LDAP groups will be unaffected by this config option.

+### Configurable LDAP
+
+Every organization may implement LDAP slightly differently based on their needs. The makes a standard LDAP recipe ineffecvtive due to missing data during LDAP ingestion. For instance, LDAP recipe assumes department information for a CorpUser would be present in the `departmentNumber` attribute. If an organization chose not to implement that attribute or rather capture similar imformation in the `department` attribute, that information can be missed during LDAP ingestion (even though the information may be present in LDAP in a slightly different form). LDAP source provides flexibility to provide optional mapping for such variations to be reperesented under attrs_mapping. So if an organization represented `departmentNumber` as `department` and `mail` as `email`, the recipe can be adapted to customiza that mapping based on need. An example is show below. If `attrs_mapping` section is not provided, the default mapping will apply.


minor typo:

ineffecvtive -> ineffective

typo:

customiza -> customize

Thanks for catching these!

jjoyce0510 · 2022-05-03T16:31:20Z

metadata-ingestion/src/datahub/ingestion/source/ldap.py

-            members = parse_from_attrs(attrs, "uniqueMember")
-            email = attrs["mail"][0].decode() if "mail" in attrs else full_name
+            owners = parse_from_attrs(attrs, self.config.attrs_mapping["owner"])
+            members = parse_from_attrs(attrs, self.config.attrs_mapping["uniqueMember"])


Remember -> this members field is deprecated. instead, we should be populating the "GroupMembership" aspect of the user object.. Were you intending to do this as a followup?

yes, as a part of #3335

Wonderful! Thanks for the update

jjoyce0510 · 2022-05-04T22:34:27Z

Looking great so far. Once we do the "datahub concept" mapping piece we are good to go!

jjoyce0510 · 2022-06-10T14:57:29Z

metadata-ingestion/src/datahub/ingestion/source/ldap.py

+# default mapping for attrs
+# general attrs
+attrs_mapping: Dict[str, Any] = {}
+attrs_mapping["urn"] = "sAMAccountName"


So this is the attribute used to construct the urn?

yes, thanks for clarifying that in the docs!

metadata-ingestion/src/datahub/ingestion/source/ldap.py

jjoyce0510

Overall looks good. As a followup we definitely should fix the LDAP group extraction to use the GroupMembership MCEs!

…hub-project-master

…atahub into configurable-ldap-ingestion

jjoyce0510 · 2022-06-21T14:45:11Z

metadata-ingestion/archived/source_docs/ldap.md

+      group_urn: cn
+      admins: owner
+      members: uniqueMember
+      displayName: name


Won't this overwrite the display name attribute defined above?

This makes me think we do need 2 distinct mappings:

userMappings
groupMappings

I should have called this out earlier - my apologies for that. What do you think?

No worries, done!

jjoyce0510 · 2022-06-21T14:45:40Z

metadata-ingestion/src/datahub/ingestion/source/ldap.py

+attrs_mapping["group_urn"] = "cn"
+attrs_mapping["admins"] = "owner"
+attrs_mapping["members"] = "uniqueMember"
+attrs_mapping["displayName"] = "name"


This overwrites like 36

…atahub into configurable-ldap-ingestion

jjoyce0510

Thank you so much for improving this connector @atulsaurav !

…ect#4682)

bda618

int(attrs[self.config.attrs_mapping["departmentId"]][0].decode())

The line above is actually failing for me with the following error:

line 320, in build_corp_user_mce
int(attrs[self.config.user_attrs_map["departmentId"]][0].decode())

ValueError: invalid literal for int() with base 10: 'USE_NEW_GL_FIELDS'

It appears that "departmentId" has been mapped to "departmentNumber" which we don't have at our corp ldap. Thus, it is null. I think you can reproduce this error by rolling back changes at the test case to null.

I have temporali applied a fix as below:

attrs[self.config.attrs_mapping["departmentId"]][0]).decode()

It fixed a problem partly, but some users were not ingested and end up with error like " Failed to extract some records due to: source produced an invalid metadata work unit"

atulsaurav · 2022-06-22T22:14:27Z

Yes this is an oddity with the way the LDAP connector was written. DH has 2 fields for department - departmentId and departmentName. DH makes a strong assumption that departmentId field should be numeric. I have opened up a discussion on slack in metadata-modeling channel on this topic. If we relax the metadata model to allow string types in departmentId field, this would be the way to go. For now instead of code change, you could define the mapping for departmentId field in your recipe to read from LDAP attr (e.g.) departmentId. Since that will not be found in your ldap, the departmentId field in DH will be left as null as it used to. Please let me know if this helps/makes sense.

bda618 · 2022-06-23T15:04:40Z

I have just done what you suggested and that fixed a problem, here is remapping from my receipt:

source:
    type: "ldap"
    config:
       # Map from a default mapping as departmentNumber to departmentId
       user_attrs_map: {"departmentId":"departmentId"}

The original source of the problem with our departmentNumber is that it is not a number, but a string.

…ect#4682)

feat(ingestion) ldap: make ldap atttrs keys configurable (datahub-pro…

cab247d

…ject#4599) * Code changes to consume attrs_mapping from ldap recipe * Doc changes outlining how to use different configurations in recipe

atulsaurav marked this pull request as draft April 17, 2022 15:33

atulsaurav changed the title ~~feat(ingestion) ldap: make ldap atttrs keys configurable (#4599)~~ feat(ingestion) ldap: make ldap atttrs keys configurable Apr 17, 2022

atulsaurav added 5 commits April 17, 2022 12:47

test(ingestion) ldap: Add test case for configurable ldap attrs

5e50969

Add missing coma

6f5fbab

fix: ldap attrs_mapping defaults & test case

d0099e5

test(ingestion): fix attrs_mapping in test case

69dfbb3

fix membership test

3b5c051

atulsaurav marked this pull request as ready for review April 17, 2022 22:03

swaroopjagadish requested a review from jjoyce0510 April 18, 2022 19:29

maggiehays assigned jjoyce0510 Apr 19, 2022

jjoyce0510 reviewed May 3, 2022

View reviewed changes

Merge branch 'master' into configurable-ldap-ingestion

0e0946a

atulsaurav marked this pull request as draft June 6, 2022 21:40

atulsaurav and others added 6 commits June 7, 2022 06:22

merge upstream changes in prior changes

7680041

Merge branch 'master' into configurable-ldap-ingestion

bfcc13b

Merge branch 'master' into configurable-ldap-ingestion

ff8b3c3

Merge branch 'master' into configurable-ldap-ingestion

015a517

Doc changes for attrs_mapping between LDAP and DH concepts

b736547

Merge branch 'master' into configurable-ldap-ingestion

5132e75

atulsaurav marked this pull request as ready for review June 9, 2022 03:27

Update ldap.md

7560c03

jjoyce0510 reviewed Jun 10, 2022

View reviewed changes

metadata-ingestion/src/datahub/ingestion/source/ldap.py Outdated Show resolved Hide resolved

jjoyce0510 reviewed Jun 10, 2022

View reviewed changes

atulsaurav added 11 commits June 20, 2022 17:36

Changes based on review comments

f485349

Merge branch 'master' of github.com:datahub-project/datahub into data…

3955951

…hub-project-master

Merge branch 'datahub-project-master' into configurable-ldap-ingestion

e077971

Merge branch 'configurable-ldap-ingestion' of github.com:atulsaurav/d…

474ac7c

…atahub into configurable-ldap-ingestion

fix f-string linting error

4df5568

Fix guess_person_ldap changes from upstream

74175e0

Fix group membership test

68add91

Update docs

44eebfe

fix default for group description field

e9aa404

Remove breaking change related to Department info

72fad69

fix handling of departmentId

0fbb294

atulsaurav requested a review from jjoyce0510 June 21, 2022 03:25

atulsaurav changed the title ~~feat(ingestion) ldap: make ldap atttrs keys configurable~~ feat(ingestion) ldap: make ldap attrs keys configurable Jun 21, 2022

Merge branch 'datahub-project:master' into configurable-ldap-ingestion

49f14ee

jjoyce0510 reviewed Jun 21, 2022

View reviewed changes

atulsaurav and others added 5 commits June 21, 2022 12:40

Merge branch 'master' into configurable-ldap-ingestion

26aa509

Split attrs_mapping into user_attrs_map & group_attrs_map

d9de05c

add missing group email attribute

9ffb3af

Merge branch 'configurable-ldap-ingestion' of github.com:atulsaurav/d…

ec943a4

…atahub into configurable-ldap-ingestion

Update ldap.md

d3c69b5

jjoyce0510 approved these changes Jun 21, 2022

View reviewed changes

jjoyce0510 merged commit 648ebfc into datahub-project:master Jun 21, 2022

shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Jun 22, 2022

feat(ingestion) ldap: make ldap attrs keys configurable (datahub-proj…

e486966

…ect#4682)

bda618 reviewed Jun 22, 2022

View reviewed changes

alexey-kravtsov pushed a commit to infobip/datahub that referenced this pull request Jul 8, 2022

feat(ingestion) ldap: make ldap attrs keys configurable (datahub-proj…

fac6d6e

…ect#4682)

maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022

feat(ingestion) ldap: make ldap attrs keys configurable (datahub-proj…

91c4afb

…ect#4682)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingestion) ldap: make ldap attrs keys configurable #4682

feat(ingestion) ldap: make ldap attrs keys configurable #4682

atulsaurav commented Apr 17, 2022 •

edited

github-actions bot commented Apr 17, 2022 •

edited

github-actions bot commented Apr 17, 2022 •

edited

jjoyce0510 commented Apr 19, 2022

jjoyce0510 May 3, 2022

atulsaurav May 3, 2022

jjoyce0510 May 3, 2022

jjoyce0510 May 3, 2022

atulsaurav May 3, 2022

jjoyce0510 May 3, 2022

atulsaurav May 3, 2022

jjoyce0510 May 4, 2022

jjoyce0510 commented May 4, 2022

jjoyce0510 Jun 10, 2022

atulsaurav Jun 10, 2022

jjoyce0510 left a comment

jjoyce0510 Jun 21, 2022

atulsaurav Jun 21, 2022

jjoyce0510 Jun 21, 2022

jjoyce0510 left a comment

bda618 left a comment •

edited

atulsaurav commented Jun 22, 2022 •

edited

bda618 commented Jun 23, 2022 •

edited

feat(ingestion) ldap: make ldap attrs keys configurable #4682

feat(ingestion) ldap: make ldap attrs keys configurable #4682

Conversation

atulsaurav commented Apr 17, 2022 • edited

Checklist

github-actions bot commented Apr 17, 2022 • edited

Unit Test Results (build & test)

github-actions bot commented Apr 17, 2022 • edited

Unit Test Results (metadata ingestion)

jjoyce0510 commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

bda618 left a comment • edited

Choose a reason for hiding this comment

atulsaurav commented Jun 22, 2022 • edited

bda618 commented Jun 23, 2022 • edited

atulsaurav commented Apr 17, 2022 •

edited

github-actions bot commented Apr 17, 2022 •

edited

github-actions bot commented Apr 17, 2022 •

edited

bda618 left a comment •

edited

atulsaurav commented Jun 22, 2022 •

edited

bda618 commented Jun 23, 2022 •

edited