Support both v4 and v3 GitLab User APIs #19961
Conversation
This change adds logic that attempts to use the v4 GitLab User API. If that fails, it falls back to the legacy v3 API. This will allow OpenShift's GitLab integration to work with newer versions of GitLab while still supporting older versions. Signed-off-by: Monis Khan <[email protected]>
|
@enj: GitHub didn't allow me to assign the following users: simo. Note that only openshift members and repo collaborators can be assigned. In response to this:
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
added
the
kind/bug
openshift-ci-robot
added
sig/security
size/M
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj 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
|
/hold 3.11 |
openshift-ci-robot
added
the
do-not-merge/hold
|
/assign @simo5 |
|
@enj: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| ) | ||
|
|
||
| type provider struct { | ||
| providerName string | ||
| transport http.RoundTripper | ||
| authorizeURL string | ||
| tokenURL string | ||
| userAPIURL string | ||
| userAPIURLV3 string | ||
| userAPIURLV4 string |
There was a problem hiding this comment.
I think we may want to keep just userAPIURL and populate it with V4 endpoint.
On the first attempt if we get a not found error we check if the URL is the V4 and we switch it to V3 and retry.
I do not think it makes sense to keep falling back at every request.
There was a problem hiding this comment.
That will require adding some form of locking.
There was a problem hiding this comment.
or an atomic holding an index to a 2 elements array with the two strings
| userdata, err := p.getUserDataURL(token, p.userAPIURLV4) | ||
| if err == nil { | ||
| return userdata, nil | ||
| } |
There was a problem hiding this comment.
See above, plus we shouldn't fall back for every error, but just for errors that indicate the endpoint does not exist.
If it does and it returns an error we should just return the error and not try any fallback.
There was a problem hiding this comment.
just for errors that indicate the endpoint does not exist
A 404?
| return p.getUserDataURL(token, p.userAPIURLV3) | ||
| } | ||
|
|
||
| func (p *provider) getUserDataURL(token, url string) (*gitlabUser, error) { |
There was a problem hiding this comment.
Sounds like wrapping getUserIdentity with a retry with fallback to V3 only on specific errors and only once and for all would makes for less code changes too.
There was a problem hiding this comment.
specific errors
Unclear to me what those errors are.
only once
Unclear to me how to do that without locking.
There was a problem hiding this comment.
specific errors -> 404 or other if gitlab returnssomething special for missing endpoints
only once -> do we have a way to atomically replace a pointer ?
|
@simo5 @liggitt with regards to https://gitlab.com/gitlab-org/gitlab-ce/issues/47565#note_80751232 and https://trello.com/c/DXntmEOV we may want to consider deprecating the use of older versions of GitLab and move the internal code to OIDC (and probably just closing this PR). |
If gitlab supports oidc, and we can pull matching identity info out of its userinfo/token claims, we should definitely collapse onto that rather than updating this. Documenting this is only for the v3 API seems fine to me |
|
@enj can we use the OIDC plugin without needing any other infrastructure work ? |
|
Per https://gitlab.com/gitlab-org/gitlab-ce/issues/47791#note_81269161 GitLab 11.1 will have OIDC support that works for OpenShift. @simo5 @liggitt how should we go about deprecating the use of older versions of GitLab? cc @kalexand-rh |
|
See #19997 |
|
Let us continue any discussion on #19997 |
This change adds logic that attempts to use the v4 GitLab User API. If that fails, it falls back to the legacy v3 API. This will allow OpenShift's GitLab integration to work with newer versions of GitLab while still supporting older versions.
Signed-off-by: Monis Khan [email protected]
Fixes #19937
/kind bug
Per https://gitlab.com/gitlab-org/gitlab-ce/issues/47565#note_80751232, this legacy v3 User API will not be removed.
/assign @liggitt @simo5
@openshift/sig-security
cc @alikhajeh1