Fix empty message when host legacy routing is true

[vincentmli]

when missing tunnel and socketLB service check and when legacy host routing
set to true, the msg is empty with log

"level=warning msg=" Falling back to iptables-based masquerading." subsys=daemon"

this is confusing to users and not sure why falling back to iptables-based masquerading

before the fix:

run cilium agent in kernel 4.19 with

1 BPF masq enabled
2 Host legacy routing enabled
3 Tunnel enabled
4 FullHostReachableServices false

Cilium agent logs:
"level=warning msg=" Falling back to iptables-based masquerading." subsys=daemon"

the msg is empty because none of the switch cases matches, this could mis-lead users to
think BPF masquerade is not supported on kernel 4.19.

then when:

1 BPF masq enabled
2 Host legacy routing enabled
3 Tunnel disabled
4 Nodeport enabled

cilium agent keeps BPF masquerade enabled on kernel 4.19 without falling back to iptables-based masquerading

after the fix:

cilum agent log should log correct message + "Falling back to iptables-based masquerading."

Signed-off-by: Vincent Li v.li@f5.com

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Thanks for contributing!

Fixes: #21237

fix empty message when tunnel and socketLB service missing in switch case

[pchaigno]

The code looks good to me!

The commit description needs to be updated to reflect latest changes. Could you please also separate refactoring from fix, i.e. in 2 commits?

[vincentmli]

sure will do

[jibi]

Not sure if it would be more idiomatic to make msg an error and then log it with log.WithError(err).Warn("Falling back to iptables-based masquerading.")?

[vincentmli]

log.WithError(err).Warn("Falling back to iptables-based masquerading.")

you mean something like this?

diff --git a/daemon/cmd/daemon.go b/daemon/cmd/daemon.go
index e2f967d9a0..8f74241d0a 100644
--- a/daemon/cmd/daemon.go
+++ b/daemon/cmd/daemon.go
@@ -923,25 +923,25 @@ func NewDaemon(ctx context.Context, cleaner *daemonCleanup,
        // happen after invoking initKubeProxyReplacementOptions().
        if option.Config.EnableIPv4Masquerade && option.Config.EnableBPFMasquerade {
 
-               var msg string
+               var err error
                switch {
                case !option.Config.EnableNodePort:
-                       msg = fmt.Sprintf("BPF masquerade requires NodePort (--%s=\"true\").",
+                       err = fmt.Sprintf("BPF masquerade requires NodePort (--%s=\"true\").",
                                option.EnableNodePort)
                case !option.Config.EnableRemoteNodeIdentity:
-                       msg = fmt.Sprintf("BPF masquerade requires remote node identities (--%s=\"true\").",
+                       err = fmt.Sprintf("BPF masquerade requires remote node identities (--%s=\"true\").",
                                option.EnableRemoteNodeIdentity)
                case option.Config.EgressMasqueradeInterfaces != "":
-                       msg = fmt.Sprintf("BPF masquerade does not allow to specify devices via --%s (use --%s instead).",
+                       err = fmt.Sprintf("BPF masquerade does not allow to specify devices via --%s (use --%s instead).",
                                option.EgressMasqueradeInterfaces, option.Devices)
                case option.Config.TunnelingEnabled() && !hasFullHostReachableServices():
-                       msg = "BPF masquerade requires full host reachable services enabled in tunnel mode."
+                       err = "BPF masquerade requires full host reachable services enabled in tunnel mode."
                }
                // ipt.InstallRules() (called by Reinitialize()) happens later than
                // this  statement, so it's OK to fallback to iptables-based MASQ.
-               if len(msg) > 0 {
+               if err != nil {
                        option.Config.EnableBPFMasquerade = false
-                       log.Warn(msg + " Falling back to iptables-based masquerading.")
+                       log.WithError(err).Warn("Falling back to iptables-based masquerading.")
                        // Too bad, if we need to revert to iptables-based MASQ, we also cannot
                        // use BPF host routing since we need the upper stack.
                        if !option.Config.EnableHostLegacyRouting {

[jibi]

you mean something like this?

yup, but using fmt.Errorf() instead of fmt.Sprintf(), as that will return an object of type error (also no need for the .s at the end of all the error messages then)

[vincentmli]

like err = fmt.Errorf("BPF masquerade requires NodePort (--%s=\"true\")" ?

[jibi]

/test

[jibi]

Double approval ✔️ ✔️
Thanks for going through the refactoring and nits 🙏