From 1234b0c30524437af34f546fab7d07728a3748af Mon Sep 17 00:00:00 2001 From: John Murret Date: Mon, 22 Jul 2024 17:14:30 -0600 Subject: [PATCH 1/3] fix logic of running dns proxy because it relied on some ambient context to be set. --- pkg/consuldp/bootstrap.go | 42 +++++++++++++-------------- pkg/consuldp/bootstrap_test.go | 13 +++++++-- pkg/consuldp/consul_dataplane.go | 26 +++++++++++------ pkg/consuldp/consul_dataplane_test.go | 8 +++++ 4 files changed, 57 insertions(+), 32 deletions(-) diff --git a/pkg/consuldp/bootstrap.go b/pkg/consuldp/bootstrap.go index f6185018..94ce5e61 100644 --- a/pkg/consuldp/bootstrap.go +++ b/pkg/consuldp/bootstrap.go @@ -27,9 +27,8 @@ const ( ) // bootstrapConfig generates the Envoy bootstrap config in JSON format. -func (cdp *ConsulDataplane) bootstrapConfig(ctx context.Context) (*bootstrap.BootstrapConfig, []byte, error) { +func (cdp *ConsulDataplane) getBootstrapParams(ctx context.Context) (*pbdataplane.GetEnvoyBootstrapParamsResponse, error) { svc := cdp.cfg.Proxy - envoy := cdp.cfg.Envoy req := &pbdataplane.GetEnvoyBootstrapParamsRequest{ ServiceId: svc.ProxyID, @@ -50,16 +49,17 @@ func (cdp *ConsulDataplane) bootstrapConfig(ctx context.Context) (*bootstrap.Boo rsp, err := cdp.dpServiceClient.GetEnvoyBootstrapParams(ctx, req) if err != nil { - return nil, nil, fmt.Errorf("failed to get envoy bootstrap params: %w", err) + return nil, fmt.Errorf("failed to get envoy bootstrap params: %w", err) } - // store the final resolved service for others to use. - cdp.resolvedProxyConfig = ProxyConfig{ - NodeName: rsp.NodeName, - ProxyID: cdp.cfg.Proxy.ProxyID, - Namespace: rsp.Namespace, - Partition: rsp.Partition, - } + return rsp, nil +} + +// bootstrapConfig generates the Envoy bootstrap config in JSON format. +func (cdp *ConsulDataplane) bootstrapConfig( + bootstrapParams *pbdataplane.GetEnvoyBootstrapParamsResponse) (*bootstrap.BootstrapConfig, []byte, error) { + svc := cdp.cfg.Proxy + envoy := cdp.cfg.Envoy prom := cdp.cfg.Telemetry.Prometheus args := &bootstrap.BootstrapTplArgs{ @@ -68,26 +68,26 @@ func (cdp *ConsulDataplane) bootstrapConfig(ctx context.Context) (*bootstrap.Boo AgentPort: strconv.Itoa(cdp.cfg.XDSServer.BindPort), AgentTLS: false, }, - ProxyCluster: rsp.Service, + ProxyCluster: bootstrapParams.Service, ProxyID: svc.ProxyID, - NodeName: rsp.NodeName, - ProxySourceService: rsp.Service, - AdminAccessLogConfig: rsp.AccessLogs, + NodeName: bootstrapParams.NodeName, + ProxySourceService: bootstrapParams.Service, + AdminAccessLogConfig: bootstrapParams.AccessLogs, AdminAccessLogPath: defaultAdminAccessLogsPath, AdminBindAddress: envoy.AdminBindAddress, AdminBindPort: strconv.Itoa(envoy.AdminBindPort), LocalAgentClusterName: localClusterName, - Namespace: rsp.Namespace, - Partition: rsp.Partition, - Datacenter: rsp.Datacenter, + Namespace: bootstrapParams.Namespace, + Partition: bootstrapParams.Partition, + Datacenter: bootstrapParams.Datacenter, PrometheusCertFile: prom.CertFile, PrometheusKeyFile: prom.KeyFile, PrometheusScrapePath: prom.ScrapePath, } - if rsp.Identity != "" { - args.ProxyCluster = rsp.Identity - args.ProxySourceService = rsp.Identity + if bootstrapParams.Identity != "" { + args.ProxyCluster = bootstrapParams.Identity + args.ProxySourceService = bootstrapParams.Identity } if cdp.xdsServer.listenerNetwork == "unix" { @@ -116,7 +116,7 @@ func (cdp *ConsulDataplane) bootstrapConfig(ctx context.Context) (*bootstrap.Boo } if cdp.cfg.Telemetry.UseCentralConfig { - if err := mapstructure.WeakDecode(rsp.Config.AsMap(), &bootstrapConfig); err != nil { + if err := mapstructure.WeakDecode(bootstrapParams.Config.AsMap(), &bootstrapConfig); err != nil { return nil, nil, fmt.Errorf("failed parsing Proxy.Config: %w", err) } diff --git a/pkg/consuldp/bootstrap_test.go b/pkg/consuldp/bootstrap_test.go index 9a89581b..b0ec5fd4 100644 --- a/pkg/consuldp/bootstrap_test.go +++ b/pkg/consuldp/bootstrap_test.go @@ -300,14 +300,23 @@ func TestBootstrapConfig(t *testing.T) { dp.xdsServer = &xdsServer{listenerAddress: fmt.Sprintf("127.0.0.1:%d", xdsBindPort)} } - _, bsCfg, err := dp.bootstrapConfig(ctx) + params, err := dp.getBootstrapParams(ctx) + require.NoError(t, err) + + _, bsCfg, err := dp.bootstrapConfig(params) require.NoError(t, err) golden(t, bsCfg) validateBootstrapConfig(t, bsCfg) if tc.resolvedProxyConfig != nil { - require.Equal(t, *tc.resolvedProxyConfig, dp.resolvedProxyConfig) + proxyCfg := ProxyConfig{ + NodeName: params.NodeName, + ProxyID: dp.cfg.Proxy.ProxyID, + Namespace: params.Namespace, + Partition: params.Partition, + } + require.Equal(t, *tc.resolvedProxyConfig, proxyCfg) } }) } diff --git a/pkg/consuldp/consul_dataplane.go b/pkg/consuldp/consul_dataplane.go index 73a589cc..a5af7aa5 100644 --- a/pkg/consuldp/consul_dataplane.go +++ b/pkg/consuldp/consul_dataplane.go @@ -47,8 +47,6 @@ type ConsulDataplane struct { aclToken string metricsConfig *metricsConfig lifecycleConfig *lifecycleConfig - - resolvedProxyConfig ProxyConfig } // NewConsulDP creates a new instance of ConsulDataplane @@ -97,6 +95,8 @@ func validateConfig(cfg *Config) error { return errors.New("non-local xDS bind address not allowed") case cfg.Mode == ModeTypeSidecar && cfg.DNSServer.Port != -1 && !net.ParseIP(cfg.DNSServer.BindAddr).IsLoopback(): return errors.New("non-local DNS proxy bind address not allowed when running as a sidecar") + case cfg.Mode == ModeTypeDNSProxy && cfg.Proxy != nil && !(cfg.Proxy.Namespace == "" || cfg.Proxy.Namespace == "default"): + return errors.New("namespace must be empty or set to 'default' when running in dns-proxy mode") } creds := cfg.Consul.Credentials @@ -180,8 +180,15 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { cdp.dpServiceClient = pbdataplane.NewDataplaneServiceClient(state.GRPCConn) doneCh := make(chan error) + bootstrapParams, err := cdp.getBootstrapParams(ctx) + if err != nil { + cdp.logger.Error("failed to get bootstrap params", "error", err) + return fmt.Errorf("failed to get bootstrap config: %w", err) + } + cdp.logger.Debug("generated envoy bootstrap params", "params", bootstrapParams) + // start up DNS server - if err = cdp.startDNSProxy(ctx); err != nil { + if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, bootstrapParams); err != nil { cdp.logger.Error("failed to start the dns proxy", "error", err) return err } @@ -203,7 +210,7 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { } go cdp.startXDSServer(ctx) - bootstrapCfg, cfg, err := cdp.bootstrapConfig(ctx) + bootstrapCfg, cfg, err := cdp.bootstrapConfig(bootstrapParams) if err != nil { cdp.logger.Error("failed to get bootstrap config", "error", err) return fmt.Errorf("failed to get bootstrap config: %w", err) @@ -267,16 +274,17 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { return <-doneCh } -func (cdp *ConsulDataplane) startDNSProxy(ctx context.Context) error { +func (cdp *ConsulDataplane) startDNSProxy(ctx context.Context, + dnsConfig *DNSServerConfig, bootstrapParams *pbdataplane.GetEnvoyBootstrapParamsResponse) error { dnsClientInterface := pbdns.NewDNSServiceClient(cdp.serverConn) dnsServer, err := dns.NewDNSServer(dns.DNSServerParams{ - BindAddr: cdp.cfg.DNSServer.BindAddr, - Port: cdp.cfg.DNSServer.Port, + BindAddr: dnsConfig.BindAddr, + Port: dnsConfig.Port, Client: dnsClientInterface, Logger: cdp.logger, - Partition: cdp.resolvedProxyConfig.Partition, - Namespace: cdp.resolvedProxyConfig.Namespace, + Partition: bootstrapParams.Partition, + Namespace: bootstrapParams.Namespace, Token: cdp.aclToken, }) if err == dns.ErrServerDisabled { diff --git a/pkg/consuldp/consul_dataplane_test.go b/pkg/consuldp/consul_dataplane_test.go index 008b4f2a..f7a5102d 100644 --- a/pkg/consuldp/consul_dataplane_test.go +++ b/pkg/consuldp/consul_dataplane_test.go @@ -311,6 +311,14 @@ func TestNewConsulDPError(t *testing.T) { }, expectErr: "bearer token (or path to a file containing a bearer token) is required for login", }, + { + name: "dns-proxy mode - namespace set to non empty or default value", + mode: ModeTypeDNSProxy, + modFn: func(c *Config) { + c.Proxy.Namespace = "test" + }, + expectErr: "namespace must be empty or set to 'default' when running in dns-proxy mode", + }, } testCases = append(testCases, dnsProxyTestCases...) From 87b9c60aa52dd4ddff5131d02871df63581bedbf Mon Sep 17 00:00:00 2001 From: John Murret Date: Wed, 24 Jul 2024 16:12:01 -0600 Subject: [PATCH 2/3] fixing setting namespace and partition when running in dns-proxy mode --- pkg/consuldp/bootstrap.go | 2 +- pkg/consuldp/consul_dataplane.go | 36 +++++++++++++++++++------------- pkg/dns/dns.go | 6 ++++++ 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/pkg/consuldp/bootstrap.go b/pkg/consuldp/bootstrap.go index 94ce5e61..41a1a693 100644 --- a/pkg/consuldp/bootstrap.go +++ b/pkg/consuldp/bootstrap.go @@ -26,7 +26,7 @@ const ( defaultAdminAccessLogsPath = os.DevNull ) -// bootstrapConfig generates the Envoy bootstrap config in JSON format. +// getBootstrapParams makes a call using the service client to get the bootstrap params for eventually getting the Envoy bootstrap config. func (cdp *ConsulDataplane) getBootstrapParams(ctx context.Context) (*pbdataplane.GetEnvoyBootstrapParamsResponse, error) { svc := cdp.cfg.Proxy diff --git a/pkg/consuldp/consul_dataplane.go b/pkg/consuldp/consul_dataplane.go index a5af7aa5..9a11be6e 100644 --- a/pkg/consuldp/consul_dataplane.go +++ b/pkg/consuldp/consul_dataplane.go @@ -180,22 +180,15 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { cdp.dpServiceClient = pbdataplane.NewDataplaneServiceClient(state.GRPCConn) doneCh := make(chan error) - bootstrapParams, err := cdp.getBootstrapParams(ctx) - if err != nil { - cdp.logger.Error("failed to get bootstrap params", "error", err) - return fmt.Errorf("failed to get bootstrap config: %w", err) - } - cdp.logger.Debug("generated envoy bootstrap params", "params", bootstrapParams) - - // start up DNS server - if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, bootstrapParams); err != nil { - cdp.logger.Error("failed to start the dns proxy", "error", err) - return err - } // if running as DNS PRoxy, xDS Server and Envoy are disabled, so // return before configuring them. if cdp.cfg.Mode == ModeTypeDNSProxy { + // start up DNS server + if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, cdp.cfg.Proxy.Namespace, cdp.cfg.Proxy.Partition); err != nil { + cdp.logger.Error("failed to start the dns proxy", "error", err) + return err + } go func() { <-ctx.Done() doneCh <- nil @@ -210,6 +203,19 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { } go cdp.startXDSServer(ctx) + bootstrapParams, err := cdp.getBootstrapParams(ctx) + if err != nil { + cdp.logger.Error("failed to get bootstrap params", "error", err) + return fmt.Errorf("failed to get bootstrap config: %w", err) + } + cdp.logger.Debug("generated envoy bootstrap params", "params", bootstrapParams) + + // start up DNS server + if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, bootstrapParams.Namespace, bootstrapParams.Partition); err != nil { + cdp.logger.Error("failed to start the dns proxy", "error", err) + return err + } + bootstrapCfg, cfg, err := cdp.bootstrapConfig(bootstrapParams) if err != nil { cdp.logger.Error("failed to get bootstrap config", "error", err) @@ -275,7 +281,7 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { } func (cdp *ConsulDataplane) startDNSProxy(ctx context.Context, - dnsConfig *DNSServerConfig, bootstrapParams *pbdataplane.GetEnvoyBootstrapParamsResponse) error { + dnsConfig *DNSServerConfig, namespace, partition string) error { dnsClientInterface := pbdns.NewDNSServiceClient(cdp.serverConn) dnsServer, err := dns.NewDNSServer(dns.DNSServerParams{ @@ -283,8 +289,8 @@ func (cdp *ConsulDataplane) startDNSProxy(ctx context.Context, Port: dnsConfig.Port, Client: dnsClientInterface, Logger: cdp.logger, - Partition: bootstrapParams.Partition, - Namespace: bootstrapParams.Namespace, + Partition: partition, + Namespace: namespace, Token: cdp.aclToken, }) if err == dns.ErrServerDisabled { diff --git a/pkg/dns/dns.go b/pkg/dns/dns.go index 93d7f8ba..39880b2f 100644 --- a/pkg/dns/dns.go +++ b/pkg/dns/dns.go @@ -106,6 +106,8 @@ func (d *DNSServer) Start(ctx context.Context) error { d.lock.Lock() defer d.lock.Unlock() + d.logger.Debug("starting DNS proxy", "partition", d.partition, "namespace", d.namespace) + if d.running { return ErrServerRunning } @@ -215,6 +217,8 @@ func (d *DNSServer) queryConsulAndRespondUDP(buf []byte, addr net.Addr) { "x-consul-token", d.token, ) + logger.Debug("querying through udp", "partition", d.partition, "namespace", d.namespace) + resp, err := d.client.Query(ctx, req) if err != nil { logger.Error("error resolving consul request", "error", err) @@ -301,6 +305,8 @@ func (d *DNSServer) proxyTCPAcceptedConn(ctx context.Context, conn net.Conn, cli "x-consul-token", d.token, ) + logger.Debug("querying through tcp", "partition", d.partition, "namespace", d.namespace) + resp, err := client.Query(ctx, req) if err != nil { logger.Error("error resolving consul request", "error", err) From 76ee7f4297c4eeeab3b84c0689a003f99a3b7dd4 Mon Sep 17 00:00:00 2001 From: John Murret Date: Mon, 29 Jul 2024 11:43:29 -0600 Subject: [PATCH 3/3] update code comments --- pkg/consuldp/consul_dataplane.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/consuldp/consul_dataplane.go b/pkg/consuldp/consul_dataplane.go index 9a11be6e..76df0b99 100644 --- a/pkg/consuldp/consul_dataplane.go +++ b/pkg/consuldp/consul_dataplane.go @@ -184,11 +184,13 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { // if running as DNS PRoxy, xDS Server and Envoy are disabled, so // return before configuring them. if cdp.cfg.Mode == ModeTypeDNSProxy { - // start up DNS server + // start up DNS server with the configuration from the consul-dataplane flags / environment variables since + // envoy bootstrapping is bypassed. if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, cdp.cfg.Proxy.Namespace, cdp.cfg.Proxy.Partition); err != nil { cdp.logger.Error("failed to start the dns proxy", "error", err) return err } + // Wait for context to be done in a more simplified goroutine dns-proxy mode. go func() { <-ctx.Done() doneCh <- nil @@ -196,6 +198,7 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { return <-doneCh } + // Configure xDS and Envoy configuration continues here when running in sidecar mode. cdp.logger.Info("configuring xDS and Envoy") err = cdp.setupXDSServer() if err != nil { @@ -210,7 +213,7 @@ func (cdp *ConsulDataplane) Run(ctx context.Context) error { } cdp.logger.Debug("generated envoy bootstrap params", "params", bootstrapParams) - // start up DNS server + // start up DNS server with envoy bootstrap params. if err = cdp.startDNSProxy(ctx, cdp.cfg.DNSServer, bootstrapParams.Namespace, bootstrapParams.Partition); err != nil { cdp.logger.Error("failed to start the dns proxy", "error", err) return err