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.

Sign up for GitHub

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

Jump to bottom

Add the InterfaceName option to Pinger #123

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Add the InterfaceName option to Pinger #123

merged 2 commits into from
Oct 31, 2024

Conversation

MatthieuCoder
Copy link
Contributor

@MatthieuCoder MatthieuCoder commented Oct 29, 2024 •
edited
Loading

This is a rebase of #32 to match the current HEAD on main.

This allows using pro-bing to use VRFs interfaces and IPv6 link-local addresses.

SuperQ
SuperQ reviewed Oct 29, 2024
View reviewed changes
ping.go Outdated Show resolved Hide resolved
@SuperQ
Copy link
Contributor

SuperQ commented Oct 29, 2024

It seems like the test fails on permissions.

@MatthieuCoder
Copy link
Contributor Author

Indeed, as the tests work locally, I wonder if it's a permission issue on CircleCI's end.
Something to do with sending to the lo interface specifically ?
Does it run on containers that could interfere with this ?

@MatthieuCoder
Copy link
Contributor Author

MatthieuCoder commented Oct 29, 2024 •
edited
Loading

As @ilolicon pointed out, the tests run just fine on a local hosts & using the circleci local cli:
Might be interesting: https://discuss.circleci.com/t/tests-cant-connect-to-localhost-in-docker/5767/6

matthieu@matthieu-archlinux ~/pro-bing (main)> circleci local execute build
Fetching latest build environment...
Docker image digest: sha256:008ba7f4223f1e26c11df9575283491b620074fa96da6961e0dcde47fb757014
====>> Spin up environment
Build-agent version  ()
System information:
 Server Version: 27.3.1
 Storage Driver: overlay2
  Backing Filesystem: btrfs
 Cgroup Driver: systemd
 Cgroup Version: 2
 Kernel Version: 6.11.5-arch1-1
 Operating System: Arch Linux
 OSType: linux
 Architecture: x86_64

Starting container cimg/go:1.20
Warning: No authentication provided, using CircleCI credentials for pulls from Docker Hub.
  image is cached as cimg/go:1.20, but refreshing...
1.20: Pulling from cimg/go
Digest: sha256:bd1abc34d5deb0c8f55e9f9c8071210e3a5ba9ce874f48a61a390175c2be0da6
Status: Image is up to date for cimg/go:1.20
cimg/go:1.20:
  using image cimg/go@sha256:bd1abc34d5deb0c8f55e9f9c8071210e3a5ba9ce874f48a61a390175c2be0da6
  pull stats: Image was already available so the image was not pulled
  time to create container: 117ms
Creating Docker containers in parallel 
Time to upload agent and config: 162.595656ms
Time to start containers: 457.684773ms
====>> Preparing environment variables
Using build environment variables:
  BASH_ENV=/tmp/.bash_env-localbuild-1730205231
  CI=true
  CIRCLECI=true
  CIRCLE_BRANCH=
  CIRCLE_BUILD_NUM=
  CIRCLE_JOB=build
  CIRCLE_NODE_INDEX=0
  CIRCLE_NODE_TOTAL=1
  CIRCLE_REPOSITORY_URL=
  CIRCLE_SHA1=
  CIRCLE_SHELL_ENV=/tmp/.bash_env-localbuild-1730205231
  CIRCLE_WORKING_DIRECTORY=~/project


The redacted variables listed above will be masked in run step output.====>> Checkout code
Making checkout directory "/home/circleci/project"
Copying files from "/tmp/_circleci_local_build_repo" to "/home/circleci/project"
====>> go mod download
  #!/bin/bash -eo pipefail
go mod download
====>> make
  #!/bin/bash -eo pipefail
make
>> checking code style
>> vetting code
GO111MODULE= go vet  ./...
>> building ping
GO111MODULE= go build  ./cmd/ping
>> running all tests
GO111MODULE= go test -race -cover  ./...
?       github.com/prometheus-community/pro-bing/cmd/ping       [no test files]
ok      github.com/prometheus-community/pro-bing        7.105s  coverage: 67.9% of statements
Success!

@MatthieuCoder
Copy link
Contributor Author

