gitlab: use gitlab instance runner to create runner

[clwluvw]

SUMMARY

When using project it will use project level runner to create runner that based on python-gitlab it will be used for enabling runner and needs a runner_id so for creating a new runner it should use gitlab level runner

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

gitlab_runner

[ansibullbot]

cc @Lunik @SamyCoenen @Shaps @dj-wasabi @marwatk @metanovii @scodeman @sh0shin @waheedi @zanssa
click here for bot help

[felixfontein]

Thanks for your contribution! This is a subset of the changes in #3935 (see https://github.com/ansible-collections/community.general/pull/3935/files#r775046631).

I think it's a good idea to have the bugfixes separate from the new feature; @lgatellier what do you think? Also #3935 has some more changes that seem to be bugfixes that aren't in this PR (https://github.com/ansible-collections/community.general/pull/3935/files#r775046714 and a few lines below).

I'm wondering whether these bugfixes are due to changes in GitLab's API though, since it did seem to work in the original PR that implemented the old behavior (#2971). CC @drzraf since they probably use this feature. I wonder whether this requires different behvaior depending on the GitLab version.

[lgatellier]

Hi,
I agree with you @felixfontein. Separating new feature & bugfixes will improve redability.
@clwluvw can you also fix lines 295 & 298 the same way you did it for L251 (see #3935) ?

Regarding an eventual Gitlab API evolution, I think it's a wrong use of the API.
The POST /projects/:id/runners endpoint exists, but it enables an already-registered runner on a new project (See docs here).

The create (resp. delete) function in python-gitlab library targets this runner-enabling (resp. disabling) endpoint, and not a registration endpoint.

[clwluvw]

@clwluvw can you also fix lines 295 & 298 the same way you did it for L251 (see #3935) ?

Done

[drzraf]

This commit is basically reverting 3 out of 5 reference to the "generic" variable (self._runners_endpoint) which is set to project.runners if project else gitlab_instance.runners
I immediately don't see which CRUD operation/corresponding module invocation would create a bug.

[felixfontein]

@drzraf did you see https://github.com/ansible-collections/community.general/pull/3935/files#r775269737 ?

[felixfontein]

@clwluvw can you please add a changelog fragment?

@drzraf would you mind testing this fix with your GitLab instance, to see whether it works with it? Apparently these changes are necessary to make the module work with some other instances.

[clwluvw]

@clwluvw can you please add a changelog fragment?

Done

[lgatellier]

This commit is basically reverting 3 out of 5 reference to the "generic" variable (self._runners_endpoint) which is set to project.runners if project else gitlab_instance.runners I immediately don't see which CRUD operation/corresponding module invocation would create a bug.

The endpoints /api/v4/projects/:id/runners and /api/v4/groups/:id/runners do not allow us to register a runner.
The first one (project-related) can be used to enable an existing runner on a specific project, but not to register a new one.

Example on a self-hosted Gitlab-EE 14.4.4 Premium instance :

# Registration attempt on a project as currently done by ansible gitlab_runner module, without this fix
$ curl -s -X POST --form "token=<project_runners_token>" --form "description=test-20220104" --form "locked=false" --form "active=false" --form "run_untagged=true" --form "access_level=not_protected" https://<my_gitlab_instance>/api/v4/projects/<project_id>/runners
{"message":"401 Unauthorized"}

# Registration attempt on a group as it would be done by ansible gitlab_runner module, without this fix
$ curl -s -X POST --form "token=<group_runners_token>" --form "description=test-20220104" --form "locked=false" --form "active=false" --form "run_untagged=true" --form "access_level=not_protected" https://<my_gitlab_instance>/api/v4/groups/<group_id>/runners
{"error":"404 Not Found"}

# The right call for runner registration
$ curl -s -X POST --form "token=<group_runners_token>" --form "description=test-20220104" --form "locked=false" --form "active=false" --form "run_untagged=true" --form "access_level=not_protected" https://<my_gitlab_instance>/api/v4/runners
{"id":<new_runner_id>,"token":"<new_runner_token>"}

Which Gitlab version did you use when you added and tested project runner registration feature ?

[drzraf]

The endpoints /api/v4/projects/:id/runners and /api/v4/groups/:id/runners do not allow us to register a runner.
[...]
Which Gitlab version did you use when you added and tested project runner registration feature ?

Thank you for the precision.

• I used to test it against GitLab.com (not a local instance) but probably GitLab << 10.2
• I used to test it using python-gitlab, probably <= 2.0.1 (at least when gitlab.Gitlab still accepted an email parameter)

Indeed, the endpoint https://gitlab.com/api/v4/projects/xxx/runners works for GET only. I don't know how I overlooked that (!) and totally agree with the changes you suggested.

Notes:

1. When trying the code today, I also got an error 'ProjectRunnerManager' object has no attribute 'all (onself._runners_endpoint.all()) using python-gitlab 2.10.1
2. Remember that updating the registration_token of an existing worker, while being a rare operation, wouldn't work (for the sake of having a real check-mode) [GitLab API does not provide the existing token]

[felixfontein]

@drzraf with "the code today", do you mean the code in this PR, or the code in the main branch?

[lgatellier]

Notes:

1. When trying the code today, I also got an error `'ProjectRunnerManager' object has no attribute 'all` (on`self._runners_endpoint.all()`) using python-gitlab **2.10.1**

I think there's something to do regarding projects endpoint and owned parameter (there's no /projects/:id/runners/all endpoint, only on /runners/all).

Maybe forcing owned to true when project parameter is present ? And defining owned and project as mutually_exclusive ?

[clwluvw]

unstale please

[felixfontein]

@drzraf @clwluvw what do you think about @lgatellier's comment?

[drzraf]

owned and project and not per-se mutually exclusive but having an actual use from both simultaneously seems like an edge-case. So making them mutually exclusive makes sense as it could solve the current existing issue quite easily.

[lgatellier]

OK, so if you're okay @drzraf @felixfontein, I suggest to validate this PR to fix the bug on project runner registration, and I'll open a new PR to change behavior between owned and project.

Then, I'll rebase & complete PR #3935 to properly add group feature.

[felixfontein]

With validate, do you mean approve/merge? If so, that's fine for me - I understand too little of this problem, so if all are happy with this I'll happily press the merge button :)

[lgatellier]

With validate, do you mean approve/merge? If so, that's fine for me - I understand too little of this problem, so if all are happy with this I'll happily press the merge button :)

Yes, that's what I mean, if @drzraf is okay ! ;-)

[felixfontein]

@drzraf would be great if you could look at this tomorrow, since the 4.4.0 release will be on Tuesday (morning), and it would be great if this doesn't have to wait another three weeks :)

[drzraf]

Totally fine with this!

[patchback]

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/929136808f670fb58dd9c13d717a7099bc9257e9/pr-3965

Backported as #4122

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/929136808f670fb58dd9c13d717a7099bc9257e9/pr-3965

Backported as #4123

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@clwluvw thanks for fixing this!
@lgatellier @drzraf thanks for reviewing and testing!