Remove the code serving the oauth and user APIs #154
Conversation
|
could we hold this PR until we change the operators so that |
|
/hold |
openshift-ci-robot
added
do-not-merge/hold
openshift-ci-robot
removed
the
needs-rebase
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
go.mod
Outdated
| @@ -29,6 +29,7 @@ require ( | |||
| github.com/openshift/build-machinery-go v0.0.0-20200917070002-f171684f77ab | |||
| github.com/openshift/client-go v0.0.0-20201123133249-10558821f936 | |||
| github.com/openshift/library-go v0.0.0-20201102091359-c4fa0f5b3a08 | |||
| github.com/openshift/oauth-apiserver v0.0.0-20201022180844-09d7ea02b5ae | |||
There was a problem hiding this comment.
It turns out that OAS reused some uservalidation that moved along with the API to the other server. Maybe apiserver-library-go would be the right place to share this code?
09845a1#diff-d5e126898d69f39400e851ed5f5effbb63db8391c3492d8014f06d3eaba73585R13
There was a problem hiding this comment.
It's all the additions in the 09845a1 commit.
Namely:
- requestlimit admission (appears to be limiting the number of allowed projects?)
- the legacy openshift authorization API + its conversion and conversion testing
- serialization tests
There was a problem hiding this comment.
Where is requestlimit admission used in oauth-apiserver?
the legacy openshift authorization API + its conversion and conversion testing
used where?
There was a problem hiding this comment.
Or in other words, which pieces could we change / split / duplicate to cut the dependency?
The need of concersion is often a smell of bad factoring of the code. No other code other than the serving API server should need conversion.
There was a problem hiding this comment.
the legacy openshift authorization API + its conversion and conversion testing
validation.ValidateUserName,validation.ValidateGroupNamein conversions to be able to convert the kind of the subjects, and it's used in validation to validate the same
Where is requestlimit admission used in oauth-apiserver?
It's not, the username validation from the oauth-apiserver is used in requestlimit admission here to determine whether the user requesting projects is a system user or openshift user
Or in other words, which pieces could we change / split / duplicate to cut the dependency?
we could factor out username and groupname validation
There was a problem hiding this comment.
In seriallization_test.go we can certainly get rid of ValidateUserName/ValidateGroupName completely by just using valid names (e.g. validusername%d) directly.
But in conversion it's not that easy. Better move them to apiserver-library-go.
| @@ -424,18 +423,6 @@ func originFuzzer(t *testing.T, seed int64) *fuzz.Fuzzer { | |||
| j.Spec.Template.Spec.InitContainers = nil | |||
| j.Status.Template.Spec.InitContainers = nil | |||
| }, | |||
| func(j *oauthapi.OAuthAuthorizeToken, c fuzz.Continue) { | |||
There was a problem hiding this comment.
do we have the serialization_test in oauth-apiserver now?
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stlaz, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
The code moved to the oauth-apiserver. Uservalidation used here is vendored from there for now.
/cc @sttts @p0lyn0mial