Skip to content

Suppress "Not forwarding …" messages when forwarding has been disabled #2600

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

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Suppress "Not forwarding …" messages when forwarding has been disabled #2600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 18 additions & 2 deletions pkg/hostagent/hostagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ func New(instName string, stdout io.Writer, signalCh chan os.Signal, opts ...Opt
AdditionalArgs: sshutil.SSHArgsFromOpts(sshOpts),
}

ignoreTCP := false
ignoreUDP := false
for _, rule := range y.PortForwards {
if rule.Ignore && rule.GuestPortRange[0] == 1 && rule.GuestPortRange[1] == 65535 {
switch rule.Proto {
case limayaml.TCP:
ignoreTCP = true
logrus.Info("TCP port forwarding is disabled (except for SSH)")
case limayaml.UDP:
ignoreUDP = true
logrus.Info("UDP port forwarding is disabled")
Copy link
Member

Choose a reason for hiding this comment

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

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

What if you don't specify rule.Proto? we consider this as ignore both or not ignore any?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

I don't have strong opinions on this, but in some way all the info messages are only useful for debugging your configuration. The only one that will be left is:

INFO[0069] READY. Run `lima` to open the shell.

I would prefer to keep this at the info level, just so that if somebody puts the block to ignore port forwarding in their default.yaml or override.yaml files, and then forgets about it, they will have a reminder in the info log of what they have done.

What if you don't specify rule.Proto? we consider this as ignore both or not ignore any?

lima/pkg/limayaml/defaults.go

Lines 809 to 811 in 52f5ad3

if rule.Proto == "" {
rule.Proto = TCP
}

So you would need to add another rule to ignore with this PR:

portForwards:
- ignore: true
- ignore: true
  proto: udp

We probably should have support for proto: all and default to that.

You will notice that support for UDP and IPv6 is not fully considered in the port forwarding implementation and needs to be improved. Note that support for UDP port forwarding was only added to Lima last week in #2411.

Copy link
Member

Choose a reason for hiding this comment

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

Another issue - this works on single rule level. If you have:

portForwards:
- ignore: true

This is pretty clear, but if you have:

portForwards:
- guestPort: 80
  hostPort: 8080
- ignore: true

This code will treat the second rule as disabling port forwarding, while the purpose of the rule is to disable anything else port 80. In this case the user may want to see the Not forwarding message for port 80.

We can fix this by adding check for len(y.PortForwards) == 1 (hopefully lima append the any port forwading rule later).

This could be much simpler if we have a simple switch to disable this feature. Maybe to keep the simple structrue, add something like:

features:
  portForwarding: false

With this you can keep your complex configuration as is, and enable/disable it with one change in the features map.

Another way is to make portForwards structure more flexible:

portForwarding:
  disabled: true
  rules:
  - guestPort: x
    hostPort: y

It is little bit more complex but much more flexible. And it models exactly nicely the actual thing: port forwarding rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is pretty clear, but if you have:

portForwards:
- guestPort: 80
  hostPort: 8080
- ignore: true

This code will treat the second rule as disabling port forwarding, while the purpose of the rule is to disable anything else port 80. In this case the user may want to see the Not forwarding message for port 80.

And they will:

for _, rule := range y.PortForwards {
	if rule.Ignore && rule.GuestPortRange[0] == 1 && rule.GuestPortRange[1] == 65535 {
		…
	} else {
		break
	}
}

Note how the else { break } part will exit the loop as soon as it encounters a rule that doesn't ignore all ports. So ignoreTCP and ignoreUDP are only set if there are no other rules in front of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be much simpler if we have a simple switch to disable this feature. Maybe to keep the simple structrue, add something like:

features:
  portForwarding: false

I don't see how this is simpler than

portForwards:
- ignore: true

(once we make the default work for UDP as well).

Adding redundant syntactic sugar for functionality that is already available is just adding complexity.

And this additional "feature" is much more limited: what if the users only wants to disable UDP. Or only disable IPv6? You are just adding a special case for the one configuration that you personally are interested in, while adding to the cognitive load of everybody else who wants to wrap their head around how this can be configured.

Copy link
Member

Choose a reason for hiding this comment

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

Are these really info level messages? They are usefull only for debugging your configuration - so debug level.

I don't have strong opinions on this, but in some way all the info messages are only useful for debugging your configuration. The only one that will be left is:

INFO[0069] READY. Run `lima` to open the shell.

I would prefer to keep this at the info level, just so that if somebody puts the block to ignore port forwarding in their default.yaml or override.yaml files, and then forgets about it, they will have a reminder in the info log of what they have done.

This is debugging why it did not work as expected, so debug level.

What can be useful in info level, is message like:

Port forwarding is overridden by overides.yaml

And if we think that using non-default default.yaml is useful, we can log about that also. But I'm not sure about that, it depends if users had problems with such configuration.

If you configured port forwarding, info level message about forwarded port confirms that the system is operating as expected. This is also a change affecting the host, so it is import event - info level.

The messages about "waiting for retirement", "requirement satisfied" are helping the user to get some feedback about the progress. These are also useful when another program is running limactl for reporting progress. Provisioning k8s cluster takes hare about 3 minutes and having no feedback from the tool for 3 minutes is not good user experience.

Maybe the right thing is to log everything to a file, so you always have all the info, and show a nice progress with only high level info for the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

everybody else who wants to wrap their head around how this can be configured

Now, if you want to argue that the documentation should be improved beyond having a bunch of samples in default.yaml, then I will wholeheartedly agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe the right thing is to log everything to a file, so you always have all the info, and show a nice progress with only high level info for the user.

We do log everything to files (ha.stderr.log and ha.stdout.log) in the instance directory.

Personally I wouldn't mind hiding most everything until it starts waiting for requirements. Everything before is rarely useful unless you are debugging a failure:

INFO[0000] QEMU binary "/usr/local/bin/qemu-system-x86_64" seems properly signed with the "com.apple.security.hypervisor" entitlement
INFO[0000] Attempting to download the image              arch=x86_64 digest="sha256:0e25ca6ee9f08ec5d4f9910054b66ae7163c6152e81a3e67689d89bd6e4dfa69" location="https://cloud-images.ubuntu.com/releases/24.04/release-20240821/ubuntu-24.04-server-cloudimg-amd64.img"
INFO[0000] Using cache "/Users/jan/Library/Caches/lima/download/by-url-sha256/b2b185213b60ce48564393cf9eeb3c2ce4e92df4d2ca2edad8657c18d0e3052d/data"
INFO[0000] Attempting to download the nerdctl archive    arch=x86_64 digest="sha256:2c841e097fcfb5a1760bd354b3778cb695b44cd01f9f271c17507dc4a0b25606" location="https://github.com/containerd/nerdctl/releases/download/v1.7.6/nerdctl-full-1.7.6-linux-amd64.tar.gz"
INFO[0000] Using cache "/Users/jan/Library/Caches/lima/download/by-url-sha256/b86908f1f5ea2af45aec405f0fd389eba1999b51e3972ca78215ace94d2da2a6/data"
WARN[0001] [hostagent] GRPC port forwarding is experimental
INFO[0001] [hostagent] hostagent socket created at /Users/jan/.lima/default/ha.sock
INFO[0002] [hostagent] Using system firmware ("/usr/local/share/qemu/edk2-x86_64-code.fd")
INFO[0002] [hostagent] Starting QEMU (hint: to watch the boot progress, see "/Users/jan/.lima/default/serial*.log")
INFO[0002] SSH Local Port: 60022

But I think this should be a separate discussion from dropping individual Not forwarding … messages from the hostagent.

}
} else {
break
}
}
rules := make([]limayaml.PortForward, 0, 3+len(y.PortForwards))
// Block ports 22 and sshLocalPort on all IPs
for _, port := range []int{sshGuestPort, sshLocalPort} {
Expand Down Expand Up @@ -188,8 +204,8 @@ func New(instName string, stdout io.Writer, signalCh chan os.Signal, opts ...Opt
instName: instName,
instSSHAddress: inst.SSHAddress,
sshConfig: sshConfig,
portForwarder: newPortForwarder(sshConfig, sshLocalPort, rules, inst.VMType),
grpcPortForwarder: portfwd.NewPortForwarder(rules),
portForwarder: newPortForwarder(sshConfig, sshLocalPort, rules, ignoreTCP, inst.VMType),
grpcPortForwarder: portfwd.NewPortForwarder(rules, ignoreTCP, ignoreUDP),
driver: limaDriver,
signalCh: signalCh,
eventEnc: json.NewEncoder(stdout),
Expand Down
8 changes: 6 additions & 2 deletions pkg/hostagent/port.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,20 @@ type portForwarder struct {
sshConfig *ssh.SSHConfig
sshHostPort int
rules []limayaml.PortForward
ignore bool
vmType limayaml.VMType
}

const sshGuestPort = 22

var IPv4loopback1 = limayaml.IPv4loopback1

func newPortForwarder(sshConfig *ssh.SSHConfig, sshHostPort int, rules []limayaml.PortForward, vmType limayaml.VMType) *portForwarder {
func newPortForwarder(sshConfig *ssh.SSHConfig, sshHostPort int, rules []limayaml.PortForward, ignore bool, vmType limayaml.VMType) *portForwarder {
return &portForwarder{
sshConfig: sshConfig,
sshHostPort: sshHostPort,
rules: rules,
ignore: ignore,
vmType: vmType,
}
}
Expand Down Expand Up @@ -94,7 +96,9 @@ func (pf *portForwarder) OnEvent(ctx context.Context, ev *api.Event) {
}
local, remote := pf.forwardingAddresses(f)
if local == "" {
logrus.Infof("Not forwarding TCP %s", remote)
if !pf.ignore {
logrus.Infof("Not forwarding TCP %s", remote)
}
continue
}
logrus.Infof("Forwarding TCP from %s to %s", remote, local)
Expand Down
1 change: 1 addition & 0 deletions pkg/limayaml/limayaml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ type Proto = string

const (
TCP Proto = "tcp"
UDP Proto = "udp"
)

type PortForward struct {
Expand Down
18 changes: 15 additions & 3 deletions pkg/portfwd/forward.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,30 @@ var IPv4loopback1 = limayaml.IPv4loopback1

type Forwarder struct {
rules []limayaml.PortForward
ignoreTCP bool
ignoreUDP bool
closableListeners *ClosableListeners
}

func NewPortForwarder(rules []limayaml.PortForward) *Forwarder {
return &Forwarder{rules: rules, closableListeners: NewClosableListener()}
func NewPortForwarder(rules []limayaml.PortForward, ignoreTCP, ignoreUDP bool) *Forwarder {
return &Forwarder{
rules: rules,
ignoreTCP: ignoreTCP,
ignoreUDP: ignoreUDP,
closableListeners: NewClosableListener(),
}
}

func (fw *Forwarder) OnEvent(ctx context.Context, client *guestagentclient.GuestAgentClient, ev *api.Event) {
for _, f := range ev.LocalPortsAdded {
local, remote := fw.forwardingAddresses(f)
if local == "" {
logrus.Infof("Not forwarding %s %s", strings.ToUpper(f.Protocol), remote)
if !fw.ignoreTCP && f.Protocol == "tcp" {
logrus.Infof("Not forwarding TCP %s", remote)
}
if !fw.ignoreUDP && f.Protocol == "udp" {
logrus.Infof("Not forwarding UDP %s", remote)
}
continue
}
logrus.Infof("Forwarding %s from %s to %s", strings.ToUpper(f.Protocol), remote, local)
Expand Down
Loading