feat: add network flag to nerdctl build #2383

vsiravar · 2023-07-20T13:41:16Z

What does this PR do?

Add's support for --network flag to nerdctl build. Fixes: #2233

Details

Supported values host|none|default. Compatible with buildctl build
Insecure entitlements in daemon insecure-entitlements = [ "network.host", "security.insecure" ] required for --network=host. Additionally pass --allow=network.host and --allow=security.insecure to buildctlArgs

Testing

Added tests in builder_build_test.go for --network=default|""|none. Tested --network=host manually since it requires modifying buildkitd.toml file.

AkihiroSuda · 2023-07-20T14:41:40Z

cmd/nerdctl/builder_build_test.go

@@ -449,3 +449,38 @@ CMD ["cat", "/source-date-epoch"]
 	base.Cmd("build", "-t", imageName, "--build-arg", "SOURCE_DATE_EPOCH="+sourceDateEpochArgStr, buildCtx).AssertOK()
 	base.Cmd("run", "--rm", imageName).AssertOutExactly(sourceDateEpochArgStr + "\n")
 }
+
+func TestBuildNetworkFlat(t *testing.T) {


What is "flat" ?

AkihiroSuda · 2023-07-20T14:42:12Z

cmd/nerdctl/builder_build.go

@@ -56,6 +56,7 @@ If Dockerfile is not present and -f is not specified, it will look for Container
 	buildCommand.Flags().StringArray("cache-from", nil, "External cache sources (eg. user/app:cache, type=local,src=path/to/dir)")
 	buildCommand.Flags().StringArray("cache-to", nil, "Cache export destinations (eg. user/app:cache, type=local,dest=path/to/dir)")
 	buildCommand.Flags().Bool("rm", true, "Remove intermediate containers after a successful build")
+	buildCommand.Flags().String("network", "default", "Set type of network for build (default, none, host)")


Can we have shell completion too?

vsiravar · 2023-07-20T15:15:20Z

@AkihiroSuda Thanks for the review. I addressed all the comments.

AkihiroSuda · 2023-07-20T16:08:37Z

cmd/nerdctl/completion.go

@@ -160,3 +160,12 @@ func shellCompletePlatforms(cmd *cobra.Command, args []string, toComplete string
 	}
 	return candidates, cobra.ShellCompDirectiveNoFileComp
 }
