-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
fixed group detection bug for LDAP prune #6323
fixed group detection bug for LDAP prune #6323
Conversation
@@ -26,6 +26,10 @@ func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) { | |||
return false, err | |||
} | |||
|
|||
if group == nil { | |||
return false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liggitt amazingly it is possible that when doing a specific search for a specific DN in LDAP we can get nothing back and not NoSuchObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the LDAP query look like for the DN lookup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the LDAP query look like for the DN lookup?
@stevekuznetsov answer this to make sure that it looks sane before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't set the scope to base object only, that makes sense. We should be, however. I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there may be larger problems. In either case, QueryForUniqueEntry
, when it retrieves nothing, it should throw an ErrorEntryNotFound
. Which the detector should recognize as meaning the thing it was looking for doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, never mind all of that. I can't read. The search request is:
&{cn=group1,ou=groups,ou=adextended,dc=example,dc=com 0 0 0 0 false (objectClass=*) [cn dn] []}
which translates to
&SearchRequest{
BaseDN: "cn=group1,ou=groups,ou=adextended,dc=example,dc=com",
Scope: ScopeBaseObject,
DerefAliases: NeverDerefAliases,
SizeLimit: None,
TimeLimit: None,
TypesOnly: false,
Filter: "(objectClass=*)",
Attributes: ["cn", "dn"],
Controls: none,
}
I think everything is working as expected now.
cc05f7f
to
3446dce
Compare
@deads2k comments addressed [test][extended:ldap_groups] |
lgtm pending tests. @stevekuznetsov update bugzilla with a reference to this pull. |
Evaluated for origin test up to 3446dce |
@deads2k Bugzilla has the update already |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7842/) (Extended Tests: ldap_groups) |
for _, locator := range l.locators { | ||
opinion, err := locator.Exists(ldapGrouUID) | ||
if err != nil { | ||
return false, err | ||
} | ||
conclusion = conclusion || opinion | ||
conclusion = conclusion && opinion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we return false early if one returns false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we return false early if one returns false?
Doc on the type guarantees an error in one errors. I'm ok with forcing all successful inquiries to be sure about the decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly @deads2k and I decided we want all of the locators to run to ensure that no errors are encountered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly @deads2k and I decided we want all of the locators to run to ensure that no errors are encountered.
You wanted, I had no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@deads2k all green |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4357/) (Image: devenv-rhel7_2962) |
Evaluated for origin merge up to 3446dce |
Merged by openshift-bot
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1291607
@deads2k PTAL