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

Integration tests for docker image #407

Closed
8 of 9 tasks
tegefaulkes opened this issue Jul 12, 2022 · 28 comments · Fixed by #391
Closed
8 of 9 tasks

Integration tests for docker image #407

tegefaulkes opened this issue Jul 12, 2022 · 28 comments · Fixed by #391
Assignees
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Jul 12, 2022

Specification

This issue is to track the progress of PKG integration tests for the docker image.

A selection of bin tests need to be updated to support using the docker image as the target for testing.

The main tests that need to be updated in order are;

  1. bin/agent/start.test.ts
  2. bin/bootstrap.test.ts
  3. rest of bin/agent
  4. bin/polykey.test.ts

Depending on the decided scope, the rest of the tests that could be updated are listed in MatrixAI/Polykey-CLI#10

Some things specific to docker that need to be checked refereed from https://github.com/MatrixAI/js-polykey/issues/389#issuecomment-1180048178.

  1. Rather than --network host, you'll need an IP address and Port mapping, make sure this works on the CI/CD too
  2. Rather than --pid host you'll need handling of the signals by the PK agent
  3. With --userns host and --user "$(id -u)" this may still be necessary locally, in order to allow the files written to be readable by the test code which is running as part of our local user. But on the CI/CD this both may be the root user. But experiment with this.

Additional context

