fix(docker): credentials issues around superset-cache in forks #26772

mistercrunch · 2024-01-24T04:57:21Z

Fixing the docker builds after merging a PR where I was getting a false positive since it was directly location on the main fork. Now when rebasing and executing docker from forks we get errors messages.

The root cause was this: https://github.com/apache/superset/pull/26766/files#diff-bcd99032795eb96dd42d38ceeeec262f6d4ab3952e3dbfb75905763c116cd0d7R133-R134

Where I used the apache/superset-cache DockerHub instead of the main one, as it was recommended for optimization purposes. In this PR, I'll point it back to apache/superset after confirming the issue, so I can confirm the fix.

Error looked like this on cache-push:

#17 exporting cache to registry
#17 preparing build cache for export
#17 writing layer sha256:6574fa54520867ff3b1634aa74c4749441c37892c2ee032cf292b798e4ee6770
#17 preparing build cache for export 0.1s done
#17 writing layer sha256:6574fa54520867ff3b1634aa74c4749441c37892c2ee032cf292b798e4ee6770 0.1s done
#17 ERROR: error writing layer blob: failed to authorize: failed to fetch oauth token: unexpected status from GET request to https://auth.docker.io/token?scope=repository%3Aapache%2Fsuperset-cache%3Apull%2Cpush&service=registry.docker.io: 401 Unauthorized

#18 [auth] apache/superset-cache:pull,push token for registry-1.docker.io
#18 DONE 0.0s
------
 > exporting cache to registry:
------
ERROR: failed to solve: error writing layer blob: failed to authorize: failed to fetch oauth token: unexpected status from GET request to https://auth.docker.io/token?scope=repository%3Aapache%2Fsuperset-cache%3Apull%2Cpush&service=registry.docker.io: 401 Unauthorized
Error: Process completed with exit code 1.

This is confusing as we're able to push images and layers, but not use the hub as cache. Probably some permission setting of some kind. Though I think the security is a bit reversed, meaning people shouldn't be able to publish an actual image, say to apache/superset:latest from a fork, but the "using the repo as a cache" is less of an issue as no one would pull from random cache. Probably one for the ASF infra folks

craig-rueda · 2024-01-24T17:23:42Z

scripts/docker_build_push.sh

@@ -130,14 +130,16 @@ if [[ -n "${BUILD_TARGET}" ]]; then
  TARGET_ARGUMENT="--target ${BUILD_TARGET}"
 fi

-CACHE_REF="${REPO_NAME}-cache:${TARGET}-${BUILD_ARG}"
+CACHE_REF="${REPO_NAME}:cache-${TARGET}-${BUILD_ARG}"


We actually have a superset-cache registry setup that's intended to be used here: https://hub.docker.com/repository/docker/apache/superset-cache/general

The reason for this is so that we don't clutter up the "real" registry with a pile of cache images

I tried to use this and got the exact same error messages on forks, I agree it's better to use this for caching

mistercrunch · 2024-01-24T20:27:18Z

Realizing this shouldn't even trigger on PRs of forks, somehow the conditional logic stopped working. Fixing it here.

While working on apache#26772, I realized that the has-secret logic was broken for unclear reasons. Now after doing this fix, I looked and realized that there's similar logic across about a dozen other gh actions, and thought it'd be a good thing to refactor/fix this with a reusable action. Now many of these workflows are set to trigger on push-on-master only meaning it's less of an issue since a false positive on has-secret dpesn't matter in that context since there's always a secret. I still thought I should refactor this to something we can trust and build upon. The solution introduces this new simple reusable action. One minor note is in cases where we need multiple secrets, as in say DOCKERHUB_TOKEN and DOCKERHUB_USER, we simply look at one and assume that's clear-enough of an indicator.

While working on #26772, I realized that the has-secret logic was broken for unclear reasons. Now after doing this fix, I looked and realized that there's similar logic across about a dozen other gh actions, and thought it'd be a good thing to refactor/fix this with a reusable action. Now many of these workflows are set to trigger on push-on-master only meaning it's less of an issue since a false positive on has-secret dpesn't matter in that context since there's always a secret. I still thought I should refactor this to something we can trust and build upon. The solution introduces this new simple reusable action. One minor note is in cases where we need multiple secrets, as in say DOCKERHUB_TOKEN and DOCKERHUB_USER, we simply look at one and assume that's clear-enough of an indicator.

…e#26772)

While working on apache/superset#26772, I realized that the has-secret logic was broken for unclear reasons. Now after doing this fix, I looked and realized that there's similar logic across about a dozen other gh actions, and thought it'd be a good thing to refactor/fix this with a reusable action. Now many of these workflows are set to trigger on push-on-master only meaning it's less of an issue since a false positive on has-secret dpesn't matter in that context since there's always a secret. I still thought I should refactor this to something we can trust and build upon. The solution introduces this new simple reusable action. One minor note is in cases where we need multiple secrets, as in say DOCKERHUB_TOKEN and DOCKERHUB_USER, we simply look at one and assume that's clear-enough of an indicator.

…e#26772)

fix(docker): credentials issues around superset-cache in forks

aeb947d

pull-request-size bot added the size/XS label Jan 24, 2024

mistercrunch added 3 commits January 23, 2024 20:59

pointing docker cache location to the apache/superset main hub

b564da2

try an empty ref

879f173

comment out the cache-related args

07398ed

mistercrunch force-pushed the fix_docker branch from 5f22977 to 07398ed Compare January 24, 2024 05:12

mistercrunch marked this pull request as ready for review January 24, 2024 05:16

craig-rueda reviewed Jan 24, 2024

View reviewed changes

eschutho approved these changes Jan 24, 2024

View reviewed changes

improve conditional step

1b6fe1c

mistercrunch requested review from villebro, geido, rusackas, betodealmeida, nytai, john-bodley and kgabryje as code owners January 24, 2024 20:18

pull-request-size bot added size/S and removed size/XS labels Jan 24, 2024

using another fix

ed6f30d

mistercrunch merged commit 73c6abd into apache:master Jan 24, 2024
20 checks passed

mistercrunch deleted the fix_docker branch January 24, 2024 20:27

This was referenced Jan 24, 2024

chore: refactor the github actions has-secrets logic #26790

Closed

chore: refactor the github actions has-secrets logic #26795

Closed

eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024

fix(docker): credentials issues around superset-cache in forks (apach…

f7f5ea2

…e#26772)

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

fix(docker): credentials issues around superset-cache in forks (apach…

5fcec78

…e#26772)

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(docker): credentials issues around superset-cache in forks #26772

fix(docker): credentials issues around superset-cache in forks #26772

mistercrunch commented Jan 24, 2024 •

edited

Loading

craig-rueda Jan 24, 2024

mistercrunch Jan 24, 2024

mistercrunch commented Jan 24, 2024

fix(docker): credentials issues around superset-cache in forks #26772

fix(docker): credentials issues around superset-cache in forks #26772

Conversation

mistercrunch commented Jan 24, 2024 • edited Loading

craig-rueda Jan 24, 2024

Choose a reason for hiding this comment

mistercrunch Jan 24, 2024

Choose a reason for hiding this comment

mistercrunch commented Jan 24, 2024

mistercrunch commented Jan 24, 2024 •

edited

Loading