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

NET-10480 - fix setting namespace and partition setting in dns-proxy mode #593

Merged
merged 3 commits into from
Jul 29, 2024
Merged

NET-10480 - fix setting namespace and partition setting in dns-proxy mode #593

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1234b0c
fix logic of running dns proxy because it relied on some ambient cont…
jmurret Jul 22, 2024
87b9c60
fixing setting namespace and partition when running in dns-proxy mode
jmurret Jul 24, 2024
76ee7f4
update code comments
jmurret Jul 29, 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
42 changes: 21 additions & 21 deletions pkg/consuldp/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,8 @@ const (
)

// bootstrapConfig generates the Envoy bootstrap config in JSON format.
jmurret marked this conversation as resolved.
Show resolved Hide resolved
func (cdp *ConsulDataplane) bootstrapConfig(ctx context.Context) (*bootstrap.BootstrapConfig, []byte, error) {
func (cdp *ConsulDataplane) getBootstrapParams(ctx context.Context) (*pbdataplane.GetEnvoyBootstrapParamsResponse, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

dns proxy requires bootstrap params to set the namespace and partition.....but it does not require the bootstrap config itself. So we are splitting this into two functions, so that startDNSProxy() can get require bootstrap params in its signature rather than just assuming it is already set on the struct.

svc := cdp.cfg.Proxy
envoy := cdp.cfg.Envoy

req := &pbdataplane.GetEnvoyBootstrapParamsRequest{
ServiceId: svc.ProxyID,
Expand All @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

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

A prior call will already have gotten bootstrap params for this call as well as startDNSProxy()

svc := cdp.cfg.Proxy
envoy := cdp.cfg.Envoy

prom := cdp.cfg.Telemetry.Prometheus
args := &bootstrap.BootstrapTplArgs{
Expand All @@ -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" {
Expand Down Expand Up @@ -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)
}

Expand Down
13 changes: 11 additions & 2 deletions pkg/consuldp/bootstrap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
Expand Down
26 changes: 17 additions & 9 deletions pkg/consuldp/consul_dataplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ type ConsulDataplane struct {
aclToken string
metricsConfig *metricsConfig
lifecycleConfig *lifecycleConfig

resolvedProxyConfig ProxyConfig
}

// NewConsulDP creates a new instance of ConsulDataplane
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
jmurret marked this conversation as resolved.
Show resolved Hide resolved
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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 8 additions & 0 deletions pkg/consuldp/consul_dataplane_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand Down
Loading