Repository: kumahq/kuma
Pull request #11402 (2024-09-12 12:54:49)

fix(transparent-proxy): check DNS related CLI flags earlier
If both values `redirect.dns.enabled` and `redirect.dns.captureAll` would be provided in the config yaml via `--config` or `--config-file`, so far we would fail, but it's a correct scenario. We should check if `--redirect-dns` and `--redirect-all-dns-traffic` are not provided together, so we should do it before we'll load config from file.

### Checklist prior to review

<!--
Each of these sections need to be filled by the author when opening the PR.

If something doesn't apply please check the box and add a justification after the `--`
-->

- [x] [Link to relevant issue][1] as well as docs and UI issues
  - no relevant issues
- [x] This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like `syscall.Mkfifo` have equivalent implementation on the other OS
  - it won't
- [x] Tests (Unit test, E2E tests, manual test on universal and k8s)
  - tested locally
  - Don't forget `ci/` labels to run additional/fewer tests
- [x] Do you need to update [`UPGRADE.md`](../blob/master/UPGRADE.md)?
  - there is no need
- [x] Does it need to be backported according to the [backporting policy](../blob/master/CONTRIBUTING.md#backporting)? ([this](https://github.com/kumahq/kuma/actions/workflows/auto-backport.yaml) GH action will add "backport" label based on these [file globs](https://github.com/kumahq/kuma/blob/master/.github/workflows/auto-backport.yaml#L6), if you want to prevent it from adding the "backport" label use [no-backport-autolabel](https://github.com/kumahq/kuma/blob/master/.github/workflows/auto-backport.yaml#L8) label)
  - there is no need

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

[1]: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword



Patch:
diff --git a/app/kumactl/cmd/install/install_transparent_proxy.go b/app/kumactl/cmd/install/install_transparent_proxy.go
index d25c13a01..a7c20cf92 100644
--- a/app/kumactl/cmd/install/install_transparent_proxy.go
+++ b/app/kumactl/cmd/install/install_transparent_proxy.go
@@ -25,6 +25,8 @@ const (
 	flagTransparentProxyConfig     = "config"
 	flagTransparentProxyConfigFile = "config-file"
 	flagIptablesExecutables        = "iptables-executables"
+	flagRedirectDNS                = "redirect-dns"
+	flagRedirectAllDNSTraffic      = "redirect-all-dns-traffic"
 )
 
 const (
@@ -113,7 +115,8 @@ runuser -u kuma-dp -- \
 
 			parseConfigFlags := func(flag *pflag.Flag, value string) error {
 				switch flag.Name {
-				case flagHelp, flagDryRun, flagTransparentProxyConfig, flagTransparentProxyConfigFile:
+				case flagHelp, flagDryRun, flagTransparentProxyConfig, flagTransparentProxyConfigFile,
+					flagRedirectDNS, flagRedirectAllDNSTraffic:
 					return flag.Value.Set(value)
 				default:
 					return nil
@@ -153,6 +156,14 @@ runuser -u kuma-dp -- \
 				)
 			}
 
+			if cfg.Redirect.DNS.Enabled && cfg.Redirect.DNS.CaptureAll {
+				return errors.Errorf(
+					"only one of '--%s' or '--%s' should be specified",
+					flagRedirectDNS,
+					flagRedirectAllDNSTraffic,
+				)
+			}
+
 			// After parsing the config flags, we load the configuration, which involves parsing
 			// the provided YAML or JSON, and including environment variables if present
 			if err := cfgLoader.Load(cmd.InOrStdin(), []byte(configValue), configFile); err != nil {
@@ -166,10 +177,6 @@ runuser -u kuma-dp -- \
 				return err
 			}
 
-			if cfg.Redirect.DNS.CaptureAll && cfg.Redirect.DNS.Enabled {
-				return errors.Errorf("one of --redirect-dns or --redirect-all-dns-traffic should be specified")
-			}
-
 			if cfg.Redirect.DNS.CaptureAll {
 				cfg.Redirect.DNS.Enabled = true
 			}
@@ -227,8 +234,8 @@ runuser -u kuma-dp -- \
 	cmd.Flags().Var(&cfg.Redirect.Outbound.ExcludePorts, "exclude-outbound-ports", "a comma separated list of outbound ports to exclude from redirect to Envoy")
 	cmd.Flags().StringVar(&cfg.KumaDPUser, "kuma-dp-user", cfg.KumaDPUser, fmt.Sprintf("the username or UID of the user that will run kuma-dp. If not provided, the system will search for a user with the default UID ('%s') or the default username ('%s')", consts.OwnerDefaultUID, consts.OwnerDefaultUsername))
 	cmd.Flags().StringVar(&cfg.KumaDPUser, "kuma-dp-uid", cfg.KumaDPUser, "the uid of the user that will run kuma-dp")
-	cmd.Flags().BoolVar(&cfg.Redirect.DNS.Enabled, "redirect-dns", cfg.Redirect.DNS.Enabled, "redirect only DNS requests targeted to the servers listed in /etc/resolv.conf to a specified port")
-	cmd.Flags().BoolVar(&cfg.Redirect.DNS.CaptureAll, "redirect-all-dns-traffic", cfg.Redirect.DNS.CaptureAll, "redirect all DNS traffic to a specified port, unlike --redirect-dns this will not be limited to the dns servers identified in /etc/resolve.conf")
+	cmd.Flags().BoolVar(&cfg.Redirect.DNS.Enabled, flagRedirectDNS, cfg.Redirect.DNS.Enabled, "redirect only DNS requests targeted to the servers listed in /etc/resolv.conf to a specified port")
+	cmd.Flags().BoolVar(&cfg.Redirect.DNS.CaptureAll, flagRedirectAllDNSTraffic, cfg.Redirect.DNS.CaptureAll, "redirect all DNS traffic to a specified port, unlike --redirect-dns this will not be limited to the dns servers identified in /etc/resolve.conf")
 	cmd.Flags().Var(&cfg.Redirect.DNS.Port, "redirect-dns-port", "the port where the DNS agent is listening")
 	cmd.Flags().StringVar(&cfg.Redirect.DNS.UpstreamTargetChain, "redirect-dns-upstream-target-chain", cfg.Redirect.DNS.UpstreamTargetChain, "(optional) the iptables chain where the upstream DNS requests should be directed to. It is only applied for IP V4. Use with care.")
 	cmd.Flags().BoolVar(&cfg.StoreFirewalld, "store-firewalld", cfg.StoreFirewalld, "store the iptables changes with firewalld")
diff --git a/pkg/config/loader.go b/pkg/config/loader.go
index 5e51e4d54..4c355496c 100644
--- a/pkg/config/loader.go
+++ b/pkg/config/loader.go
@@ -5,17 +5,13 @@ import (
 	"os"
 	"reflect"
 
-	"github.com/go-logr/logr"
 	"github.com/kelseyhightower/envconfig"
 	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
-
-	"github.com/kumahq/kuma/pkg/core"
 )
 
 type Loader struct {
-	logger logr.Logger
-	cfg    Config
+	cfg Config
 
 	strict        bool
 	includeEnv    bool
@@ -24,10 +20,7 @@ type Loader struct {
 }
 
 func NewLoader(cfg Config) *Loader {
-	return &Loader{
-		logger: core.Log.WithName("config"),
-		cfg:    cfg,
-	}
+	return &Loader{cfg: cfg}
 }
 
 func (l *Loader) WithStrictParsing() *Loader {
@@ -59,7 +52,6 @@ func (l *Loader) Load(stdin io.Reader, content []byte, filename string) error {
 
 func (l *Loader) LoadFile(filename string) error {
 	if filename == "" {
-		l.logger.Info("no configuration file provided, skipping file-based config loading")
 		return l.postProcess()
 	}
 
@@ -81,7 +73,6 @@ func (l *Loader) LoadFile(filename string) error {
 
 func (l *Loader) LoadReader(r io.Reader) error {
 	if r == nil {
-		l.logger.Info("no configuration reader provided, skipping reader-based config loading")
 		return nil
 	}
 
@@ -99,7 +90,6 @@ func (l *Loader) LoadBytes(content []byte) error {
 	}
 
 	if content == nil {
-		l.logger.Info("no configuration content provided, skipping byte-based config loading")
 		return nil
 	}
 


Test patch:
diff --git a/app/kumactl/cmd/install/install_transparent_proxy_test.go b/app/kumactl/cmd/install/install_transparent_proxy_test.go
index 37b208259..6321bc8a3 100644
--- a/app/kumactl/cmd/install/install_transparent_proxy_test.go
+++ b/app/kumactl/cmd/install/install_transparent_proxy_test.go
@@ -203,7 +203,7 @@ var _ = Context("kumactl install transparent proxy", func() {
 				"--redirect-all-dns-traffic",
 			},
 			goldenFile:   "install-transparent-proxy.defaults.golden.txt",
-			errorMatcher: Equal("Error: one of --redirect-dns or --redirect-all-dns-traffic should be specified\n"),
+			errorMatcher: Equal("Error: only one of '--redirect-dns' or '--redirect-all-dns-traffic' should be specified\n"),
 		}),
 		Entry("should error out on invalid port value", testCase{
 			extraArgs: []string{