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

fix(ingest): If there is no manager for a LDAP user (example: system account) #5180

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

bda618
Copy link
Contributor

@bda618 bda618 commented Jun 15, 2022

The first element of the results3 tuple could be an empty array [], for example, if it is a system account and it has no managers. Thus, the the current code is failing with "out of range" error. I added a quick check if array is not empty which has fixed this error.

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

@bda618 bda618 changed the title Fix for cases If there is no manager for a LDAP user (example: system account) fix(ingest): fix for cases If there is no manager for a LDAP user (example: system account) Jun 15, 2022
@bda618 bda618 changed the title fix(ingest): fix for cases If there is no manager for a LDAP user (example: system account) fix(ingest): If there is no manager for a LDAP user (example: system account) Jun 15, 2022
_m_dn, m_attrs = self.ldap_client.result3(manager_msgid)[1][0]
manager_ldap = guess_person_ldap(m_attrs)
result = self.ldap_client.result3(manager_msgid)
if result[1]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this also result in an out of range error if the list is empty? since we want to access index 1, we could check if result.length > 1:

Copy link
Contributor Author

@bda618 bda618 Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result is a Tuple and here is an example where the existing code is going to fail (directly from my debug):

(101, [], 3, [<ldap.controls.libldap.SimplePagedResultsControl object …>])

But checking the number elements in the tuple as: len(result) > 1 will not work because there are 4 elements at this tuple. I think an actual issue is a second [0], when we try to access the first element (index 0) from the second element of the tuple (index 1). However, it is empty ([]) and this is where the error is happaning. By checking result[1] we will skip such scenario.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh gotcha, yeah that was my misunderstanding on what the actual output of self.ldap_client.result3(manager_msgid) was. Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chriscollins3456 Sure, glad if this was helpful.

@github-actions
Copy link

Unit Test Results (build & test)

381 tests  ±0   381 ✔️ ±0   3m 36s ⏱️ -7s
  89 suites ±0       0 💤 ±0 
  89 files   ±0       0 ±0 

Results for commit c97ea3f. ± Comparison against base commit b4bf1d4.

@github-actions
Copy link

Unit Test Results (metadata ingestion)

       5 files         5 suites   1h 29m 11s ⏱️
   555 tests    552 ✔️   3 💤 0
2 552 runs  2 477 ✔️ 75 💤 0

Results for commit c97ea3f.

@anshbansal anshbansal merged commit 9d7d7de into datahub-project:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants