Add Docker login to Jenkins script to avoid rate limit issue #6368

XinShuYang · 2024-05-24T07:04:55Z

Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues

XinShuYang · 2024-05-24T07:09:14Z

/test-e2e

XinShuYang · 2024-05-24T07:27:32Z

/test-e2e

tnqn · 2024-05-24T08:00:19Z

ci/jenkins/test.sh

@@ -984,6 +994,7 @@ if [[ $TESTCASE =~ "multicast" ]]; then
 fi

 source $WORKSPACE/ci/jenkins/utils.sh
+docker_login "${dockerUser}" "${dockerPassword}"


Not sure if there is such convention: should it disable xtrace first before using the secret, otherwise it would be printed to terminal by enabling xtrace in other place in this script and it won't be easy to notice.
The following code should ensure it won't be printed:

( set +x docker_login "${dockerUser}" "${dockerPassword}" )

@antoninbas do you know how is this usually handled?

The secret is stored in Jenkins credentials and will not be exposed in the results. However, we should indeed avoid printing too much information about the login step, will update the code.

Personally I think like this should not even be in the script if possible.
Isn't there a way to configure Jenkins so that we login to Docker before even calling this script?

@antoninbas Do you mean to put this login script into Jenkins macros to avoid modifying the test.sh related scripts? Using this method, we would need to modify the macros definition for all related jobs. In the Nephe CI pipeline, we have verified a similar method to log into Docker as we did in this PR. https://github.com/antrea-io/nephe/blob/9faeba2c189821045e4ea6d37a1c97dbdecf67bd/ci/jenkins/nephe-ci.sh#L112

And I haven't found another way to log into Docker before running script in a freestyle job. It is possible to configure Docker login through a Jenkins pipeline job, but pipeline jobs are not compatible with the freestyle Jenkins setup we are currently using.

Personally I think like this should not even be in the script if possible. Isn't there a way to configure Jenkins so that we login to Docker before even calling this script?

Yes, Handling Docker login credentials within a script can be a security risk. It's generally better to manage authentication outside of the script . isn't the docker credentials be stored in jenkins credentials and used to access it using credentials in job itself, In that case it has be configured in each and every job then !

Personally I think like this should not even be in the script if possible. Isn't there a way to configure Jenkins so that we login to Docker before even calling this script?

Yes, Handling Docker login credentials within a script can be a security risk. It's generally better to manage authentication outside of the script . isn't the docker credentials be stored in jenkins credentials and used to access it using credentials in job itself, In that case it has be configured in each and every job then !

When using the Jenkins Credential secrets, you can securely inject sensitive credentials into the build process as environment variables. These variables are visible during use, but Jenkins automatically masks their values to prevent them from being displayed in the build logs.

After consideration, I placed the login script in the macro YAML, which ensures that the login step cannot be changed by any PR.

XinShuYang · 2024-05-24T08:26:33Z

/test-all

XinShuYang · 2024-05-24T09:33:42Z

/test-all

XinShuYang · 2024-05-24T15:23:19Z

/test-conformance

luolanzone · 2024-05-27T01:50:23Z

ci/jenkins/jobs/macros.yaml

@@ -103,7 +103,7 @@
          #!/bin/bash
          set -ex
          DOCKER_REGISTRY="$(head -n1 ci/docker-registry)"
-          ./ci/jenkins/test-vmc.sh --cluster-name "$BUILD_TAG" --testcase e2e --coverage --codecov-token "${CODECOV_TOKEN}" --registry ${DOCKER_REGISTRY} --username "${CAPVC_USERNAME}" --password "${CAPVC_PASSWORD}"
+          ./ci/jenkins/test-vmc.sh --cluster-name "$BUILD_TAG" --testcase e2e --coverage --codecov-token "${CODECOV_TOKEN}" --registry ${DOCKER_REGISTRY} --username "${CAPVC_USERNAME}" --password "${CAPVC_PASSWORD}" --dockerUser "${{DOCKER_USERNAME}}" --dockerPassword "${{DOCKER_PASSWORD}}"


Probably better to follow the same name pattern as others, e.g. '--docker-user' --docker-password

Sure, updated.

ci/jenkins/test-mc.sh

ci/jenkins/utils.sh

XinShuYang · 2024-05-27T10:29:07Z

