Gitlab Source: Backoff from Scan2 which is experimental to legacy pagination API call #4608
Conversation
This commit rewrite simplifiedGitlabEnumeration to use legacy pagination API call with keyset pagination instead of Scan2 which is currently in experimental state. Note that this doesn't promise to fix this problem it's just a test to check. It also adds a retry logic in case any 500 error occurs. I added some logs as well to keep track of no of projects being enumerated.
kashifkhan0771
changed the title
martinlocklear
left a comment
martinlocklear
left a comment
There was a problem hiding this comment.
Thanks for looking at this! Some notes:
- Definitely need test coverage for this before merging in.
- I notice in our go.mod we're overriding the version of the library we're using. Is it possible that some of the issues that we've been seeing could be fixed by newer versions of this library?
kashifkhan0771
force-pushed
the
fix/ins-191
branch
from
25a7098 to
815ac8b
Compare
kashifkhan0771
force-pushed
the
fix/ins-191
branch
from
2e0dc27 to
4696e76
Compare
martinlocklear
left a comment
martinlocklear
left a comment
There was a problem hiding this comment.
I think we still need to add unit testing before we merge this.
Reversed the gitlab cloud logic to add membership flag, so that we use the default false for non gitlab.com instances. This can help if the issue really was membership flag as mentioned in some gitlab issues. Also added simple flag in list projects to get only minimal fields in response instead of big json response for each project. Added test case as well.
Added the test case |
camgunz
left a comment
camgunz
left a comment
There was a problem hiding this comment.
LGTM! If you want you could add a couple more checks in the test, like I'm pretty sure some other sources check chunk count and so on. But up to you--I know you've done a lot of local testing w/ this.
I just had a look, and while the changelog doesn't go as far back as our pinned version, I didn't see any bug fix that would've affected us. I made INS-249 to do this some day. |
Somehow I missed this comment. The reason we did that was the latest version at that time was v1.134.0 but it deprecated existing Auth methods, also the Scan2 API was experimental so we pinned to version to avoid any new updates which accidentally break the API. As Charlie said there isn't any bug fix in later versions, so I would prefer to update that in a separate PR. |
| // use simplified gitlab enumeration | ||
| feature.UseSimplifiedGitlabEnumeration.Store(true) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*30) |
There was a problem hiding this comment.
Just use the testing context, I think it's t.Context()
There was a problem hiding this comment.
t.Context() returns the standard library context.Context, but this codebase uses TruffleHog’s custom context.Context (pkg/context) which extends the standard context with additional methods. Because of that, t.Context() cannot be used here - the types are incompatible.
|
Some non-blocking comments about the new test |
kashifkhan0771
force-pushed
the
fix/ins-191
branch
from
afabb72 to
1002e52
Compare
4 participants
This PR refactors simplifiedGitlabEnumeration to use the legacy pagination API with keyset pagination instead of Scan2, which is currently in an experimental state. This change is intended as an experiment and does not guarantee a fix for the issue.
Additionally, it introduces retry logic to handle 500-level errors and adds logging to track the number of projects being enumerated.
Related issue on Gitlab: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/7516
Checklist:
make test-community)?make lintthis requires golangci-lint)?