Seems like using another interface did not fix the issue regarding permissions,
Specifying an interface might be unsupported in CircleCI ?

@SuperQ
Copy link
Contributor

SuperQ commented Oct 29, 2024

We can try switching from Docker to Machine mode for CI.

@MatthieuCoder
Copy link
Contributor Author

I would like that.

@MatthieuCoder MatthieuCoder changed the title Add the Interface option to Pinger Add the InterfaceName option to Pinger Oct 29, 2024
@MatthieuCoder
Copy link
Contributor Author

Do I need to pull the changes on main for them to apply here ?

@MatthieuCoder
Copy link
Contributor Author

Permission denied still, I'll look into it tomorrow

@MatthieuCoder
Copy link
Contributor Author

MatthieuCoder commented Oct 29, 2024 •
edited
Loading

As I thought, Using the machine executor does make the tests pass,
Subsequently the change from docker to machine should be in another PR, since @SuperQ's PR (#124) does not change the execution to the machine type

@SuperQ
Copy link
Contributor

SuperQ commented Oct 29, 2024

Yea, the only problem with the machine image is it doesn't directly control the Go version.

@MatthieuCoder
Copy link
Contributor Author

Yea, the only problem with the machine image is it doesn't directly control the Go version.

Indeed, I read online that this could be solved by running docker in the machine executor, this might require more configuration.

@MatthieuCoder
Copy link
Contributor Author

hold on I have a last thing to change

@MatthieuCoder
Implement a new InterfaceDevice property in the Pinger struct tha…
2c9120c 
…t specifies the device that needs to be used to send and receive ICMP echo requests and response.

This allows usage of `pro-bing` with linux VRFs, and IPv6 link-local addresses.

Squashed commit of the following:

commit 7385672
Author: Matthieu Pignolet <matthieu@matthieu-dev.xyz>
Date:   Tue Oct 29 16:11:33 2024 +0400

    Renaming the `Interface` Pinger property to `InterfaceDevice` to avoid conflict with the `interface` keyword.

    Signed-off-by: Matthieu Pignolet <matthieu@matthieu-dev.xyz>

commit 978c60e
Author: Matthieu Pignolet <matthieu@matthieu-dev.xyz>
Date:   Tue Oct 29 15:59:36 2024 +0400

Add the `Interface` option to `Pinger`

This allows using `pro-bing` to use VRFs interfaces and IPv6 link-local addresses.
Originally from @ilolicon in prometheus-community#32.

commit 16f9286
Author: Matthieu Pignolet <matthieu@matthieu-dev.xyz>
Date:   Tue Oct 29 15:46:13 2024 +0400

    Remove the un-used function in te `packetConn` interface that did not get removed during the merging process

commit 88bb1f5
Author: ilolicon <97431110@qq.com>
Date:   Wed Apr 12 16:23:06 2023 +0800

    Refactoring the variable name `Iface` to `Interface` and `ifaceIndex` to `ifIndex`(keeping it consistent with `ControlMessage`)

    Signed-off-by: ilolicon <97431110@qq.com>

commit 887b4e2
Author: ilolicon <97431110@qq.com>
Date:   Wed Apr 12 10:39:36 2023 +0800

    feat: interface binding

    Signed-off-by: ilolicon <97431110@qq.com>

Signed-off-by: Matthieu Pignolet <matthieu@matthieu-dev.xyz>
@MatthieuCoder
Copy link
Contributor Author

now ready!

Hipska
Hipska suggested changes Oct 30, 2024
View reviewed changes
ping.go Outdated Show resolved Hide resolved
@MatthieuCoder
Change the InterfaceDevice property to InterfaceName to match the…
94017ff 
… `net.InterfaceByName` naming convention

Sgguestd by @Hipska

Signed-off-by: Matthieu Pignolet <matthieu@matthieu-dev.xyz>
SuperQ
SuperQ approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Thanks. We can fix the Go version matrix later. For now we can live with the version provide in the machine image.

@SuperQ SuperQ merged commit a36072f into prometheus-community:main Oct 31, 2024
5 checks passed
@HsimWong HsimWong mentioned this pull request Nov 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

@Hipska Hipska Hipska approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MatthieuCoder @SuperQ @Hipska