/test-all

XinShuYang · 2024-05-28T01:40:32Z

/test-conformance

ci/jenkins/docker_login.sh

antoninbas · 2024-05-28T18:29:57Z

ci/jenkins/jobs/macros.yaml

+          ./ci/jenkins/docker_login.sh --docker-user "${{DOCKER_USERNAME}}" --docker-password "${{DOCKER_PASSWORD}}"
          DOCKER_REGISTRY="$(head -n1 ci/docker-registry)"


it's a little bit strange that we call docker_login.sh unconditionally, and then we run DOCKER_REGISTRY. In theory, if we have DOCKER_REGISTRY="$(head -n1 ci/docker-registry)", that means that DOCKER_REGISTRY is not guaranteed to be docker.io, but docker_login.sh assumes that the registry is docker.io.

Maybe these 2 instructions should be swapped, and docker_login.sh should return immediately if DOCKER_REGISTRY is not docker.io? Or some other solution that makes docker login conditional.

Yes it make sense, I add a registry flag for the login script.

XinShuYang · 2024-05-29T03:15:29Z

/test-all

XinShuYang · 2024-05-29T03:31:51Z

/test-all

ci/jenkins/docker_login.sh

tnqn · 2024-05-29T14:52:46Z

ci/jenkins/docker_login.sh

+    echo "Logging into Docker Hub..."
+    docker_login "${DOCKER_USERNAME}" "${DOCKER_PASSWORD}"
+else
+    echo "Registry $DOCKER_REGISTRY is not supported for login by this script."


should it exit with non-zero?

it feels to me that we should just avoid calling the script in this case.
If we use the following, it won't really be more verbose than what we have currently:

[ "$DOCKER_REGISTRY" == "docker.io" ] || ./ci/jenkins/docker_login.sh --docker-user ${{DOCKER_USERNAME}} --docker-password ${{DOCKER_PASSWORD}}

it feels to me that we should just avoid calling the script in this case. If we use the following, it won't really be more verbose than what we have currently:

[ "$DOCKER_REGISTRY" == "docker.io" ] || ./ci/jenkins/docker_login.sh --docker-user ${{DOCKER_USERNAME}} --docker-password ${{DOCKER_PASSWORD}}

Got the point, updated the macro script, PTAL.

@antoninbas I think it should be
[ "$DOCKER_REGISTRY" != "docker.io" ] || ./ci/jenkins/docker_login.sh --docker-user ${{DOCKER_USERNAME}} --docker-password ${{DOCKER_PASSWORD}} right?
The docker_login.sh should only be executed when $DOCKER_REGISTRY is docker.io

tnqn · 2024-05-29T14:53:15Z

ci/jenkins/docker_login.sh

+            echo "Docker login failed. Retrying"
+        fi
+    done
+    set -ex


shouldn't it exit with non-zero code if it never succeeds?

Yes, updated the script.

I think this will never be called?

XinShuYang · 2024-05-30T01:52:28Z

/test-all

XinShuYang · 2024-05-30T02:22:27Z

/test-all

tnqn · 2024-05-30T04:43:18Z

ci/jenkins/docker_login.sh

+            echo "Docker login failed. Retrying"
+        fi
+    done
+    set -ex


I think this will never be called?

tnqn · 2024-05-30T04:43:54Z

ci/jenkins/docker_login.sh

+esac
+done
+
+function docker_login() {


where is it called?

My bad, corrected it.

XinShuYang · 2024-05-30T04:51:28Z

/test-all

Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues Signed-off-by: Shuyang Xin <[email protected]>

XinShuYang · 2024-05-30T11:08:55Z

/test-all

Signed-off-by: Antonin Bas <[email protected]>

antoninbas

I added a commit to fix some issues in docker_login.sh
I want to merge this ASAP to restore CI

antoninbas · 2024-05-30T17:55:05Z

/test-all

…io#6368) * Add Docker login to Jenkins script to avoid rate limit issue Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues Signed-off-by: Shuyang Xin <[email protected]> * Fix some issues in docker_login.sh Signed-off-by: Antonin Bas <[email protected]> --------- Signed-off-by: Shuyang Xin <[email protected]> Signed-off-by: Antonin Bas <[email protected]> Co-authored-by: Antonin Bas <[email protected]>

…6439) * Add Docker login to Jenkins script to avoid rate limit issue Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues * Fix some issues in docker_login.sh Signed-off-by: Shuyang Xin <[email protected]> Signed-off-by: Antonin Bas <[email protected]>

