Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-71955

GitLab branch source scan fails with null pointer exception if credentials are not provided

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major

      A GitLab branch source project fails its initial scan with a null pointer exception. The message is:

      Checking project markewaite-jenkins-plugins/tasks-pipeline
      Proposing markewaite-jenkins-plugins/tasks-pipeline
      ERROR: Failed to create or update a subproject markewaite-jenkins-plugins/tasks-pipeline
      java.lang.NullPointerException: Cannot invoke "java.lang.Boolean.booleanValue()" because the return value of "org.gitlab4j.api.models.Project.getMergeRequestsEnabled()" is null
      	at io.jenkins.plugins.gitlabbranchsource.GitLabSCMSource.retrieve(GitLabSCMSource.java:334)
      	at jenkins.scm.api.SCMSource._retrieve(SCMSource.java:372)
      	at jenkins.scm.api.SCMSource.fetch(SCMSource.java:326)
      	at jenkins.branch.MultiBranchProjectFactory$BySCMSourceCriteria.recognizes(MultiBranchProjectFactory.java:262)
      	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.recognizes(OrganizationFolder.java:1361)
      	at jenkins.branch.OrganizationFolder$SCMSourceObserverImpl$1.complete(OrganizationFolder.java:1376)
      	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:252)
      	at jenkins.scm.api.trait.SCMNavigatorRequest.process(SCMNavigatorRequest.java:202)
      	at io.jenkins.plugins.gitlabbranchsource.GitLabSCMNavigator.visitSources(GitLabSCMNavigator.java:305)
      	at jenkins.branch.OrganizationFolder.computeChildren(OrganizationFolder.java:535)
      	at com.cloudbees.hudson.plugins.folder.computed.ComputedFolder.updateChildren(ComputedFolder.java:269)
      	at com.cloudbees.hudson.plugins.folder.computed.FolderComputation.run(FolderComputation.java:167)
      	at jenkins.branch.OrganizationFolder$OrganizationScan.run(OrganizationFolder.java:920)
      	at hudson.model.ResourceController.execute(ResourceController.java:101)
      	at hudson.model.Executor.run(Executor.java:442)
      

      The job definition is available as job-definition.xml

          [JENKINS-71955] GitLab branch source scan fails with null pointer exception if credentials are not provided

          Mark Waite added a comment -

          Same failure is also visible on a similarly configured Jenkins 2.414.1 with Java 11.0.20.

          Jenkins: 2.414.1
          OS: Linux - 6.2.0-26-generic
          Java: 11.0.20 - Eclipse Adoptium (OpenJDK 64-Bit Server VM)
          

          Mark Waite added a comment - Same failure is also visible on a similarly configured Jenkins 2.414.1 with Java 11.0.20. Jenkins: 2.414.1 OS: Linux - 6.2.0-26-generic Java: 11.0.20 - Eclipse Adoptium (OpenJDK 64-Bit Server VM)

          markewaite I have been able to reproduce this but only without using a proper token to access the gitlab api in my gitlab server configuration. Also I noticed several 401 errors on my log when I was able to reproduce the error. The issue was removed once I added a valid token to my gitlab server conf.

          Also there are tests in https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/plugins/GitLabPluginTest.java that check you can create a org and a multibranch pipeline without issues.

          Can you please check in your logs for 401 errors? Or check the token you use in your gitlab server conf to see if that is the cause of the issue?

          Raul Arabaolaza added a comment - markewaite I have been able to reproduce this but only without using a proper token to access the gitlab api in my gitlab server configuration. Also I noticed several 401 errors on my log when I was able to reproduce the error. The issue was removed once I added a valid token to my gitlab server conf. Also there are tests in https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/plugins/GitLabPluginTest.java that check you can create a org and a multibranch pipeline without issues. Can you please check in your logs for 401 errors? Or check the token you use in your gitlab server conf to see if that is the cause of the issue?

          Mark Waite added a comment - - edited

          rarabaolaza I had missed that my authentication token was null for the GitLab server that I had configured. I see the NPE when the authentication token is not defined. The scan works as expected when I use a valid authentication token. Thanks for the investigation!

          Mark Waite added a comment - - edited rarabaolaza I had missed that my authentication token was null for the GitLab server that I had configured. I see the NPE when the authentication token is not defined. The scan works as expected when I use a valid authentication token. Thanks for the investigation!

          Then I believe we can close this ticket, arguably we can ask for a better UX in the case the token is missing or not valid, but that would be another issue I believe?

          Raul Arabaolaza added a comment - Then I believe we can close this ticket, arguably we can ask for a better UX in the case the token is missing or not valid, but that would be another issue I believe?

          Mark Waite added a comment -

          Would you be OK if I left this ticket open and noted that a null pointer exception is reported when the GitLab credentials are not provided? I think that a null pointer exception is not a good user experience.

          Mark Waite added a comment - Would you be OK if I left this ticket open and noted that a null pointer exception is reported when the GitLab credentials are not provided? I think that a null pointer exception is not a good user experience.

          No issues on my side, I do agree is a very bad experience, for what I see the token is if not mandatory almost mandatory so there should be some hint in the UI and also the code should wrap those 401 exceptions more nicely.

          But as this is no longer a big priority for me I am gonna unassign myselfin case some other wants to work on it

          Raul Arabaolaza added a comment - No issues on my side, I do agree is a very bad experience, for what I see the token is if not mandatory almost mandatory so there should be some hint in the UI and also the code should wrap those 401 exceptions more nicely. But as this is no longer a big priority for me I am gonna unassign myselfin case some other wants to work on it

          Manan added a comment -

          rarabaolaza Can I pick this up?

           

          Manan added a comment - rarabaolaza Can I pick this up?  

          Mark Waite added a comment -

          manan_goel you are welcome to propose a pull request to fix it.

          Mark Waite added a comment - manan_goel you are welcome to propose a pull request to fix it.

          Manan added a comment -

          Hi markewaite , thanks for the quick response. imo the validation check for the existence of gitlab credentials should live somewhere in the [gitlab-branch-source-plugin|https://github.com/jenkinsci/gitlab-branch-source-plugin/tree/7dde70ed15224ec60c76b9699fc73ad4b6afbd5f.] 

          I think having a validation check while building the GitLab API, more precisely here would make sense. Would love to get your thoughts.

          Manan added a comment - Hi markewaite , thanks for the quick response. imo the validation check for the existence of gitlab credentials should live somewhere in the [gitlab-branch-source-plugin| https://github.com/jenkinsci/gitlab-branch-source-plugin/tree/7dde70ed15224ec60c76b9699fc73ad4b6afbd5f .]  I think having a validation check while building the GitLab API, more precisely here would make sense. Would love to get your thoughts.

          Mark Waite added a comment - - edited

          manan_goel I would look at the GitHub Branch Source plugin as a first example of the preferred technique. The GitHub Branch Source plugin shows the message "Credentials are recommended" when credentials are not provided, but then allows the user to continue without credentials.

          The Gitea plugin is another example you could use for the technique.

          I think that the GitLab branch source should strongly suggest use of a credential, but should not require it. That's how the other branch sources behave in Jenkins.

          Mark Waite added a comment - - edited manan_goel I would look at the GitHub Branch Source plugin as a first example of the preferred technique. The GitHub Branch Source plugin shows the message "Credentials are recommended" when credentials are not provided, but then allows the user to continue without credentials. The Gitea plugin is another example you could use for the technique. I think that the GitLab branch source should strongly suggest use of a credential, but should not require it. That's how the other branch sources behave in Jenkins.

          Manan added a comment -

          That makes sense, will take a look

          Manan added a comment - That makes sense, will take a look

          Manan added a comment -

          Hey markewaite, based on the Gitea plugin, I've come up with an implementation to validate credentials. I can't quite figure out how the frontend and the backend interact with each other. I was wondering if you could direct me in the right direction.

           

          Manan added a comment - Hey markewaite , based on the Gitea plugin, I've come up with an implementation to validate credentials. I can't quite figure out how the frontend and the backend interact with each other. I was wondering if you could direct me in the right direction.  

          Mark Waite added a comment - - edited

          manan_goel thanks for the implementation. That's a great start and that's just the way that I would have done it. Look at an existing implementation, reuse it as much as possible. Well done.

          I ran your code with the command:

          mvn clean -Djenkins.version=2.426.3 hpi:run
          

          There is an important typo in your implementation. Change doCheckCredentialId to doCheckCredentialsId and you'll then see that when creating a new GitLab multibranch Pipeline, it will prompt that a credential is recommended. With that change, I was able to see the "Credentials are recommended" message when I created a GitLab multibranch Pipeline without credentials.

          The same change will be needed for GitLab organization folders. When I use your code to create a GitLab organization folder, no message is displayed. I assume that means a doCheckCredentialsId implementation is needed in the organization folder class as well.

          You'll also need to update the code to allow empty credentials and not report a null pointer exception, but that can come later.

          Mark Waite added a comment - - edited manan_goel thanks for the implementation. That's a great start and that's just the way that I would have done it. Look at an existing implementation, reuse it as much as possible. Well done. I ran your code with the command: mvn clean -Djenkins.version=2.426.3 hpi:run There is an important typo in your implementation. Change doCheckCredentialId to doCheckCredentialsId and you'll then see that when creating a new GitLab multibranch Pipeline, it will prompt that a credential is recommended. With that change, I was able to see the "Credentials are recommended" message when I created a GitLab multibranch Pipeline without credentials. The same change will be needed for GitLab organization folders. When I use your code to create a GitLab organization folder, no message is displayed. I assume that means a doCheckCredentialsId implementation is needed in the organization folder class as well. You'll also need to update the code to allow empty credentials and not report a null pointer exception, but that can come later.

          Manan added a comment -

          Thanks for the quick review! I have replicated the same for gitlab organization folders as well and it is working! Is this where the tests for this would go ?

          For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          Manan added a comment - Thanks for the quick review! I have replicated the same for gitlab organization folders as well and it is working! Is this where the tests for this would go ? For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          Mark Waite added a comment - - edited

          Is this where the tests for this would go ?

          I'm not very confident in asserting where a test should be placed for the doCheckCredentialsId method. I'm not especially confident that writing an automated test of that method is a good use of your time. Some examples of automated tests of those types of methods are available in:

          If you can create an automated test, then that is a good thing. If it is terribly painful to create the automated tests, then I think the maintainers of the plugin might be persuaded to accept the change based on a description of the interactive testing that you performed.

          For not reporting the null pointer exception, would we want to keep it within the scope of this PR?

          I'm less concerned about whether it is in the scope of that pull request than whether it is in the scope of this issue. I think it is in the scope of this issue because the issue is specifically reporting a null pointer exception that is reported to the user. If you'd rather use two pull requests to resolve this issue, I think that is a reasonable choice. The pull request you're currently developing will improve the user experience without resolving the null pointer exception. That improvement is a very good thing. If you use a second pull request to avoid the null pointer exception, that is also a very good thing.

          Mark Waite added a comment - - edited Is this where the tests for this would go ? I'm not very confident in asserting where a test should be placed for the doCheckCredentialsId method. I'm not especially confident that writing an automated test of that method is a good use of your time. Some examples of automated tests of those types of methods are available in: AssemblaWebDoCheckURLTest UserRemoteConfigRefSpecTest NodeLabelBuildParameterTest If you can create an automated test, then that is a good thing. If it is terribly painful to create the automated tests, then I think the maintainers of the plugin might be persuaded to accept the change based on a description of the interactive testing that you performed. For not reporting the null pointer exception, would we want to keep it within the scope of this PR? I'm less concerned about whether it is in the scope of that pull request than whether it is in the scope of this issue. I think it is in the scope of this issue because the issue is specifically reporting a null pointer exception that is reported to the user. If you'd rather use two pull requests to resolve this issue, I think that is a reasonable choice. The pull request you're currently developing will improve the user experience without resolving the null pointer exception. That improvement is a very good thing. If you use a second pull request to avoid the null pointer exception, that is also a very good thing.

          Manan added a comment -

          Hey markewaite , I have raised a PR here. I'm continuing to try to automate the test for this, thanks for sharing examples for the same.

          I am going to pick up the Null pointer exception in a separate issue

          Manan added a comment - Hey markewaite , I have raised a PR here. I'm continuing to try to automate the test for this, thanks for sharing examples for the same. I am going to pick up the Null pointer exception in a separate issue

          Mark Waite added a comment -

          Very good manan_goel. I'm about to leave town for two weeks. I won't be much help until I return. Hopefully one of the maintainers of the GitLab branch source plugin will review the pull request.

          Mark Waite added a comment - Very good manan_goel . I'm about to leave town for two weeks. I won't be much help until I return. Hopefully one of the maintainers of the GitLab branch source plugin will review the pull request.

          Manan added a comment -

          Thanks for all the help so far! I really appreciate it!

          Manan added a comment - Thanks for all the help so far! I really appreciate it!

          Jagrat added a comment -

          this bug is related to plugin https://github.com/jenkinsci/gitlab-branch-source-plugin/blob/master/src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSCMSource.java

          where mergeRequestsEnabled is null in specific use case so all there is needed is to have a null value check and a output indecating that there was some error connecting to the gitlab server or such 

          Jagrat added a comment - this bug is related to plugin https://github.com/jenkinsci/gitlab-branch-source-plugin/blob/master/src/main/java/io/jenkins/plugins/gitlabbranchsource/GitLabSCMSource.java where mergeRequestsEnabled is null in specific use case so all there is needed is to have a null value check and a output indecating that there was some error connecting to the gitlab server or such 

            the_jsc_ Jagrat
            markewaite Mark Waite
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated: