Merge pull request #6323 from stevekuznetsov/skuznets/prune-groups-bug

Merged by openshift-bot
openshift · Dec 16, 2015 · f7e2d9d · f7e2d9d
2 parents db93f2f + 3446dce
commit f7e2d9d
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 9 deletions.
diff --git a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector.go
@@ -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
 	for _, locator := range l.locators {
 		opinion, err := locator.Exists(ldapGrouUID)
 		if err != nil {
 			return false, err
 		}
-		conclusion = conclusion || opinion
+		conclusion = conclusion && opinion
 	}
 	return conclusion, nil
 }
diff --git a/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go b/pkg/cmd/experimental/syncgroups/groupdetector/groupdetector_test.go
@@ -43,9 +43,15 @@ func TestGroupBasedDetectorExists(t *testing.T) {
 			expectedExists: false,
 		},
 		{
-			name:           "no error",
+			name:           "no error, no entries",
 			groupGetter:    &puppetGetterExtractor{},
 			expectedErr:    nil,
+			expectedExists: false,
+		},
+		{
+			name:           "no error, has entries",
+			groupGetter:    &puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}},
+			expectedErr:    nil,
 			expectedExists: true,
 		},
 	}
@@ -126,10 +132,16 @@ func TestCompoundDetectorExists(t *testing.T) {
 		expectedErr    error
 		expectedExists bool
 	}{
+		{
+			name:           "no detectors",
+			locators:       []interfaces.LDAPGroupDetector{},
+			expectedErr:    nil,
+			expectedExists: false,
+		},
 		{
 			name: "none fail to locate",
 			locators: []interfaces.LDAPGroupDetector{
-				NewGroupBasedDetector(&puppetGetterExtractor{}),
+				NewGroupBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}),
 				NewMemberBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}),
 			},
 			expectedErr:    nil,
@@ -142,17 +154,26 @@ func TestCompoundDetectorExists(t *testing.T) {
 				NewMemberBasedDetector(&puppetGetterExtractor{returnVal: []*ldap.Entry{dummyEntry}}),
 			},
 			expectedErr:    nil,
-			expectedExists: true,
+			expectedExists: false,
 		},
 		{
-			name: "all fail to locate",
+			name: "all fail to locate because of errors",
 			locators: []interfaces.LDAPGroupDetector{
 				NewGroupBasedDetector(&errEntryNotFoundGetterExtractor{}),
 				NewMemberBasedDetector(&errOutOfBoundsGetterExtractor{}),
 			},
 			expectedErr:    nil,
 			expectedExists: false,
 		},
+		{
+			name: "all locate no entries",
+			locators: []interfaces.LDAPGroupDetector{
+				NewGroupBasedDetector(&puppetGetterExtractor{}),
+				NewMemberBasedDetector(&puppetGetterExtractor{}),
+			},
+			expectedErr:    nil,
+			expectedExists: false,
+		},
 		{
 			name: "one errors",
 			locators: []interfaces.LDAPGroupDetector{

diff --git a/test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt b/test/extended/authentication/ldap/augmented-ad/valid_all_ldap_sync_delete_prune.txt
@@ -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
diff --git a/test/extended/ldap_groups.sh b/test/extended/ldap_groups.sh
@@ -216,12 +216,11 @@ for (( i=0; i<${#schema[@]}; i++ )); do
 	openshift ex sync-groups --type=openshift --sync-config=sync-config.yaml --confirm
 	compare_and_cleanup valid_all_ldap_sync_user_defined.yaml
 
-
-
 	echo -e "\tTEST: Sync all LDAP groups from LDAP server using DN as attribute whenever possible"
     openshift ex sync-groups --sync-config=sync-config-dn-everywhere.yaml --confirm
 	compare_and_cleanup valid_all_ldap_sync_dn_everywhere.yaml
 
+
 	# PRUNING
 	echo -e "\tTEST: Sync all LDAP groups from LDAP server, change LDAP UID, then prune OpenShift groups"
 	openshift ex sync-groups --sync-config=sync-config.yaml --confirm
@@ -231,3 +230,12 @@ for (( i=0; i<${#schema[@]}; i++ )); do
 
     popd > /dev/null
 done
+
+# special test for ad-extended
+pushd ${BASETMPDIR}/augmented-ad > /dev/null
+echo -e "\tTEST: Sync all LDAP groups from LDAP server, remove LDAP group metadata entry, then prune OpenShift groups"
+openshift ex sync-groups --sync-config=sync-config.yaml --confirm
+ldapdelete -x -h $LDAP_SERVICE_IP -p 389 -D cn=Manager,dc=example,dc=com -w admin "${group1_ldapuid}"
+openshift ex prune-groups --sync-config=sync-config.yaml --confirm
+compare_and_cleanup valid_all_ldap_sync_delete_prune.txt		
+popd > /dev/null