-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,14 +18,18 @@ type GroupBasedDetector struct { | |
} | ||
|
||
func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) { | ||
_, err := l.groupGetter.GroupEntryFor(ldapGroupUID) | ||
group, err := l.groupGetter.GroupEntryFor(ldapGroupUID) | ||
if ldaputil.IsQueryOutOfBoundsError(err) || ldaputil.IsEntryNotFoundError(err) || ldaputil.IsNoSuchObjectError(err) { | ||
return false, nil | ||
} | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
if group == nil { | ||
return false, nil | ||
} | ||
|
||
return true, nil | ||
} | ||
|
||
|
@@ -76,13 +80,17 @@ type CompoundDetector struct { | |
} | ||
|
||
func (l *CompoundDetector) Exists(ldapGrouUID string) (bool, error) { | ||
conclusion := false | ||
if len(l.locators) == 0 { | ||
return false, nil | ||
} | ||
|
||
conclusion := true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. If you don't have any locators, the result should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test that makes sure that no locators results in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should allow a |
||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
You wanted, I had no strong opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
} | ||
return conclusion, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
apiVersion: v1 | ||
kind: Group | ||
metadata: | ||
annotations: | ||
openshift.io/ldap.uid: cn=group2,ou=groups,ou=adextended,dc=example,dc=com | ||
openshift.io/ldap.url: LDAP_SERVICE_IP:389 | ||
creationTimestamp: null | ||
labels: | ||
openshift.io/ldap.host: LDAP_SERVICE_IP | ||
name: extended-group2 | ||
users: | ||
- person1smith@example.com | ||
- person2smith@example.com | ||
- person3smith@example.com | ||
apiVersion: v1 | ||
kind: Group | ||
metadata: | ||
annotations: | ||
openshift.io/ldap.uid: cn=group3,ou=groups,ou=adextended,dc=example,dc=com | ||
openshift.io/ldap.url: LDAP_SERVICE_IP:389 | ||
creationTimestamp: null | ||
labels: | ||
openshift.io/ldap.host: LDAP_SERVICE_IP | ||
name: extended-group3 | ||
users: | ||
- person1smith@example.com | ||
- person5smith@example.com |
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.
@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 anErrorEntryNotFound
. 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:
which translates to
I think everything is working as expected now.