Add quay support #2783

Ashmita152 · 2021-02-04T02:35:53Z

Signed-off-by: Ashmita Bohara [email protected]

I am still trying to figure out ways to test it properly before merging. Looking for feedback to validate the approach.

Which problem is this PR solving?

Solves #2498

Short description of the changes

All docker images are now built with two tags one for dockerhub and another for quay.io.
All docker images are now uploaded to both dockerhub and quay.io.
All github actions now try to login to quay.io registry.

yurishkuro · 2021-02-04T02:45:39Z

Thanks @Ashmita152 .

To my taste, there's too much repetition. What if we encapsulate the docker vs quay only within the scripts/travis/upload-to-docker.sh script? I don't think anything above it needs to know about where the images are being uploaded. And ideally we would even move the logging in steps into the same script, as there's no reason to do it before the actual upload decision is made. I assume we can pass the login/password via env and it won't be leaked to the logs (we should verify that).

codecov · 2021-02-04T02:47:45Z

Codecov Report

Merging #2783 (4df51e9) into master (169322f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2783      +/-   ##
==========================================
+ Coverage   95.88%   95.92%   +0.03%     
==========================================
  Files         218      218              
  Lines        9606     9620      +14     
==========================================
+ Hits         9211     9228      +17     
+ Misses        327      323       -4     
- Partials       68       69       +1