Tasks

  • 1. update to use testIf to explicitly run the test if testing docker. Right now we implicitly running them by disabling the tests that are not supported for docker.
  • 2. convert the following test...
    • bin/agent/start.test.ts
    • bin/bootstrap.test.ts
    • rest of bin/agent
    • `bin/polykey.test.ts
    • rest of the bin tests
  • 3. update .env.example with any changes to the required env vars.
  • 4. ensure CI/CD job is working.
@tegefaulkes
Copy link
Contributor Author

So we're running into the quirks of using DIND to run our container. Currently the problem is how mounting the host FS works. Since we're running the dockerd daemon in the DIND service container we don't actually have access to the FS that the docker is running on.

This means I can't actually specify the temp file directory for the docker to run since we can't actually create it for the container. I'm not sure there is a way for us to share the file system between the main container and the DIND service.

That leaves us with the following options.

  1. Skip or modify tests that require accessing files created by the docker container.
  2. Modify the gitlab-runner to run it's own dockerd daemon instead of using a DIND service.

The simpler solution for now is just only do tests that don't require directly accessing the contents of the agent's directory. This is a pretty big restriction however since the docker image is just running the raw node code we don't need to be as exhaustive in the testing here. We only need a limited amount of tests to act as a sanity check.

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Jul 13, 2022

Nice comment about the problem Here https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41227#note_52029664

a possible solution mentioned here is using

variables:
  SHARED_PATH: /builds/shared/$CI_PROJECT_PATH

I'll look into this.

Update: Yep, looks like this works.

@CMCDragonkai
Copy link
Member

The next comment is also insightful: https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41227#note_52092575

@CMCDragonkai
Copy link
Member

And also https://gitlab.com/gitlab-org/gitlab-foss/-/issues/41227#note_128388116... seems like there's a few ways.

@tegefaulkes
Copy link
Contributor Author

Sweet, it's working. In the recent test all but 1 of the tests/bin/agent/start.test.ts succeeded.

@tegefaulkes
Copy link
Contributor Author

Both the start.test.ts and bootstrap.test.ts is passing now. From here on I just need to convert tests to use docker and the do some cleaning up.

tegefaulkes added a commit that referenced this issue Jul 14, 2022
tegefaulkes added a commit that referenced this issue Jul 14, 2022
tegefaulkes added a commit that referenced this issue Jul 14, 2022
tegefaulkes added a commit that referenced this issue Jul 14, 2022
tegefaulkes added a commit that referenced this issue Jul 14, 2022
tegefaulkes added a commit that referenced this issue Jul 19, 2022
@tegefaulkes
Copy link
Contributor Author

Converted all of the tests now. Just need to test them.

tegefaulkes added a commit that referenced this issue Jul 20, 2022
tegefaulkes added a commit that referenced this issue Jul 20, 2022
tegefaulkes added a commit that referenced this issue Jul 20, 2022
@tegefaulkes
Copy link
Contributor Author

I'm running into a problem that's making no sense. A lot of tests are just timing out, tests/bin/identities/allowDisallowPermissions.test.ts:102 for example. The command spawns however it never exits and never outputs anything on stdout and stderr.

I'm pretty sure that the start tests are still working. I need to do some sanity checks.

@tegefaulkes
Copy link
Contributor Author

The sanity check has revealed that the command running inside the container can't connect to an agent running on host or another container. I'm exploring solutions.

@CMCDragonkai
Copy link
Member

Is it a matter of mapping the right ports? Since we are using dind, both containers the client and the agent would be running in the dind container, so then ports have mapped to be non-conflicting.

I also remember docker supports a sort of "link" or bridge between docker containers rather than port mapping.

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Jul 21, 2022

They should share the same default network if I remove --network host but after doing that it still doesn't connect. I think I have to specify a IP address if I'm not using the host network. I can explore what the network looks like using docker network inspect.

The other problem is that some of the tests need to connect to a agent with mocking running on the test host.

@CMCDragonkai
Copy link
Member

I said that in the CICD, that --network host may not work. Because you're not actually running the containers with respect to the host network. You don't have access to it at all, your job is itself a container. So port mapping may be necessary, or using a docker link/bridge.

@CMCDragonkai
Copy link
Member

Yea so we need to get rid of any mocking. Integration tests will need to test with minimal mocking...

It looks like linking is a legacy feature now.

It is recommended to use "user defined networking":

Will these work in the CI/CD? Not sure. Have a try.

@CMCDragonkai
Copy link
Member

@tegefaulkes
Copy link
Contributor Author

Inspecting the network with and without --network host set shows no containers in the network?

  console.log
    [
        {
            "Name": "bridge",
            "Id": "64b8db696e685496de21762554890d54da715d9a45ced403f75a55d82032fe9c",
            "Created": "2022-07-21T06:44:27.366845638Z",
            "Scope": "local",
            "Driver": "bridge",
            "EnableIPv6": false,
            "IPAM": {
                "Driver": "default",
                "Options": null,
                "Config": [
                    {
                        "Subnet": "172.18.0.0/16"
                    }
                ]
            },
            "Internal": false,
            "Attachable": false,
            "Ingress": false,
            "ConfigFrom": {
                "Network": ""
            },
            "ConfigOnly": false,
            "Containers": {},
            "Options": {
                "com.docker.network.bridge.default_bridge": "true",
                "com.docker.network.bridge.enable_icc": "true",
                "com.docker.network.bridge.enable_ip_masquerade": "true",
                "com.docker.network.bridge.host_binding_ipv4": "0.0.0.0",
                "com.docker.network.bridge.name": "docker0",
                "com.docker.network.driver.mtu": "1500"
            },
            "Labels": {}
        }
    ]
      at Object.<anonymous> (tests/bin/sanity.test.ts:83:13)
  console.log
    [
        {
            "Name": "host",
            "Id": "d27ec6ed1d4ad75e08bd37a5639a42ae7eb0003fd2d440fa7f1a14247a53465c",
            "Created": "2022-07-21T06:44:27.353684646Z",
            "Scope": "local",
            "Driver": "host",
            "EnableIPv6": false,
            "IPAM": {
                "Driver": "default",
                "Options": null,
                "Config": []
            },
            "Internal": false,
            "Attachable": false,
            "Ingress": false,
            "ConfigFrom": {
                "Network": ""
            },
            "ConfigOnly": false,
            "Containers": {},
            "Options": {},
            "Labels": {}
        }
    ]
      at Object.<anonymous> (tests/bin/sanity.test.ts:84:13)

tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
This will remove the container when it ends.

#407
tegefaulkes added a commit that referenced this issue Jul 26, 2022
Disabling tests that don't work with docker for now.

#407
tegefaulkes added a commit that referenced this issue Jul 26, 2022
disabled some more tests with prompts mocking.

Removed `sanity.test.ts`

#407
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 26, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
- converting lock.test.ts
- cleaning up `testIf` commands and envs
- converted lockall.test.ts
- converted status.test.ts
- converted stop.test.ts
- converted unlock.test.ts

Related #407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
Also enabled `FF_NETWORK_PER_BUILD` flag for CI.

Related #407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
This will remove the container when it ends.

#407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
Disabling tests that don't work with docker for now.

#407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
tegefaulkes added a commit that referenced this issue Jul 27, 2022
Also enabled `FF_NETWORK_PER_BUILD` flag for CI.

Related #407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
This will remove the container when it ends.

#407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
Disabling tests that don't work with docker for now.

#407
tegefaulkes added a commit that referenced this issue Jul 27, 2022
@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity
Development

Successfully merging a pull request may close this issue.

2 participants