Bug 1879980: Fix swallowed error message in GroupDetector

[zerodayz]

ldapquery.IsQueryOutOfBoundsError(err) ||
ldapquery.IsEntryNotFoundError(err) ||
ldapquery.IsNoSuchObjectError(err)

Would return false but error was swallowed

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zerodayz
To complete the pull request process, please assign tnozicka after the PR has been reviewed.
You can assign the PR to them by writing /assign @tnozicka in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[zerodayz]

Following that, with swallowed error we set exists to false, but no err as we returned nil.

		exists, err := s.GroupDetector.Exists(ldapGroupUID)
		if err != nil {
			fmt.Fprintf(s.Err, "Error determining LDAP group existence for group %q: %v.\n", ldapGroupUID, err)
			errors = append(errors, err)
			continue
		}
		if exists {
			continue
		}

This PR fixes that and we exit with error properly.

[zerodayz]

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.

[openshift-ci-robot]

@zerodayz: This pull request references Bugzilla bug 1879980, which is invalid:

• expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1879980: Fix swallowed error message in GroupDetector

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]

/bugzilla refresh

[openshift-ci-robot]

@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

• bug is open, matching expected state (open)
• bug target release (4.7.0) matches configured target release for branch (4.7.0)
• bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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]

/retest

[zerodayz]

/retest

[zerodayz]

/retest

[zerodayz]

/retest

[zerodayz]

/retest

[zerodayz]

/retest

[zerodayz]

/retest

[openshift-ci]

@zerodayz: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	ab6be6f	link	/test unit
ci/prow/e2e-aws-serial	ab6be6f	link	/test e2e-aws-serial

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.

[zerodayz]

@deads2k, @mfojtik Can you guys please review, this has been laying around since January 30th and it prevents the unnecessary delete of the AD groups from OpenShift when customer use wrong casing.

[soltysh]

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.

[zerodayz]

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.

[soltysh]

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

oc/go.mod

Line 55 in 64b59fe

gopkg.in/ldap.v2 v2.5.1

whereas the current version is 3.3 which would also require updating what we're using in oc. Lemme know if you're interested.

[zerodayz]

I will talk to you, I would like to take this.

[soltysh]

/assign

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zerodayz
To complete the pull request process, please assign soltysh after the PR has been reviewed.
You can assign the PR to them by writing /assign @soltysh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-merge-robot]

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

[openshift-ci]

@openshift-merge-robot: This pull request references Bugzilla bug 1879980, which is invalid:

• expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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.

[openshift-merge-robot]

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

[openshift-ci]

@openshift-merge-robot: This pull request references Bugzilla bug 1879980, which is invalid:

• expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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.

[openshift-bot]

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

/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-ci]

@openshift-bot: This pull request references Bugzilla bug 1879980, which is invalid:

• expected the bug to target the "4.11.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity.

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.

[wking]

/bugzilla refresh

[openshift-ci]

@wking: This pull request references Bugzilla bug 1879980, which is invalid:

• expected the bug to target the "4.11.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

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.

[openshift-bot]

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

[openshift-ci]

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

[openshift-ci]

@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:

Bug 1879980: Fix swallowed error message in GroupDetector

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.