Impacted Files	Coverage Δ
cmd/collector/app/builder_flags.go	`100.00% <100.00%> (ø)`
cmd/collector/app/collector.go	`78.08% <100.00%> (+3.43%)`	⬆️
cmd/collector/app/server/http.go	`100.00% <100.00%> (+12.00%)`	⬆️
cmd/query/app/static_handler.go	`95.16% <0.00%> (-1.62%)`	⬇️
cmd/query/app/server.go	`95.62% <0.00%> (-1.46%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️
cmd/collector/app/server/zipkin.go	`76.92% <0.00%> (+3.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 169322f...21237b7. Read the comment docs.

Ashmita152 · 2021-02-04T03:01:58Z

Hi @yurishkuro

Thank you for the feedback. I see your point. Let me think about it.

Ashmita152 · 2021-02-06T16:28:23Z

Hi @yurishkuro

I have updated the PR with your suggestion. I hope this time, I took the expected approach.

Two things we need to do before merging:

create jaegertracingbot user in quay.io and allow it to be able to upload images to jaegertracing quay org.
get its token and set QUAY_TOKEN envvar in github.

Regards.

yurishkuro · 2021-02-06T18:42:10Z

did you try pushing the login steps into scripts/travis/upload-to-docker.sh? Before GHA we used to do login manually, and if it is done in that script, we do not need to keep adding the login steps in all the GHA workflows.

yurishkuro · 2021-02-06T20:56:53Z

@Ashmita152 see my test PR #2797. This is the GH run: https://github.com/jaegertracing/jaeger/runs/1846319012?check_suite_focus=true

The env var is never printed in the logs when logging into docker. We can remove all the login steps from the workflows.

DOCKERHUB_TOKEN is defined, attempting to login
+ docker login docker.io --username jaegertracingbot --password-stdin
+ printenv DOCKERHUB_TOKEN
Error response from daemon: Get https://registry-1.docker.io/v2/: unauthorized: incorrect username or password

The error is because I used a fake token, it should work with the real one.

Ashmita152 · 2021-02-07T01:38:07Z

Sure Yuri, sounds good. I will update it.

Ashmita152 · 2021-02-07T13:48:04Z

Hi @yurishkuro

I just realised that the DOCKERHUB_TOKEN is exposed in the above GitHub Action run you shared.

I agree with you that we need to cleanup this login. I am trying to find a way to move dockerhub login and quay login as a separate GitHub Action if possible.

Ashmita152 · 2021-02-07T13:50:42Z

Oh I misunderstood your point. You are right, it will never show up in logs.

Ashmita152 · 2021-02-07T15:14:30Z

Hi Yuri,

I decided to keep the dockerhub+quay login part as a separate github action mainly because of two reasons:

This way those tokens are only visible to this github action script, which I felt more safer since it won't change often.
Our upload-to-docker.sh isn't very neat so I didn't want to add login part there.

Let me know in case you feel otherwise.

yurishkuro · 2021-02-07T17:17:20Z

scripts/travis/build-all-in-one-image.sh

-  if [[ ("$BRANCH" == "master" || $BRANCH =~ ^v[0-9]+\.[0-9]+\.[0-9]+$) && "$DOCKERHUB_LOGIN" == "true" ]]; then
-    echo "upload $1 to Docker Hub"
+  # Only push the docker image to dockerhub/quay.io for master/release branch and when dockerhub or quay.io login is done
+  if [[ ("$BRANCH" == "master" || $BRANCH =~ ^v[0-9]+\.[0-9]+\.[0-9]+$) && ("$DOCKERHUB_LOGIN" == "true" || "$QUAY_LOGIN" == "true") ]]; then


I still think we need to push to upload script the login-related checks. If in the future we need to add one more registry, I only want to have to change the upload script. The var checks are not essential here, and we didn't have them before GHA.

If I remember correctly, those DOCKERHUB_LOGIN var check wasn't there in Travis days because we used to set DOCKERHUB_TOKEN from Travis UI. After moving to GHA, we set them Github org secrets which aren't accessible in case of PRs that's why we added it. My understanding is if we remove those checks and if someone raises a PR from their fork's master branch, it will try to push images to dockerhub and fail.

yurishkuro · 2021-02-07T17:20:39Z

scripts/travis/upload-to-docker.sh

  if [[ -n ${minor:-} ]]; then
    docker tag $IMAGE $REPO:${major}.${minor}
+    docker tag $IMAGE ${QUAY_PREFIX}/$REPO:${major}.${minor}


This doesn't look quite right - this tagging will apply to Docker as well? Maybe we can simply prefix the REPO at the beginning once if quay login is in effect, and run the same set of commands otherwise. Let's separate the logic of the script from the destination registry.

If I get it correctly, we should make the script upload-to-docker.sh to accept a parameter with acceptable values as "dockerhub" or "quay" and should remove all if dockerhub/quay conditions and make the script agnostic of the upstream registry ?

this is what I am suggesting:

In the workflows, we call upload-to-docker.sh and pass secrets as env vars (instead of having a dedicated login action)

upload-to-docker.sh looks like this:

function do_upload() { registry = $1 prefix = $2 token_var = $3 marker=.$registry.login if [ ! -f $marker && -v $token_var ]; then printenv $token_var | docker login ... touch marker fi if [ ! -f $marker ]; then echo "skipping upload to $registry, not logged in: return fi # current content of upload-to-docker.sh before this PR } do_upload "docker.io" "" "DOCKERHUB_TOKEN" do_upload "quay.io" "quay prefix" "QUAY_TOKEN"

This way, the only thing that individual actions need to do about logging in is pass the token as env. The login logic is encapsulated into the upload script, the fact that it does docker and quay is also encapsulated, and the login is efficient by using a cache "marker" file

Sounds great Yuri.

jpkrohling · 2021-02-08T09:51:26Z

A couple of comments:

the username should be externalized, as Quay supports robot accounts tied to the organization, which aren't real accounts. The username would then be something like jaegertracing+github_workflows
you can assume that all repositories are prefixed, like: docker.io, quay.io, ...

Signed-off-by: Ashmita Bohara <[email protected]>

jpkrohling · 2021-02-08T14:26:28Z

I just added the following as "repository secrets"

The DOCKERHUB_TOKEN and JAEGERTRACINGBOT_GITHUB_TOKEN were already set as organization secrets.

scripts/travis/hotrod-integration-test.sh

scripts/travis/upload-to-registry.sh

scripts/travis/upload-all-docker-images.sh

Signed-off-by: Ashmita Bohara <[email protected]>

scripts/travis/upload-to-registry.sh

yurishkuro

Looks great! Almost all redundancy is eliminated, should be much easier to maintain going forward.

scripts/travis/upload-to-registry.sh

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro

It would be good if you try running the upload script manually (you can build one of the images locally and try to push it to your personal account at quay/dockerhub) and include a paste of the execution logs.

scripts/travis/upload-to-registry.sh

Ashmita152 · 2021-02-09T15:39:21Z

Thank you for the feedbacks. Sure Yuri, I will test this PR from my fork properly both from merge to master + release perspective and share with the links.

Signed-off-by: Ashmita Bohara <[email protected]>

scripts/travis/upload-to-registry.sh

jpkrohling · 2021-02-10T14:30:46Z

Nice!

yurishkuro · 2021-02-10T15:12:33Z

I think this is almost ready to merge (just one tiny fix), but it would be nice to see a test run logs. Of course, we could merge and see if the publishing succeeds and fix forward.

Signed-off-by: Ashmita Bohara <[email protected]>

Pull request has been modified.

Ashmita152 · 2021-02-10T15:31:29Z

Right, I am still testing with my fork. I will share the result once I am done testing.

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro · 2021-02-10T15:54:42Z

scripts/travis/upload-to-registry.sh

+
+  try_login ${registry} ${user} ${token} ${marker}
+
+  if [ ! -f ${marker} ] && [ -z ${token} ]; then


There's no need to check for token here, it is in fact harmful since if it is not set you fall through to the else-clause and try to upload

another way to refactor this is to do something like this:

if [ try_login ${registry} ${user} ${token} ] ; then # do upload else # echo no upload fi

This way the marker can be hidden inside try_login

I am little confused about this change. If we use this way, let's say we don't specify token for one of the registries, won't it will still try to upload to that registry since we no longer checking for marker file and deciding based on the return value of try_login function.

In this specific location, if the marker file does not exist, it means we did not log in, so no point in trying to upload. That's why the extra && -v $token condition was wrong.

Otherwise, if you're asking about the if [ try_login ] version, then the function should return true only if it successfully logged in, i.e. it needs to be modified slightly to return false when token is not defined.

I wouldn't worry about it, lets leave it as it is now.

Signed-off-by: Ashmita Bohara <[email protected]>

Ashmita152 · 2021-02-10T17:34:55Z

Hi @yurishkuro

I can confirm that this PR works as expected. I tested by merging my branch in my fork.

I wasn't able to make the quay's robot user work for pushing to quay image and keep getting this error while pushing from robot account.

unauthorized: access to the requested resource is not authorized

Then I used the main quay user account.

I found one bug and fixed it here: 3b2ea2e

Otherwise for quay.io,

docker push ${registry}/${image}

This will fail because image will be missing because we was never labelling the latest tag for quay.

All the GH actions completed successfully: https://github.com/Ashmita152/jaeger/runs/1873169008
All Quay Images: https://quay.io/organization/ashmita1 (quay by default creates private images so you won't be able to see many images)
All Dockerhub Images: https://hub.docker.com/u/ashmita1

Regards.

yurishkuro · 2021-02-10T17:46:44Z

Given the positive test evidence, I think it's time to merge, and see if it works with real credentials / repos.

yurishkuro · 2021-02-10T17:59:05Z

@jpkrohling any thoughts?

printenv QUAY_TOKEN | docker login quay.io --username jaegertracingbot+github_workflows --password-stdin
Error response from daemon: Get https://quay.io/v2/: unauthorized: Could not find robot with specified username

jpkrohling · 2021-02-11T10:31:52Z

Oops, it should be jaegertracing+github_workflows (no bot) I updated the env var QUAY_USERNAME with the correct username.

jpkrohling · 2021-02-11T11:01:59Z

scripts/travis/upload-to-registry.sh

+
+DOCKERHUB_USERNAME=${DOCKERHUB_USERNAME:-"jaegertracingbot"}
+DOCKERHUB_TOKEN=${DOCKERHUB_TOKEN:-}
+QUAY_USERNAME=${QUAY_USERNAME:-"jaegertracingbot+github_workflows"}


Looks like this isn't being take from the env var set as secret in the project. The right username for this is jaegertracing+github_workflows

Ashmita152 requested a review from a team as a code owner February 4, 2021 02:35

Ashmita152 requested a review from pavolloffay February 4, 2021 02:35

mergify bot assigned jpkrohling Feb 4, 2021

mergify bot requested a review from jpkrohling February 4, 2021 02:36

Ashmita152 changed the title ~~Add quay~~ Add quay support Feb 4, 2021

yurishkuro reviewed Feb 7, 2021

View reviewed changes

Push all images to quay.io and docker.io

2517a5c

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro reviewed Feb 8, 2021

View reviewed changes

scripts/travis/hotrod-integration-test.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 8, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 8, 2021

View reviewed changes

scripts/travis/upload-all-docker-images.sh Show resolved Hide resolved

Feedbacks

ac276f2

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro reviewed Feb 9, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 9, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 9, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Show resolved Hide resolved

yurishkuro reviewed Feb 9, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 9, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

Feedbacks

fe21cbb

Signed-off-by: Ashmita Bohara <[email protected]>

Feedbacks

f6b46ae

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro reviewed Feb 9, 2021

View reviewed changes

Feedbacks

0395c5b

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro reviewed Feb 10, 2021

View reviewed changes

scripts/travis/upload-to-registry.sh Outdated Show resolved Hide resolved

yurishkuro previously approved these changes Feb 10, 2021

View reviewed changes

Feedbacks

15ba2db

Signed-off-by: Ashmita Bohara <[email protected]>

Ashmita152 added 2 commits February 10, 2021 23:33

Feedbacks

cf4faef

Signed-off-by: Ashmita Bohara <[email protected]>

Explicitly add latest label otherwise for quay it will not work

3b2ea2e

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro reviewed Feb 10, 2021

View reviewed changes

Feedbacks

21237b7

Signed-off-by: Ashmita Bohara <[email protected]>

yurishkuro approved these changes Feb 10, 2021

View reviewed changes

yurishkuro merged commit 3d9537d into jaegertracing:master Feb 10, 2021

Ashmita152 deleted the add-quay branch February 11, 2021 10:22

jpkrohling reviewed Feb 11, 2021

View reviewed changes

jpkrohling mentioned this pull request Feb 11, 2021

Fix default username when pushing container images to Quay.io #2809

Merged

yurishkuro mentioned this pull request Feb 11, 2021

push official images to quay.io in addition to docker.io #2588

Closed

albertteoh mentioned this pull request Feb 19, 2021

Push official images to quay.io, in addition to Docker Hub #2498

Closed

dependabot bot mentioned this pull request Mar 8, 2021

Bump github.com/jaegertracing/jaeger from 1.21.0 to 1.22.0 httpsgithu/opentelemetry-collector#22

Closed

Karm mentioned this pull request Oct 29, 2024

Start Jaeger to avoid errors related to opentracing/opentelemetry Karm/mandrel-integration-tests#291

Merged


		try_login ${registry} ${user} ${token} ${marker}

		if [ ! -f ${marker} ] && [ -z ${token} ]; then

Add quay support #2783

Add quay support #2783

Conversation

Ashmita152 commented Feb 4, 2021

Which problem is this PR solving?

Short description of the changes

yurishkuro commented Feb 4, 2021

codecov bot commented Feb 4, 2021 • edited Loading

Codecov Report

Ashmita152 commented Feb 4, 2021

Ashmita152 commented Feb 6, 2021

yurishkuro commented Feb 6, 2021

yurishkuro commented Feb 6, 2021 • edited Loading

Ashmita152 commented Feb 7, 2021

Ashmita152 commented Feb 7, 2021

Ashmita152 commented Feb 7, 2021

Ashmita152 commented Feb 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Feb 8, 2021

jpkrohling commented Feb 8, 2021

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Ashmita152 commented Feb 9, 2021

jpkrohling commented Feb 10, 2021

yurishkuro commented Feb 10, 2021

Ashmita152 commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ashmita152 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ashmita152 commented Feb 10, 2021

yurishkuro commented Feb 10, 2021

yurishkuro commented Feb 10, 2021

jpkrohling commented Feb 11, 2021

Choose a reason for hiding this comment

codecov bot commented Feb 4, 2021 •

edited

Loading

yurishkuro commented Feb 6, 2021 •

edited

Loading

Ashmita152 Feb 10, 2021 •

edited

Loading