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

feat(icmp) support DoNotFragment + Size #633

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
+35 −2
Open

feat(icmp) support DoNotFragment + Size #633

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4a2b642
feat(ping)/support size and df
Dec 4, 2023
65f24e9
typo
antoinekh Dec 4, 2023
4515572
typo
antoinekh Dec 4, 2023
4f1cd93
add client configuration
antoinekh Dec 4, 2023
39952da
Revert "add client configuration"
antoinekh Dec 5, 2023
e71d178
update README info and fix dead links
antoinekh Dec 5, 2023
2e3c5bb
update comments for df flag
antoinekh Dec 5, 2023
104ec1d
Delete q:q!
antoinekh Dec 6, 2023
30aea72
put icmp config under client.icmp
antoinekh Dec 29, 2023
2340f65
linting
antoinekh Dec 29, 2023
4c0a523
doc
antoinekh Dec 29, 2023
73c0ff2
Update client/config.go
antoinekh Jan 5, 2024
e9703ec
add ICMPConfig everywhere
antoinekh Jan 5, 2024
ccafe4b
fix ICMPConfig everywhere
antoinekh Jan 5, 2024
a2ebfd1
update icmp size default to 24
antoinekh Jan 8, 2024
dbe1815
Merge branch 'master' into master
TwiN Jan 14, 2024
88c94e2
Merge branch 'master' into master
TwiN Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,14 +241,14 @@ func Ping(address string, config *Config) (bool, time.Duration) {
}
pinger.Count = 1
pinger.Timeout = config.Timeout
pinger.Size = config.Icmp.Size
pinger.Size = config.ICMPConfig.Size
// Set the pinger's privileged mode to true for every GOOS except darwin
// See https://github.com/TwiN/gatus/issues/132
//
// Note that for this to work on Linux, Gatus must run with sudo privileges.
// See https://github.com/prometheus-community/pro-bing#linux
pinger.SetPrivileged(runtime.GOOS != "darwin")
if config.Icmp.Df {
if config.ICMPConfig.DF {
pinger.SetDoNotFragment(true)
}
err = pinger.Run()
Expand Down
24 changes: 11 additions & 13 deletions client/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ var (
Insecure: false,
IgnoreRedirect: false,
Timeout: defaultTimeout,
Icmp: &Icmp{
Df: false,
ICMPConfig: &ICMPConfig{
DF: false,
Size: defaultSize,
},
}
Expand Down Expand Up @@ -61,7 +61,7 @@ type Config struct {
DNSResolver string `yaml:"dns-resolver,omitempty"`

// See ICMP for more details.
Icmp *Icmp `yaml:"icmp,omitempty"`
ICMPConfig *ICMPConfig `yaml:"icmp,omitempty"`

// OAuth2Config is the OAuth2 configuration used for the client.
//
Expand All @@ -83,11 +83,9 @@ type DNSResolverConfig struct {
}

// ICMP is the configuration for the ICMP client specific config
type Icmp struct {
// Don't Fragment flag (DF) for the client
Df bool `yaml:"df"`
// Size of the packet
Size int `yaml:"size"`
type ICMPConfig struct {
DF bool `yaml:"df"` // Don't Fragment flag used for the ICMP client
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
DF bool `yaml:"df"` // Don't Fragment flag used for the ICMP client
DF bool `yaml:"df"` // Don't Fragment flag used for the ICMP client

Size int `yaml:"size"` // Size of the packet
}

// OAuth2Config is the configuration for the OAuth2 client credentials flow
Expand All @@ -110,15 +108,15 @@ func (c *Config) ValidateAndSetDefaults() error {
}

// Only validate or set defaults for Icmp if it's not nil
if c.Icmp != nil {
if c.ICMPConfig != nil {
// limit for pro-ping, below 25 it's not working
if c.Icmp.Size < 25 {
c.Icmp.Size = 25
if c.ICMPConfig.Size < 25 {
c.ICMPConfig.Size = 25
}
} else {
// If Icmp is nil, let's initialize it with default values
c.Icmp = &Icmp{
Df: false,
c.ICMPConfig = &ICMPConfig{
DF: false,
Size: 25,
Copy link
Owner

Choose a reason for hiding this comment

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

Based on what I just looked at, the current default value is 25 (on my machine, at least 🤣)

image

Side note, I also see that there's a comment on L112 that may no longer be valid - it says // limit for pro-ping, below 25 it's not working.

Copy link
Author

Choose a reason for hiding this comment

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

I've set this configuration because it's the only way I've found to make it work.
But maybe I'm missing something in Go.

I did the tests by validating with network captures:

  • A classic linux ping can have a data of 0.
  • If I don't specify a Size or defaultSize by default the Size is 24.
    So you are right I can remove defaultSize = 24

But then, If I don't specifiy anything in icmp in my yaml file, the ping is not working.

That's why I put this code with the Size in it.

        // If Icmp is nil, let's initialize it with default values
        c.ICMPConfig = &ICMPConfig{
            DF:   false,
            Size: 24,
        }

But maybe this is not the good way to initialise it.

If I test with a Size <24 it doesn't work either.
I notice that this limitation exists in other tools that use pro-bing
For example
https://github.com/SuperQ/smokeping_prober
Packetdata size in bytes. Default 56 (Range: 24 - 65535)

That's why I put

    if c.ICMPConfig != nil {
        // limit for pro-ping, below 24 it's not working
        if c.ICMPConfig.Size < 24 {
            c.ICMPConfig.Size = 24
        }

And my mistake: the limit is 24, not 25.

}
}
Expand Down