Skip to content
Snippets Groups Projects

Add configuration of access_level for runners on registration

1 unresolved thread

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @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.

  • Zelin L added 1 commit

    added 1 commit

    • 9b84001e - Added test for access-level (tmaczukin) and renamed variables

    Compare with previous version

  • 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.

  • Zelin L added 1 commit

    added 1 commit

    • 09f50645 - Added test for access-level (tmaczukin) and renamed variables

    Compare with previous version

  • Zelin L added 1 commit

    added 1 commit

    • fc643667 - Added test for access-level (tmaczukin) and renamed variables

    Compare with previous version

  • Steve Xuereb changed milestone to %12.0

    changed milestone to %12.0

  • Tomasz Maczukin resolved all discussions

    resolved all discussions

  • 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

    • Please register or sign in to reply
  • @zelin-l Last one comment and we should be ready to merge this :)

  • Zelin L added 1 commit

    added 1 commit

    • e051d850 - Retyped givenLevel to AccessLevel

    Compare with previous version

  • @tmaczukin Made the changes above. Thanks for taking the time go through so many review iterations and providing helpful feedback :)

    Edited by Zelin L
  • Tomasz Maczukin mentioned in merge request !1387 (closed)

    mentioned in merge request !1387 (closed)

  • Tomasz Maczukin approved this merge request

    approved this merge request

  • 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 :)

  • Zelin L added 198 commits

    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

    Compare with previous version

  • Zelin L added 1 commit

    added 1 commit

    • a244c80e - Retyped givenLevel to AccessLevel

    Compare with previous version

  • @tmaczukin Rebased and resolved the merge conflicts :)

  • Tomasz Maczukin approved this merge request

    approved this merge request

  • Tomasz Maczukin mentioned in commit 68342b81

    mentioned in commit 68342b81

  • joe d mentioned in issue #3186 (closed)

    mentioned in issue #3186 (closed)

  • Julianne Gendrano mentioned in merge request !2137 (merged)

    mentioned in merge request !2137 (merged)

  • Please register or sign in to reply
    Loading