Add configuration of access_level for runners on registration
What does this MR do?
Allows users to use the gitlab-runner register --access-level="ref_protected"
to be able to set the access-level at registration rather than via the UI/API. This speeds up the registration process and also helps to close a minor security issue between the registration of a runner and the time when it is set to protected via UI/API.
See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27490 for the Rails side of the code to accept the API request from gitlab-runner
Why was this MR needed?
See #3186 (closed)
Are there points in the code the reviewer needs to double check?
Does this MR meet the acceptance criteria?
-
Documentation created/updated -
Added tests for this feature/bug -
In case of conflicts with master
- branch was rebased
What are the relevant issue numbers?
Closes #3186 (closed)
Merge request reports
Activity
added Community contribution label
added 1st contribution label
@ayufan Could you please take a look at this MR? It's the counterpart to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/27490 which you approved last week. Thanks
!Edited by Zelin LPlease @steveazz od @tmaczukin :)
Ah ok - @tmaczukin, could you please take a look? Thanks!
@tmaczukin , What do we need to do to get this reviewed? This is coming from a great Premium customer.
added customer label
@zelin-l Thank you for your contribution, both on GitLab CE side and here :)
This MR looks good. I have only one concern: most of the registration related options require no values (boolean flags) or are accepting any custom-defined value (like the description or tags which fully depends on what the user want to have there). But the setting that you're adding here requires a specific values to work properly.
I'm thinking if we should maybe:
- define consts containing allowed values (currently
not_protected
andref_protected
), - prepare a slice that contains all the allowed values,
- add a validation that will check if the value provided by user is within the list of allowed ones - and if not it will fail registration without doing any request to GitLab.
My point is that with current implementation I could do
gitlab-runner register --access-level some-invalid-value (...)
and it would be sent to GitLab like that. Of course, GitLab's validation would deny such request but with the short list of possible settings we have an opportunity to check this in advance.And we already have some dictionaries that are shared between GitLab's and GitLab Runner's codebase, for example the list of failure reasons.
- define consts containing allowed values (currently
Thanks for the review @tmaczukin :) - added the changes to validate the access level on the gitlab-runner side and fail if the access level is provided and incorrect. Let me know if there are any other changes needed.
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
- Resolved by Tomasz Maczukin
@zelin-l Last few comments, mostly cosmetic ones. I'd also like to add a new test to
commands/register_test.go
for the newly added code:func TestAccessLevelSetting(t *testing.T) { tests := map[string]struct { accessLevel AccessLevel failureExpected bool }{ "access level not defined": {}, "ref_protected used": { accessLevel: Protected, }, "not_protected used": { accessLevel: NotProtected, }, "unknown access level": { accessLevel: AccessLevel("unknown"), failureExpected: true, }, } for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { hook := test.NewGlobal() defer func() { logrusOutput := getLogrusOutput(t, hook) r := recover() if r == nil { assert.Contains(t, logrusOutput, "Runner registered successfully.") assert.NotContains(t, logrusOutput, "Given access-level is not valid. Please refer to gitlab-runner register -h for the correct options.") return } if _, ok := r.(*logrus.Entry); ok { assert.NotContains(t, logrusOutput, "Runner registered successfully.") assert.Contains(t, logrusOutput, "Given access-level is not valid. Please refer to gitlab-runner register -h for the correct options.") return } assert.Fail(t, fmt.Sprintf("Unexpected panic: %v", r)) }() network := new(common.MockNetwork) defer network.AssertExpectations(t) if !testCase.failureExpected { parametersMocker := mock.MatchedBy(func(parameters common.RegisterRunnerParameters) bool { return AccessLevel(parameters.AccessLevel) == testCase.accessLevel }) network.On("RegisterRunner", mock.Anything, parametersMocker). Return(&common.RegisterRunnerResponse{ Token: "test-runner-token", }). Once() } configFile, err := ioutil.TempFile("", "config.toml") require.NoError(t, err) configFile.Close() defer os.Remove(configFile.Name()) arguments := []string{ "-n", "--config", configFile.Name(), "--url", "http://gitlab.example.com/", "--registration-token", "test-registration-token", "--executor", "shell", "--access-level", string(testCase.accessLevel), } testRegisterCommandRun(t, network, arguments...) }) } }
Let's just put this at the end of the test file.
added 1 commit
- 9b84001e - Added test for access-level (tmaczukin) and renamed variables
Thanks @tmaczukin for the test! I added the changes you mentioned above. Let me know if you want me to squash the commits and repush.
added 1 commit
- 09f50645 - Added test for access-level (tmaczukin) and renamed variables
added 1 commit
- fc643667 - Added test for access-level (tmaczukin) and renamed variables
changed milestone to %12.0
assigned to @tmaczukin
343 359 } 344 360 } 345 361 362 func accessLevelValid(levels []AccessLevel, givenLevel string) bool { Let's make types here consistent:
diff --git a/commands/register.go b/commands/register.go index 691c5d2aa..bf6c1e7a4 100644 --- a/commands/register.go +++ b/commands/register.go @@ -291,7 +291,7 @@ func (s *RegisterCommand) Execute(context *cli.Context) { } validAccessLevels := []AccessLevel{NotProtected, RefProtected} - if !accessLevelValid(validAccessLevels, s.AccessLevel) { + if !accessLevelValid(validAccessLevels, AccessLevel(s.AccessLevel)) { logrus.Panicln("Given access-level is not valid. " + "Please refer to gitlab-runner register -h for the correct options.") } @@ -359,13 +359,13 @@ func newRegisterCommand() *RegisterCommand { } } -func accessLevelValid(levels []AccessLevel, givenLevel string) bool { +func accessLevelValid(levels []AccessLevel, givenLevel AccessLevel) bool { if givenLevel == "" { return true } for _, level := range levels { - if AccessLevel(givenLevel) == level { + if givenLevel == level { return true } }
I should have notice this at the first review...
changed this line in version 7 of the diff
@zelin-l Last one comment and we should be ready to merge this :)
@tmaczukin Made the changes above. Thanks for taking the time go through so many review iterations and providing helpful feedback :)
Edited by Zelin Lmentioned in merge request !1387 (closed)
Thanks @zelin-l. It looks good, so let's merge it :)
Ah, I haven't noticed this: @zelin-l there are merge conflicts. Please rebase your MR on top of current
master
and remove the conflicts :)added 198 commits
-
e051d850...0634e6a5 - 194 commits from branch
gitlab-org:master
- 88c61c7f - Add configuration of access_level for runners on registration
- c30fc565 - Validate AccessLevel CLI flag
- 7bfba1c3 - Added test for access-level (tmaczukin) and renamed variables
- e1341681 - Retyped givenLevel to AccessLevel
Toggle commit list-
e051d850...0634e6a5 - 194 commits from branch
@tmaczukin Rebased and resolved the merge conflicts :)
mentioned in commit 68342b81
mentioned in issue #3186 (closed)
mentioned in merge request !2137 (merged)
added linked-issue label