Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added LDAP Group Sync for Single LDAP Schema #3996

Closed
wants to merge 8 commits into from
Closed

Added LDAP Group Sync for Single LDAP Schema #3996

wants to merge 8 commits into from

Conversation

stevekuznetsov
Copy link
Contributor

@deads2k @liggitt

Implementation in place, testing in place but blocked from running by openshift/openldap#6. Implementation for following schema only:

  1. Groups as first-class LDAP entries with an attribute containing a list of members:
    1. members listed with their DN in the attribute
    2. members listed with some other UID in the attribute

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-group-sync branch 5 times, most recently from a2054ab to 72618af Compare August 5, 2015 13:54
stevekuznetsov
stevekuznetsov reviewed Aug 5, 2015
View reviewed changes
pkg/cmd/experimental/sync-groups/groupsyncer.go
groupGetter: extractor,
}
}
extractor.groupNameMapper = nameMapper
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we wanted to break out a GroupNameMapper as a discrete object and allow us to substitute whatever for it, we run into this cyclical dependency where you can't create the DataExtractor without populating the GroupNameMapper but the GroupNameMapper needs to have a pointer to a groupGetter, which is the DataExtractor itself.

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-group-sync branch 8 times, most recently from 0276eee to 6ba0050 Compare August 7, 2015 15:56
deads2k
deads2k reviewed Aug 7, 2015
View reviewed changes
pkg/auth/authenticator/password/ldappassword/ldap.go
c.ServerName = a.options.URL.Host
}
tlsConfig = &c
serverConfig := &ldaputil.LDAPServerConfig{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't the Options contain an LDAPServerConfig directly?

deads2k
deads2k reviewed Aug 7, 2015
View reviewed changes
pkg/cmd/experimental/syncgroups/config/types.go
// LDAPSyncTypeFirstClassUserss defines an LDAP schema where users are stored as first-class LDAP
// entries with an attribute containing a list of groups they are a member of, with groups listed
// by a unique name, with no additional group metadata in the LDAP server.
LDAPSchemaTypeFirstClassUserss LDAPSchemaType = "FirstClassUserss"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double s typo

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-group-sync branch 2 times, most recently from 0390f11 to 62a194e Compare August 7, 2015 16:53
deads2k
deads2k reviewed Aug 7, 2015
View reviewed changes
pkg/cmd/experimental/syncgroups/config/types.go
WhitelistContents []string // added to loaded config from cli input

// Type determines which LDAP server schema exists for the sync
Type LDAPSchemaType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I like this. I'd rather split into different types based on the LDAPSchemaType rather than have one object that contains it all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I borrowed from our buildapi - it was a very common thing there. Pros/cons? Clearly having <nil> parts of the config object isn't the best, but otherwise we will have three objects that are exactly the same save this one part.

deads2k
deads2k reviewed Sep 25, 2015
View reviewed changes
pkg/auth/ldaputil/query.go
allAttributes := util.NewStringSet(o.NameAttributes...)
allAttributes.Insert(additionalAttributes...)

if o.QueryAttribute == "DN" || o.QueryAttribute == "dn" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not case insensitive?

@stevekuznetsov stevekuznetsov force-pushed the skuznets/ldap-group-sync branch 4 times, most recently from c8603a1 to e9e6bf3 Compare September 25, 2015 21:47
@stevekuznetsov
Copy link
Contributor Author

@deads2k Addressed comments. Added unit tests. Extended test is failing to connect to the LDAP server, and I cannot get the build for the server to succeed when running locally, but when I run the build as part of the extended test locally, I have no issues.

@stevekuznetsov stevekuznetsov changed the title [WIP] Added LDAP Group Sync for Single LDAP Schema Added LDAP Group Sync for Single LDAP Schema Sep 26, 2015
@stevekuznetsov
Copy link
Contributor Author

[test][extended]

@stevekuznetsov
Copy link
Contributor Author

Flake on Travis in test-cmd:

!!! Error in test/cmd/admin.sh:76
    'oadm policy add-cluster-role-to-user cluster-admin system:no-user' exited with status 1
Call stack:
    1: test/cmd/admin.sh:76 main(...)
Exiting with status 1
!!! Error in hack/test-cmd.sh:269
    '${test}' exited with status 1
Call stack:
    1: hack/test-cmd.sh:269 main(...)
Exiting with status 1
[FAIL] !!!!! Test Failed !!!!

cc: @liggitt

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 88e074e

@deads2k
Copy link
Contributor

deads2k commented Sep 28, 2015

@stevekuznetsov squash this to something more reasonable. I'd guess

  1. Sync implementation
  2. Command
  3. Config objects

Fix the problem with "I will not merge this. That contract is insane. Lists don't fail on empty result sets." and I think I'll merge it warts and all unless @liggitt yells.

@deads2k deads2k mentioned this pull request Sep 28, 2015
Merged
@stevekuznetsov stevekuznetsov deleted the skuznets/ldap-group-sync branch October 8, 2015 12:17
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 47cbb68 on stevekuznetsov:skuznets/ldap-group-sync into ** on openshift:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@stevekuznetsov @ncdc @smarterclayton @liggitt @deads2k @danmcp @openshift-bot @coveralls