+
+func shellCompleteBuildNetworks(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {


This can be just in builder_build.go

Thanks, I made the suggested change and added a test.

AkihiroSuda · 2023-07-20T16:09:08Z

cmd/nerdctl/builder_build_test.go

@@ -449,3 +449,38 @@ CMD ["cat", "/source-date-epoch"]
 	base.Cmd("build", "-t", imageName, "--build-arg", "SOURCE_DATE_EPOCH="+sourceDateEpochArgStr, buildCtx).AssertOK()
 	base.Cmd("run", "--rm", imageName).AssertOutExactly(sourceDateEpochArgStr + "\n")
 }
+
+func TestBuildNetworkFlag(t *testing.T) {


Suggested change

func TestBuildNetworkFlag(t *testing.T) {

func TestBuildNetwork(t *testing.T) {

AkihiroSuda

Thanks

AkihiroSuda · 2023-07-20T18:10:01Z

test-integration-docker-compatibility is failing

=== NAME  TestBuildNetwork
    builder_build_test.go:483: assertion failed: res.ExitCode is not exitCode: Sending build context to Docker daemon  2.048kB

        Step 1/3 : FROM ghcr.io/stargz-containers/alpine:3.13-org
         ---> 49f3[56](https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383#step:6:57)fa4513
        Step 2/3 : RUN apk add --no-cache curl
         ---> Using cache
         ---> 2d6f56ba72af
        Step 3/3 : RUN curl -I http://google.com/
         ---> Using cache
         ---> fdcd322[63](https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383#step:6:64)92a
        Successfully built fdcd3226392a
        Successfully tagged nerdctl-testbuildnetwork:latest
        
=== NAME  TestBuildNetwork/test_with_no_network
    testing.go:1471: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
--- FAIL: TestBuildNetwork (4.10s)
    --- PASS: TestBuildNetwork/test_with_empty_network (3.87s)
    --- PASS: TestBuildNetwork/test_with_default_network (0.05s)
    --- FAIL: TestBuildNetwork/test_with_no_network (0.06s)

https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383

vsiravar · 2023-07-20T21:34:40Z

test-integration-docker-compatibility is failing

=== NAME  TestBuildNetwork
    builder_build_test.go:483: assertion failed: res.ExitCode is not exitCode: Sending build context to Docker daemon  2.048kB

        Step 1/3 : FROM ghcr.io/stargz-containers/alpine:3.13-org
         ---> 49f3[56](https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383#step:6:57)fa4513
        Step 2/3 : RUN apk add --no-cache curl
         ---> Using cache
         ---> 2d6f56ba72af
        Step 3/3 : RUN curl -I http://google.com/
         ---> Using cache
         ---> fdcd322[63](https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383#step:6:64)92a
        Successfully built fdcd3226392a
        Successfully tagged nerdctl-testbuildnetwork:latest
        
=== NAME  TestBuildNetwork/test_with_no_network
    testing.go:1471: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
--- FAIL: TestBuildNetwork (4.10s)
    --- PASS: TestBuildNetwork/test_with_empty_network (3.87s)
    --- PASS: TestBuildNetwork/test_with_default_network (0.05s)
    --- FAIL: TestBuildNetwork/test_with_no_network (0.06s)

https://github.com/containerd/nerdctl/actions/runs/5613705111/job/15211524315?pr=2383

This is a bit strange to what I observe when I test locally.

$ cat Dockerfile 
FROM alpine
RUN apk add --no-cache curl
RUN curl -I http://google.com

$ docker build -t test-tag --network=none . 
[+] Building 12.4s (5/6)                                                                                                                                                                                 
 => [internal] load build definition from Dockerfile                                                                                                                                                0.0s
 => => transferring dockerfile: 31B                                                                                                                                                                 0.0s
 => [internal] load .dockerignore                                                                                                                                                                   0.0s
 => => transferring context: 2B                                                                                                                                                                     0.0s
 => [internal] load metadata for docker.io/library/alpine:latest                                                                                                                                    2.2s
 => CACHED [1/3] FROM docker.io/library/alpine@sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1                                                                              0.0s
 => ERROR [2/3] RUN apk add --no-cache curl                                                                                                                                                        10.1s
------                                                                                                                                                                                                   
 > [2/3] RUN apk add --no-cache curl:                                                                                                                                                                    
#0 0.045 fetch https://dl-cdn.alpinelinux.org/alpine/v3.18/main/aarch64/APKINDEX.tar.gz                                                                                                                  
#0 5.053 fetch https://dl-cdn.alpinelinux.org/alpine/v3.18/community/aarch64/APKINDEX.tar.gz                                                                                                             
#0 5.055 WARNING: fetching https://dl-cdn.alpinelinux.org/alpine/v3.18/main: temporary error (try again later)
#0 10.07 WARNING: fetching https://dl-cdn.alpinelinux.org/alpine/v3.18/community: temporary error (try again later)
#0 10.07 ERROR: unable to select packages:
#0 10.07   curl (no such package):
#0 10.07     required by: world[curl]
------
ERROR: failed to solve: executor failed running [/bin/sh -c apk add --no-cache curl]: exit code: 1
$ echo $?
1

Also tested this test on lima docker instance with docker.

--- PASS: TestBuildNetwork (14.56s)
    --- PASS: TestBuildNetwork/test_with_empty_network (3.71s)
    --- PASS: TestBuildNetwork/test_with_default_network (0.30s)
    --- PASS: TestBuildNetwork/test_with_no_network (10.40s)

AkihiroSuda · 2023-07-21T03:38:32Z

cmd/nerdctl/builder_build_test.go

+	for _, tc := range validCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			base.Cmd("build", buildCtx, "--network", tc.network).AssertExitCode(tc.exitCode)


Please try adding --no-cache to fix the CI failure with docker.
Or, test --network=none before running other tests

And it is weird that the test is passing for nerdctl without --no-cache. 🤔

Please try adding --no-cache to fix the CI failure with docker.
Or, test --network=none before running other tests

This did the trick although not sure why it works with nerdctl without this option. Also works with docker on lima instance on my local. I'll add a comment about it.

AkihiroSuda · 2023-07-21T07:51:07Z

=== RUN   TestContainerListWithFilter
    container_list_linux_test.go:94: container nerdctl-testcontainerlistwithfilterlistWithFilterA is now running
    container_list_linux_test.go:94: container nerdctl-testcontainerlistwithfilterlistWithFilterB is now running
    container_list_linux_test.go:505: assertion failed: error is not nil: unexpected container interesting_bell found: CONTAINER ID   IMAGE                                       COMMAND                  CREATED         STATUS                              PORTS     NAMES
        e81c6c7dddac   ghcr.io/stargz-containers/alpine:3.13-org   "top --restart=no"       1 second ago    Exited (1) Less than a second ago             nerdctl-testcontainerlistwithfilterlistWithFilterC
        31505ac91a40   49f356fa4513                                "/bin/sh -c 'apk add…"   3 minutes ago   Exited (1) 3 minutes ago                      interesting_bell
        
--- FAIL: TestContainerListWithFilter (4.23s)

https://github.com/containerd/nerdctl/actions/runs/5618569480/job/15225123013?pr=2383

vsiravar · 2023-07-22T08:54:24Z

=== RUN   TestContainerListWithFilter
    container_list_linux_test.go:94: container nerdctl-testcontainerlistwithfilterlistWithFilterA is now running
    container_list_linux_test.go:94: container nerdctl-testcontainerlistwithfilterlistWithFilterB is now running
    container_list_linux_test.go:505: assertion failed: error is not nil: unexpected container interesting_bell found: CONTAINER ID   IMAGE                                       COMMAND                  CREATED         STATUS                              PORTS     NAMES
        e81c6c7dddac   ghcr.io/stargz-containers/alpine:3.13-org   "top --restart=no"       1 second ago    Exited (1) Less than a second ago             nerdctl-testcontainerlistwithfilterlistWithFilterC
        31505ac91a40   49f356fa4513                                "/bin/sh -c 'apk add…"   3 minutes ago   Exited (1) 3 minutes ago                      interesting_bell
        
--- FAIL: TestContainerListWithFilter (4.23s)

https://github.com/containerd/nerdctl/actions/runs/5618569480/job/15225123013?pr=2383

I could not find the race root cause for this. However a workaround could be container prune in container_list test. It should have no impact to the functionality being tested in those tests or to the build network test. PTAL.

AkihiroSuda · 2023-07-22T17:01:53Z

This may help you

CI: test-integration-docker-compatibility: enable BuildKit #2387

AkihiroSuda · 2023-07-22T17:04:13Z

I could not find the race root cause for this.

Not a race.
Docker (with the legacy builder) does not clean up failed build containers.
So you should have just run docker container prune.

vsiravar · 2023-07-23T04:03:01Z

This may help you

CI: test-integration-docker-compatibility: enable BuildKit #2387

Thanks for the insight. I will rebase off of main once this PR is merged rather than using docker container prune in the container list test(although it should not have any side effects to the container list tests).

Update: After the rebase with main the tests run fine.

Signed-off-by: Vishwas Siravara <[email protected]>

AkihiroSuda

Thanks

vsiravar changed the title ~~feat: add --network flag to nerdctl build~~ feat: add network flag to nerdctl build Jul 20, 2023

AkihiroSuda added this to the v1.5.0 milestone Jul 20, 2023

AkihiroSuda reviewed Jul 20, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2233 branch from ee4db5e to 3d61a92 Compare July 20, 2023 15:12

AkihiroSuda reviewed Jul 20, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2233 branch from 3d61a92 to 1a4ecc4 Compare July 20, 2023 16:46

AkihiroSuda approved these changes Jul 20, 2023

View reviewed changes

austinvazquez mentioned this pull request Jul 20, 2023

ci: simplify go linter and rename it to differentiate with other linters aws/shim-loggers-for-containerd#57

Merged

vsiravar force-pushed the vsiravar/fix-2233 branch from 1a4ecc4 to 63cd139 Compare July 20, 2023 21:19

AkihiroSuda reviewed Jul 21, 2023

View reviewed changes

vsiravar force-pushed the vsiravar/fix-2233 branch 2 times, most recently from 2d79c7d to 0d2413f Compare July 21, 2023 04:19

vsiravar force-pushed the vsiravar/fix-2233 branch 2 times, most recently from a30e72a to 4206529 Compare July 22, 2023 05:48

AkihiroSuda marked this pull request as draft July 22, 2023 07:32

vsiravar force-pushed the vsiravar/fix-2233 branch 2 times, most recently from 9ca560f to 946036e Compare July 22, 2023 08:36

vsiravar force-pushed the vsiravar/fix-2233 branch 2 times, most recently from d461fcb to ca4d601 Compare July 24, 2023 02:47

Add network flag to nerdctl build

ccce195

Signed-off-by: Vishwas Siravara <[email protected]>

vsiravar force-pushed the vsiravar/fix-2233 branch from ca4d601 to ccce195 Compare July 24, 2023 03:16

vsiravar marked this pull request as ready for review July 24, 2023 03:47

AkihiroSuda approved these changes Jul 29, 2023

View reviewed changes

AkihiroSuda merged commit 2f96dd9 into containerd:main Jul 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add network flag to nerdctl build #2383

feat: add network flag to nerdctl build #2383

vsiravar commented Jul 20, 2023

AkihiroSuda Jul 20, 2023

AkihiroSuda Jul 20, 2023

vsiravar commented Jul 20, 2023

AkihiroSuda Jul 20, 2023

vsiravar Jul 20, 2023

AkihiroSuda Jul 20, 2023

AkihiroSuda left a comment

AkihiroSuda commented Jul 20, 2023

vsiravar commented Jul 20, 2023 •

edited

Loading

AkihiroSuda Jul 21, 2023 •

edited

Loading

AkihiroSuda Jul 21, 2023

vsiravar Jul 21, 2023

AkihiroSuda commented Jul 21, 2023

vsiravar commented Jul 22, 2023

AkihiroSuda commented Jul 22, 2023

AkihiroSuda commented Jul 22, 2023

vsiravar commented Jul 23, 2023 •

edited

Loading

AkihiroSuda left a comment

	func TestBuildNetworkFlag(t *testing.T) {
	func TestBuildNetwork(t *testing.T) {

feat: add network flag to nerdctl build #2383

feat: add network flag to nerdctl build #2383

Conversation

vsiravar commented Jul 20, 2023

What does this PR do?

Details

Testing

AkihiroSuda Jul 20, 2023

Choose a reason for hiding this comment

AkihiroSuda Jul 20, 2023

Choose a reason for hiding this comment

vsiravar commented Jul 20, 2023

AkihiroSuda Jul 20, 2023

Choose a reason for hiding this comment

vsiravar Jul 20, 2023

Choose a reason for hiding this comment

AkihiroSuda Jul 20, 2023

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jul 20, 2023

vsiravar commented Jul 20, 2023 • edited Loading

AkihiroSuda Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda Jul 21, 2023

Choose a reason for hiding this comment

vsiravar Jul 21, 2023

Choose a reason for hiding this comment

AkihiroSuda commented Jul 21, 2023

vsiravar commented Jul 22, 2023

AkihiroSuda commented Jul 22, 2023

AkihiroSuda commented Jul 22, 2023

vsiravar commented Jul 23, 2023 • edited Loading

AkihiroSuda left a comment

Choose a reason for hiding this comment

vsiravar commented Jul 20, 2023 •

edited

Loading

AkihiroSuda Jul 21, 2023 •

edited

Loading

vsiravar commented Jul 23, 2023 •

edited

Loading