-
Notifications
You must be signed in to change notification settings - Fork 386
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
Bug 1879980: Fix swallowed error message in GroupDetector #719
Conversation
ldapquery.IsQueryOutOfBoundsError(err) || ldapquery.IsEntryNotFoundError(err) || ldapquery.IsNoSuchObjectError(err) Would return false but error was swallowed
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zerodayz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Following that, with swallowed error we set exists to false, but no err as we returned nil.
This PR fixes that and we exit with error properly. |
Also to mention, it has a side effect of deleting the group from OpenShift, since it doesn't find the group in AD, throws an error, that error is not passed over. no error, means skip the next block, check if exists if true, if not, then delete from OpenShift. |
@zerodayz: This pull request references Bugzilla bug 1879980, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@zerodayz: This pull request references Bugzilla bug 1879980, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest |
6 similar comments
/retest |
/retest |
/retest |
/retest |
/retest |
/retest |
@zerodayz: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@@ -20,7 +20,7 @@ type GroupBasedDetector struct { | |||
func (l *GroupBasedDetector) Exists(ldapGroupUID string) (bool, error) { | |||
group, err := l.groupGetter.GroupEntryFor(ldapGroupUID) | |||
if ldapquery.IsQueryOutOfBoundsError(err) || ldapquery.IsEntryNotFoundError(err) || ldapquery.IsNoSuchObjectError(err) { | |||
return false, nil | |||
return false, err |
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.
Does your fix from go-ldap/ldap#307 applies, I'm worried this is breaking expected scripts where we wanted to ignore, I'd be more inclined to log with V(4), for example, rather than hard fail.
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.
hi @soltysh this is related to https://bugzilla.redhat.com/show_bug.cgi?id=1879980
The groupSync deleted the groups because it can't find the correct OU
OU=Groups,OU=Unix != OU=groups,OU=unix
I believe this will return also if you query wrong DN. I will take some time to check it again. (its been a while ago)
Now this means you hit the error ldapquery.IsQueryOutOfBoundsError(err), but that err is never returned.
Not sure which case is used for ignoring, but if you by mistake set the wrong DN, which will hit OutOfBounds Error
. GroupSync will delete all your OpenShift groups because this error is swallowed and GroupSync doesn't know you hit the wrong DN, instead it just can't see any groups, so it will sync with nothing.
The other patch for upper/lowercase in that case you are in correct DN but just wrong case.
For example:
usersQuery:
baseDN: "ou=users,dc=redhat,dc=com"
usersQuery:
baseDN: "ou=Users,dc=redhat,dc=com"
With go-ldap/ldap#307, we can then easily update oc client, especially https://github.com/openshift/library-go/blob/master/pkg/security/ldapquery/query.go#L116
From:
if !baseDN.AncestorOf(dn) && !baseDN.Equal(dn) {
To:
if !baseDN.AncestorOfFold(dn) && !baseDN.EqualFold(dn) {
But we would also need to bump the version of go-ldap to include that fix, I am not sure how to do that part.
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.
But we would also need to bump the version of go-ldap to include that fix, I am not sure how to do that part.
Hmm... it looks like we're using quite old ldap library
whereas the current version is 3.3 which would also require updating what we're using in oc. Lemme know if you're interested.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 will talk to you, I would like to take this.
/assign |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zerodayz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/bugzilla refresh The requirements for Bugzilla bugs have changed, recalculating validity. |
@openshift-merge-robot: This pull request references Bugzilla bug 1879980, which is invalid:
Comment Retaining the bugzilla/valid-bug label as it was manually added. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh The requirements for Bugzilla bugs have changed, recalculating validity. |
@openshift-merge-robot: This pull request references Bugzilla bug 1879980, which is invalid:
Comment Retaining the bugzilla/valid-bug label as it was manually added. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
/bugzilla refresh The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity. |
@openshift-bot: This pull request references Bugzilla bug 1879980, which is invalid:
Comment Retaining the bugzilla/valid-bug label as it was manually added. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@wking: This pull request references Bugzilla bug 1879980, which is invalid:
Comment Retaining the bugzilla/valid-bug label as it was manually added. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@zerodayz: This pull request references Bugzilla bug 1879980. The bug has been updated to no longer refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ldapquery.IsQueryOutOfBoundsError(err) ||
ldapquery.IsEntryNotFoundError(err) ||
ldapquery.IsNoSuchObjectError(err)
Would return false but error was swallowed