-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Split identity and user objects #1450
Conversation
[test] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1515/) |
need to add the Identity types to the proper internal groups and/or default policy. See pkg/authorization/api/types.go |
@@ -7,10 +7,10 @@ import ( | |||
// UserIdentityInfo contains information about an identity. Identities are distinct from users. An authentication server of | |||
// some kind (like oauth for example) describes an identity. Our system controls the users mapped to this identity. | |||
type UserIdentityInfo interface { |
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.
This isn't used how you'd expect. GetProviderName()
is never called. That suggests that this is the wrong abstraction
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.
Agree. Moved configurable providerName from the mapper into the various identity providers. I'm not sure why I didn't want to trust the identity providers to set the providerName on the identity info they give to the mapper
Comments addressed |
all the password authenticators are determining identity and then making a call to lookup the user from the identity. Seems like it would make sense to have an interface that returns an identity and then a single filter/wrapper for taking the identity and getting a user. |
@@ -7,5 +7,5 @@ import ( | |||
) | |||
|
|||
func TestGoogle(t *testing.T) { | |||
_ = external.Provider(NewProvider("", "")) | |||
_ = external.Provider(NewProvider("", "", "")) |
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.
Don't mock up an empty providerName
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.
fixed
Need a comment in at least one spot that talks about how users always reference an identity before an identity references back and how an identity never exists before a corresponding user is created. |
hmm... That last isn't always true, that's just the order of initialization. |
return nil, existingMappingErr | ||
} | ||
|
||
// Ensure identity is valid |
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.
At this point, can't we delegate to Update?
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.
refactored so create and update both delegate to a common method
|
||
// Validate validates a new user | ||
func (userStrategy) Validate(obj runtime.Object) kerrs.ValidationErrorList { | ||
user := obj.(*api.User) |
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.
could have been a bad cast.
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.
copied from other Validate methods... a cast without a double-assignment to a boolean panics if it fails.
Comments addressed or converted into TODOs in the checklist |
|
||
// Spawn 5 simultaneous mappers to test race conditions | ||
var wg sync.WaitGroup | ||
for i := 0; i < 5; i++ { |
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'll sing a little song and do a little dance.
----- Original Message -----
if err != nil {
t.Errorf("%s: Could not create identity: %v", k, err)
continue
}
}
if testcase.CreateMapping != nil {
clusterAdminClient.UserIdentityMappings().Update(testcase.CreateMapping)_, err :=
if err != nil {
t.Errorf("%s: Could not create mapping: %v", k, err)
continue
}
}
// Spawn 5 simultaneous mappers to test race conditions
var wg sync.WaitGroup
for i := 0; i < 5; i++ {
@smarterclayton tell me this doesn't make you happy
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1450/files#r27328280
} | ||
|
||
// Create creates a new user. Returns the server's representation of the user and error if one occurs. | ||
func (c *identities) Create(user *userapi.Identity) (result *userapi.Identity, err error) { |
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.
identity, not user.
lgtm. Squash in comments and merge. |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1345/) (Image: devenv-fedora_1164) |
Evaluated for origin up to 1919eda |
Merged by openshift-bot
Did the UPSTREAM patches make it upstream? |
@smarterclayton "Fix namespace on delete" was already upstream. The other got bogged down here: kubernetes/kubernetes#6201 |
…service-catalog/' changes from aa27078754..510060232e 510060232e origin build: add origin tooling de45e94 v0.1.0 chart changes (openshift#1468) 0bb9982 Modify Makefile to only specify ldflags once (openshift#1471) 5d6afac Fixes openshift#735: Add repo-sync script for charts (openshift#1453) 630f13f fix lingering unversioned client API (openshift#1466) 6f49128 Fix several logging errors (openshift#1464) 2aece61 Delete removed serviceClasses when they have no instances left (openshift#1450) 179d302 Uncommenting UID field after updating to k8s 1.8 (openshift#1457) b70c076 Reorder class and plan creation; test plan conflict handling (openshift#1459) 4bea012 Use versioned client APIs (openshift#1458) ff4af30 clean up logic for 410 gone deprovision poll (openshift#1452) 3fddf27 clean up logic and fix message for failed poll (openshift#1451) 40926cd Fix typo from openshift#1354 (openshift#1456) ff86ef2 Delete removed serviceplans when they have no instances left (openshift#1444) 8411a16 tweak binding setAndUpdateOrphanMitigation function (openshift#1448) ce28252 Combine apiserver and controller-manager into a single service-catalog image (openshift#1343) 7bbc8ee Check service class / plan before allowing provisioning or plan changes. (openshift#1439) baf28de Create listers before adding event handlers in controller (openshift#1446) 294157d remove setServiceBindingCondition dependency on controller (openshift#1441) 118a0f7 Fix typo in validation (openshift#1447) 117bfbd clean up error logging (openshift#1443) dff470f Move "External" around in some resource names/properties (openshift#1354) 0885edb Adding expectedGot function and using it. (openshift#1440) a7d582e Pretty controller broker (openshift#1442) c5edfaf Set apimachinery build variables with semver info (openshift#1429) 0e90d82 Add a pretty formatter for ClusterService[Class|Plan] (openshift#1408) fb874df Remove deprecated basic auth config support (openshift#1431) f4cd181 Migrate to metav1 methods for manipulating controllerRefs (openshift#1433) 96b286e Make service/plan reference fields on instance spec selectable (openshift#1422) 33f2b04 First example using the pretty context builder. (openshift#1403) 7852917 Stop using corev1.ObjectReference and corev1.LocalObjectReference (openshift#1417) fcf9480 Add tests for plan updates (openshift#1412) 819332e Add root CAs (openshift#1419) b49a76a Clean Makefile a little (openshift#1399) d681da0 Use a separate etcd prefix for each integration test to keep tests isolated (openshift#1415) 314a622 Wire etcd prefix to storage and call complete with options (openshift#1394) REVERT: aa27078754 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 510060232e54eb64b294213bb5d7847e169a2fac
…service-catalog/' changes from 892b0368f0..3064247d05 3064247d05 origin build: add origin tooling 48ecff1 Chart changes for 0.1.2 (openshift#1527) 8727247 Fix change validator to look up serviceclass properly (openshift#1518) b0b138b Broker Reconciliation occurs too frequently (openshift#1514) cc816eb When a SI is unbindable, mark the binding request failed. (openshift#1522) 41290e9 Clarify semantics around RelistDuration (openshift#1516) 6bcb593 Send Context with UpdateInstanceRequest (openshift#1517) c3b72d8 Enforce stricly increasing broker relistRequests (openshift#1515) 01d81e5 Follow up from openshift#1450 and openshift#1444 (openshift#1504) 5e77882 Retry failed deprovision requests (openshift#1505) f6c891d Allow updates of instances that failed a previous update. (openshift#1502) 5107086 Use Plan ID from ExternalProperties when deprovisioning instance (openshift#1501) d897c60 Adding message builder for expected event strings (openshift#1465) 9f6152e update osb client and freeze gorilla context (openshift#1496) c63277e Add unit tests verifying deleting a resource with an on-going operation or in orphan mitigation. (openshift#1490) 4ecca16 Pretty logging for controller_instance.go (openshift#1472) c232db4 Register qemu statics when building non-amd64 images (openshift#1494) 0e54c57 fix 'Spring Cloud Services -> Configuring with Vault' link in docs (openshift#1495) REVERT: 892b0368f0 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 3064247d0544aa43f976d93caea0a11771434ef7
@deads2k still working on fixing integration tests, but the rest is ready to start looking at
Follow-ups:
UserFor
should return a user that knows its groups