…6440) * Add Docker login to Jenkins script to avoid rate limit issue Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues * Fix some issues in docker_login.sh Signed-off-by: Shuyang Xin <[email protected]> Signed-off-by: Antonin Bas <[email protected]>

XinShuYang force-pushed the dockerlogin branch from 95ccc1f to 6b0e783 Compare May 24, 2024 07:38

tnqn reviewed May 24, 2024

View reviewed changes

XinShuYang force-pushed the dockerlogin branch from 6b0e783 to bee41ab Compare May 24, 2024 08:24

XinShuYang force-pushed the dockerlogin branch from bee41ab to f2d80b6 Compare May 24, 2024 09:31

XinShuYang requested review from antoninbas, tnqn and luolanzone May 24, 2024 09:32

XinShuYang marked this pull request as ready for review May 24, 2024 09:32

luolanzone reviewed May 27, 2024

View reviewed changes

ci/jenkins/utils.sh Outdated Show resolved Hide resolved

XinShuYang force-pushed the dockerlogin branch 5 times, most recently from cc79745 to cbad3f4 Compare May 27, 2024 10:21

antoninbas reviewed May 28, 2024

View reviewed changes

XinShuYang force-pushed the dockerlogin branch 2 times, most recently from 429f614 to 98e81de Compare May 29, 2024 02:56

XinShuYang force-pushed the dockerlogin branch from 98e81de to 8a5a3b2 Compare May 29, 2024 03:31

tnqn reviewed May 29, 2024

View reviewed changes

XinShuYang force-pushed the dockerlogin branch from 8a5a3b2 to d941d0c Compare May 30, 2024 01:46

XinShuYang force-pushed the dockerlogin branch from d941d0c to c1a2626 Compare May 30, 2024 02:20

tnqn reviewed May 30, 2024

View reviewed changes

XinShuYang force-pushed the dockerlogin branch from c1a2626 to b9e8176 Compare May 30, 2024 04:50

XinShuYang requested a review from tnqn May 30, 2024 04:52

Add Docker login to Jenkins script to avoid rate limit issue

38e0646

Enhance Jenkins pipeline by integrating Docker credentials for login to avoid rate limit issues Signed-off-by: Shuyang Xin <[email protected]>

XinShuYang force-pushed the dockerlogin branch from b9e8176 to 38e0646 Compare May 30, 2024 11:07

Fix some issues in docker_login.sh

9e9145d

Signed-off-by: Antonin Bas <[email protected]>

antoninbas approved these changes May 30, 2024

View reviewed changes

antoninbas merged commit f7cce73 into antrea-io:main May 30, 2024
52 of 56 checks passed

luolanzone mentioned this pull request Jun 13, 2024

Automated cherry pick of #6368: Add Docker login to Jenkins script to avoid rate limit issue #6439

Merged

luolanzone mentioned this pull request Jun 13, 2024

Automated cherry pick of #6368: Add Docker login to Jenkins script to avoid rate limit issue #6440

Merged

		./ci/jenkins/docker_login.sh --docker-user "${{DOCKER_USERNAME}}" --docker-password "${{DOCKER_PASSWORD}}"
		DOCKER_REGISTRY="$(head -n1 ci/docker-registry)"

Add Docker login to Jenkins script to avoid rate limit issue #6368

Add Docker login to Jenkins script to avoid rate limit issue #6368

Uh oh!

Conversation

XinShuYang commented May 24, 2024

Uh oh!

XinShuYang commented May 24, 2024

Uh oh!

XinShuYang commented May 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang commented May 24, 2024

Uh oh!

XinShuYang commented May 24, 2024

Uh oh!

XinShuYang commented May 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

XinShuYang commented May 27, 2024

Uh oh!

XinShuYang commented May 28, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang commented May 29, 2024

Uh oh!

XinShuYang commented May 29, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang commented May 30, 2024

Uh oh!

XinShuYang commented May 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XinShuYang May 27, 2024 •

edited

Loading

XinShuYang May 30, 2024 •

edited

Loading