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

Use groupUIDNameMapping for LDAP sync/prune with Openshift groups #16071

Merged
merged 1 commit into from
Sep 13, 2017
Merged

Use groupUIDNameMapping for LDAP sync/prune with Openshift groups #16071

merged 1 commit into from
Sep 13, 2017

Conversation

enj
Copy link
Contributor

@enj enj commented Aug 31, 2017

When syncing LDAP groups with --type=openshift or when pruning groups, the LDAPGroupUIDToOpenShiftGroupNameMapping should be taken into consideration since:

  1. The system of truth in both flows is openshift groups
  2. The mapping was probably used to name said openshift groups

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1484831

Signed-off-by: Monis Khan mkhan@redhat.com

@openshift/sig-security @stevekuznetsov

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 31, 2017
@enj
Copy link
Contributor Author

enj commented Aug 31, 2017

/test extended_ldap_groups

@liggitt
Copy link
Contributor

liggitt commented Aug 31, 2017

Seems like there should be a test for this scenario

@enj
Copy link
Contributor Author

enj commented Aug 31, 2017

Seems like there should be a test for this scenario

Definitely. But I have not even tested locally yet 😄

This was referenced Aug 31, 2017
Closed
Closed
@enj
Copy link
Contributor Author

enj commented Aug 31, 2017

/retest

Flake on kubernetes/kubernetes#51704 and #16025

stevekuznetsov
stevekuznetsov reviewed Aug 31, 2017
View reviewed changes
pkg/oc/admin/groups/sync/cli/sync.go
@@ -230,13 +231,28 @@ func (o *SyncOptions) Complete(typeArg, whitelistFile, blacklistFile, configFile
}

// buildOpenShiftGroupNameList builds a list of OpenShift names from file and args
func buildOpenShiftGroupNameList(args []string, file string) ([]string, error) {
// nameMapping is used to override the OpenShift names built from file and args
Copy link
Contributor

Choose a reason for hiding this comment

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

bad godoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Er what should I say instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't match func name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the second line of the go doc. The first line correctly states buildOpenShiftGroupNameList.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh wow am bad at reading diffs

@simo5
Copy link
Contributor

simo5 commented Sep 5, 2017

/retest

@simo5
Copy link
Contributor

simo5 commented Sep 11, 2017

@enj what's the status of this PR ?

@enj
Copy link
Contributor Author

enj commented Sep 12, 2017

/test extended_ldap_groups

@enj
Use mapping for LDAP sync/prune w/ Openshift group
c3c59c2 
When syncing LDAP groups with --type=openshift or when pruning
groups, the LDAPGroupUIDToOpenShiftGroupNameMapping should be taken
into consideration since:

1. The system of truth in both flows is openshift groups
2. The mapping was probably used to name said openshift groups

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1484831

Signed-off-by: Monis Khan <mkhan@redhat.com>
@enj enj force-pushed the enj/i/ldap_prune_mapping/1484831 branch from 110b644 to c3c59c2 Compare September 12, 2017 20:02
@enj
Copy link
Contributor Author

enj commented Sep 12, 2017

/test extended_ldap_groups

@enj
Copy link
Contributor Author

enj commented Sep 12, 2017

Running both of my added tests on master results in the expect error groups "cn=group1,ou=groups,ou=rfc2307,dc=example,dc=com" not found.

@enj
Copy link
Contributor Author

enj commented Sep 12, 2017

@stevekuznetsov now the CI is being hateful: The ImageStreamTag "openldap:latest" is invalid: from: Error resolving ImageStreamTag openldap:latest in namespace openldap: unable to find latest tagged image.

/retest

@stevekuznetsov
Copy link
Contributor

/shrug

@openshift-ci-robot openshift-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Sep 12, 2017
@enj
Copy link
Contributor Author

enj commented Sep 12, 2017

@stevekuznetsov @liggitt @simo5 PTAL

@simo5
Copy link
Contributor

simo5 commented Sep 13, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2017
@stevekuznetsov
Copy link
Contributor

/approve

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, simo5, stevekuznetsov

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2017
@enj
Copy link
Contributor Author

enj commented Sep 13, 2017

/retest

@enj
Copy link
Contributor Author

enj commented Sep 13, 2017

Failed on #16323

@enj
Copy link
Contributor Author

enj commented Sep 13, 2017

/retest

@stevekuznetsov
Copy link
Contributor

/shrug cancel

@stevekuznetsov
Copy link
Contributor

/unshrug

@openshift-ci-robot
Copy link

@stevekuznetsov: ¯\_(ツ)_/¯

In response to this:

/unshrug

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-robot openshift-ci-robot removed the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Sep 13, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16150, 16284, 16296, 16071)

@openshift-merge-robot openshift-merge-robot merged commit 75e79bc into openshift:master Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

Assignees

@mfojtik mfojtik

@fabianofranz fabianofranz

@simo5 simo5

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@enj @liggitt @simo5 @stevekuznetsov @openshift-merge-robot @openshift-ci-robot @mfojtik @fabianofranz