Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth failure obscured by NullPointerException from checkForDockerSwarmResponse in PullImageResultCallback #2225

Open
FyiurAmron opened this issue Oct 20, 2023 · 0 comments

Comments

@FyiurAmron
Copy link

FyiurAmron commented Oct 20, 2023

With invalid auth, the PullImageResultCallback creates PullResponseItem with error e.g. initializing source xyz: unable to retrieve auth token: invalid username/password: unauthorized: incorrect username or password and corresponding errorDetail. However, when the onNext triggers, it has

        if (this.results == null && this.latestItem == null) {
            this.checkForDockerSwarmResponse(item);
        }

which actually triggers always for failed auth - and that check has

        if (item.getStatus().matches("Pulling\\s.+\\.{3}$")) {

check, which, in case of failed auth, throws NPEx due to item.getStatus() returning null.

Code reference: https://github.com/docker-java/docker-java/blob/main/docker-java-api/src/main/java/com/github/dockerjava/api/command/PullImageResultCallback.java#L48

While the flow is failed anyway in the case of failed auth, figuring "what happened"/debugging it successfully gets extremely hard, and prevents any graceful recovery later on (TBH catching and providing logic for any NPEx in Gradle build flows is really problematic), especially since docker-java is often consumed by other libraries, which can't really handle this in any reasonable way.

Since the current logic for detecting swarm is based on status field anyway, I'd just add a short-circuit null guard to the check, e.g.

        if (item.getStatus() != null && item.getStatus().matches("Pulling\\s.+\\.{3}$")) {

An additional bonus for this fix would be that we'd have LOGGER.debug("{}", item); from onNext to actually execute (now it doesn't due to the NPEx), so it would be even easier to figure what's going on here.

(side note: if a PR with this is needed, I can do it 😸 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant