Skip to content

Remove more registry v1 code#39371

Closed
tiborvass wants to merge 2 commits intomoby:masterfrom
tiborvass:remove-more-v1-code
Closed

Remove more registry v1 code#39371
tiborvass wants to merge 2 commits intomoby:masterfrom
tiborvass:remove-more-v1-code

Conversation

@tiborvass
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass commented Jun 18, 2019

Follow up to #39365

@tiborvass tiborvass force-pushed the remove-more-v1-code branch 2 times, most recently from f25fa78 to 1bde9e5 Compare June 19, 2019 00:51
@tiborvass tiborvass marked this pull request as ready for review June 19, 2019 00:51
@thaJeztah thaJeztah added area/distribution Image Distribution status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Jun 27, 2019
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 27, 2019

Tests are failing @tiborvass

@dmcgowan dmcgowan mentioned this pull request Jul 1, 2019
Closed
6 tasks
@tiborvass tiborvass added rebuild/janky and removed status/failing-ci Indicates that the PR in its current state fails the test suite rebuild/janky labels Jul 2, 2019
@tiborvass tiborvass added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 2, 2019
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ad71802). Click here to learn what that means.
The diff coverage is 7.4%.

@@            Coverage Diff            @@
##             master   #39371   +/-   ##
=========================================
  Coverage          ?   37.41%           
=========================================
  Files             ?      609           
  Lines             ?    45108           
  Branches          ?        0           
=========================================
  Hits              ?    16875           
  Misses            ?    25944           
  Partials          ?     2289

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jan 21, 2020

needs rebase

@thaJeztah thaJeztah force-pushed the remove-more-v1-code branch from 3b9dc70 to bead3a3 Compare July 7, 2020 07:53
@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 9, 2020
@thaJeztah thaJeztah force-pushed the remove-more-v1-code branch from bead3a3 to bc5f563 Compare July 9, 2020 10:21
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 9, 2020

Rebased to first a fresh run of CI

@thaJeztah thaJeztah added this to the 20.03.0 milestone Jul 9, 2020
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jul 9, 2020

Various tests are still failing (same error at a glance);

=== RUN   TestDockerRegistryAuthHtpasswdSuite/TestBuildFromAuthenticatedRegistry
    --- FAIL: TestDockerRegistryAuthHtpasswdSuite/TestBuildFromAuthenticatedRegistry (0.16s)
        cli.go:29: assertion failed: 
            Command:  /usr/local/cli/docker login -u testuser -p testpassword 127.0.0.1:5000
            ExitCode: 1
            Error:    exit status 1
            Stdout:   
            Stderr:   Error response from daemon: Get https://127.0.0.1:5000/v2/: http: server gave HTTP response to HTTPS client
            
            
            Failures:
            ExitCode was 1 expected 0
            Expected no error
        check_test.go:252: [d1b0e177d160c] daemon is not started

Looks like it doesn't use / ignores the default 127.0.0.1 marked as "insecure registry"?

@thaJeztah thaJeztah mentioned this pull request Oct 28, 2020
Merged
Tibor Vass added 2 commits October 28, 2020 23:07
@thaJeztah thaJeztah force-pushed the remove-more-v1-code branch from bc5f563 to fadaf90 Compare October 28, 2020 22:09
thaJeztah
thaJeztah reviewed Oct 28, 2020
View reviewed changes
Comment thread registry/service.go
}

return "", "", err
return loginV2(authConfig, endpoint, userAgent)
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is where the problem is for the failing test; previously this was returning a fallbackError if;

  • the registry didn't support v2 (in which case it would fallback to v1
  • connecting with the registry using https failed, and the registry we're connecting to is marked as insecure registry

In the latter case, it should continue trying the next (non-https) endpoint, and currently we only try the first endpoint in the list.

Not sure if we should continue trying on every type of error though (authentication failed errors should likely not be ignored)

thaJeztah
thaJeztah reviewed Oct 28, 2020
View reviewed changes
Comment thread registry/auth.go
Comment on lines -220 to -238
return nil, false, err
return nil, err
}
resp, err := pingClient.Do(req)
if err != nil {
return nil, false, err
return nil, err
}
defer resp.Body.Close()

versions := auth.APIVersions(resp, DefaultRegistryVersionHeader)
for _, pingVersion := range versions {
if pingVersion == v2Version {
// The version header indicates we're definitely
// talking to a v2 registry. So don't allow future
// fallbacks to the v1 protocol.

foundV2 = true
break
}
}
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we should keep some of the v1 detection logic so that we can return a more informative error ("registry is v1, which is no longer supported, see https://docs.docker.com/go/registry-v1")

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Oct 28, 2020

Rebased this PR, as #41599 was merged

@thaJeztah thaJeztah mentioned this pull request Dec 7, 2020
Merged
@thaJeztah thaJeztah modified the milestones: 20.10.0, 20.10.1, 20.10.2 Dec 9, 2020
@thaJeztah thaJeztah modified the milestones: 20.10.2, 20.10.3 Jan 5, 2021
@thaJeztah thaJeztah modified the milestones: 20.10.3, 20.10.4, 21.xx Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

Assignees

@johnstep johnstep

Labels

area/distribution Image Distribution status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tiborvass @thaJeztah @AkihiroSuda @cpuguy83 @johnstep @GordonTheTurtle