Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Remove --insecure-registries flag #643

Merged

Conversation

eunomie
Copy link
Member

@eunomie eunomie commented Sep 26, 2019

Signed-off-by: Yves Brissaud yves.brissaud@docker.com

- What I did

The --insecure-registries flag is removed to have a consistent UX with
Docker CLI.

- How I did it

Instead of this flag, the list of insecure registries is retrieved from
the engine. See https://docs.docker.com/registry/insecure/ to configure
your engine to allow communication to an insecure registry.

See the engine API and RegistryConfig.IndexConfigs fields.

- How to verify it

  1. run an insecure registry:

    docker run --rm -p 5000:5000 registry:2

  2. create an app

    docker app init test-insecure

  3. push the app against the insecure registry

    docker app push --tag 127.0.0.1:5000/test-insecure:0.1 test-insecure.dockerapp

    It should fail with an error about HTTP (insecure) response to a HTTPS (secure) client:

    http: server gave HTTP response to HTTPS client

  4. update your daemon configuration to add the local registry as insecure (apply and restart the daemon, restart the stopped registry)

    "insecure-registries": [
      "127.0.0.1:5000"
    ]
    
  5. push again the app

    Successfully pushed bundle to 127.0.0.1:5000/test-insecure:0.1.

- Description for the changelog

Remove --insecure-registries flag and rely on the daemon to retrieve the list of insecure registries.

@eunomie eunomie force-pushed the remove-insecure-registries-flag branch from 56ce605 to 31c686d Compare September 26, 2019 14:26
@eunomie eunomie marked this pull request as ready for review September 26, 2019 14:49

info, err := dockerCli.Client().Info(context.Background())
if err != nil {
logrus.Debugf("could not get docker info: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should return an error instead of logging it and return.


logrus.Debugf("insecure registries: %v", registries)

return
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This return is no-op

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Just small changes requested, but overall it sounds good 👍

@silvin-lubecki
Copy link
Contributor

@eunomie can you rebase?

@eunomie eunomie force-pushed the remove-insecure-registries-flag branch from 31c686d to 1288e00 Compare September 26, 2019 19:25
@codecov
Copy link

codecov bot commented Sep 26, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #643   +/-   ##
=========================================
  Coverage          ?   71.92%           
=========================================
  Files             ?       51           
  Lines             ?     2675           
  Branches          ?        0           
=========================================
  Hits              ?     1924           
  Misses            ?      506           
  Partials          ?      245
Impacted Files Coverage Δ
internal/commands/upgrade.go 58.92% <0%> (ø)
internal/commands/install.go 66.19% <100%> (ø)
internal/commands/inspect.go 80.95% <100%> (ø)
internal/commands/render.go 80.64% <100%> (ø)
internal/commands/push.go 45.22% <60%> (ø)
internal/commands/cnab.go 71.62% <66.66%> (ø)
internal/commands/pull.go 64.28% <66.66%> (ø)
internal/commands/root.go 69.72% <80%> (ø)

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 acbaab0...82e4948. Read the comment docs.

@eunomie eunomie force-pushed the remove-insecure-registries-flag branch from 1288e00 to b50b810 Compare September 26, 2019 19:27
@eunomie
Copy link
Member Author

eunomie commented Sep 26, 2019

Updated:

  • InsecureRegistries now also returns an error instead of logging it

    func InsecureRegistries(dockerCli command.Cli) ([]string, error) {

  • a dedicated end to end test has been added, pushing the same image against an insecure registry with and without daemon configuration to show

    app/e2e/pushpull_test.go

    Lines 190 to 205 in b50b810

    func TestPushInsecureRegistry(t *testing.T) {
    runWithDindSwarmAndRegistry(t, func(info dindSwarmAndRegistryInfo) {
    ref := info.registryAddress + "/test/push-insecure"
    // create a command outside of the dind context so without the insecure registry configured
    cmd2, cleanup2 := dockerCli.createTestCmd()
    defer cleanup2()
    cmd2.Command = dockerCli.Command("app", "push", "--tag", ref, filepath.Join("testdata", "push-pull", "push-pull.dockerapp"))
    icmd.RunCmd(cmd2).Assert(t, icmd.Expected{ExitCode: 1})
    // run the push with the command inside dind context configured to allow access to the insecure registry
    cmd := info.configuredCmd
    cmd.Command = dockerCli.Command("app", "push", "--tag", ref, filepath.Join("testdata", "push-pull", "push-pull.dockerapp"))
    icmd.RunCmd(cmd).Assert(t, icmd.Success)
    })
    }

  • rebased

@eunomie eunomie force-pushed the remove-insecure-registries-flag branch from b50b810 to b59b3d0 Compare September 27, 2019 07:47

// InsecureRegistries reads the registry configuration from the daemon and returns
// a list of all insecure ones.
func InsecureRegistries(dockerCli command.Cli) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can be made private

The `--insecure-registries` flag is removed to have a consistent UX with
Docker CLI.

Instead of this flag, the list of insecure registries is retrieved from
the engine. See https://docs.docker.com/registry/insecure/ to configure
your engine to allow communication to an insecure registry.

See the engine API documentation at
https://docs.docker.com/engine/api/v1.40/#operation/SystemInfo
and `RegistryConfig.IndexConfigs` fields.

Signed-off-by: Yves Brissaud <yves.brissaud@docker.com>
@eunomie eunomie force-pushed the remove-insecure-registries-flag branch from b59b3d0 to 82e4948 Compare September 27, 2019 14:09
@eunomie
Copy link
Member Author

eunomie commented Sep 27, 2019

Rebased after acbaab0 that added a new test with --insecure-registries.
InsecureRegistries is now private and named insecureRegistriesFromEngine (to not collision with variables insecureRegistries.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@silvin-lubecki silvin-lubecki merged commit b785c27 into docker:master Sep 27, 2019
@eunomie eunomie deleted the remove-insecure-registries-flag branch September 27, 2019 15:18
ref := info.registryAddress + "/test/push-insecure"

// create a command outside of the dind context so without the insecure registry configured
cmd2, cleanup2 := dockerCli.createTestCmd()
Copy link
Member

Choose a reason for hiding this comment

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

cmd2, cleanup2: naming could be better 👼 ?

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

Successfully merging this pull request may close these issues.

None yet

5 participants