From 8067e8ee2fee27dd757a51dcfa75ce3d367c7005 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Fri, 12 Jan 2024 15:11:21 -0600 Subject: [PATCH] [1.16.x] agent: remove data race in agent config (#20200) To fix an issue displaying the current reloaded config in the v1/agent/self endpoint #18681 caused the agent's internal config struct member to be deepcopied and replaced on reload. This is not safe because the field is not protected by a lock, nor should it be due to how it is accessed by the rest of the system. This PR does the same deepcopy, but into a new field solely for the point of capturing the current reloaded values for display purposes. If there has been no reload then the original config is used. --- agent/agent.go | 27 +++++++++++++++++++------ agent/agent_endpoint.go | 44 ++++++++++++++++++++--------------------- 2 files changed, 43 insertions(+), 28 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 57edbdb185230..7a64b74bd1b18 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -24,6 +24,12 @@ import ( "github.com/armon/go-metrics" "github.com/armon/go-metrics/prometheus" + "github.com/rboyer/safeio" + "golang.org/x/net/http2" + "golang.org/x/net/http2/h2c" + "google.golang.org/grpc" + "google.golang.org/grpc/keepalive" + "github.com/hashicorp/go-connlimit" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" @@ -31,11 +37,6 @@ import ( "github.com/hashicorp/hcp-scada-provider/capability" "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" - "github.com/rboyer/safeio" - "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" - "google.golang.org/grpc" - "google.golang.org/grpc/keepalive" "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/acl/resolver" @@ -225,6 +226,9 @@ type Agent struct { // config is the agent configuration. config *config.RuntimeConfig + displayOnlyConfigCopy *config.RuntimeConfig + displayOnlyConfigCopyLock sync.Mutex + // Used for writing our logs logger hclog.InterceptLogger @@ -4310,11 +4314,22 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { a.config.EnableDebug = newCfg.EnableDebug // update Agent config with new config - a.config = newCfg.DeepCopy() + a.displayOnlyConfigCopyLock.Lock() + a.displayOnlyConfigCopy = newCfg.DeepCopy() + a.displayOnlyConfigCopyLock.Unlock() return nil } +func (a *Agent) getRuntimeConfigForDisplay() *config.RuntimeConfig { + a.displayOnlyConfigCopyLock.Lock() + defer a.displayOnlyConfigCopyLock.Unlock() + if a.displayOnlyConfigCopy != nil { + return a.displayOnlyConfigCopy.DeepCopy() + } + return a.config +} + // LocalBlockingQuery performs a blocking query in a generic way against // local agent state that has no RPC or raft to back it. It uses `hash` parameter // instead of an `index`. diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 23bfa49c88444..ade06172d84e0 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -11,16 +11,12 @@ import ( "strings" "time" + "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" - "github.com/mitchellh/hashstructure" - - "github.com/hashicorp/consul/envoyextensions/xdscommon" - "github.com/hashicorp/consul/version" - - "github.com/hashicorp/go-bexpr" "github.com/hashicorp/serf/coordinate" "github.com/hashicorp/serf/serf" + "github.com/mitchellh/hashstructure" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" @@ -32,11 +28,13 @@ import ( "github.com/hashicorp/consul/agent/structs" token_store "github.com/hashicorp/consul/agent/token" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/envoyextensions/xdscommon" "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/lib" "github.com/hashicorp/consul/logging" "github.com/hashicorp/consul/logging/monitor" "github.com/hashicorp/consul/types" + "github.com/hashicorp/consul/version" ) type Self struct { @@ -90,6 +88,8 @@ func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (i } } + displayConfig := s.agent.getRuntimeConfigForDisplay() + var xds *XDSSelf if s.agent.xdsServer != nil { xds = &XDSSelf{ @@ -97,15 +97,15 @@ func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (i "envoy": xdscommon.EnvoyVersions, }, // Prefer the TLS port. See comment on the XDSSelf struct for details. - Port: s.agent.config.GRPCTLSPort, + Port: displayConfig.GRPCTLSPort, Ports: GRPCPorts{ - Plaintext: s.agent.config.GRPCPort, - TLS: s.agent.config.GRPCTLSPort, + Plaintext: displayConfig.GRPCPort, + TLS: displayConfig.GRPCTLSPort, }, } // Fallback to standard port if TLS is not enabled. - if s.agent.config.GRPCTLSPort <= 0 { - xds.Port = s.agent.config.GRPCPort + if displayConfig.GRPCTLSPort <= 0 { + xds.Port = displayConfig.GRPCPort } } @@ -120,22 +120,22 @@ func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (i Version string BuildDate string }{ - Datacenter: s.agent.config.Datacenter, - PrimaryDatacenter: s.agent.config.PrimaryDatacenter, - NodeName: s.agent.config.NodeName, - NodeID: string(s.agent.config.NodeID), - Partition: s.agent.config.PartitionOrEmpty(), - Revision: s.agent.config.Revision, - Server: s.agent.config.ServerMode, + Datacenter: displayConfig.Datacenter, + PrimaryDatacenter: displayConfig.PrimaryDatacenter, + NodeName: displayConfig.NodeName, + NodeID: string(displayConfig.NodeID), + Partition: displayConfig.PartitionOrEmpty(), + Revision: displayConfig.Revision, + Server: displayConfig.ServerMode, // We expect the ent version to be part of the reported version string, and that's now part of the metadata, not the actual version. - Version: s.agent.config.VersionWithMetadata(), - BuildDate: s.agent.config.BuildDate.Format(time.RFC3339), + Version: displayConfig.VersionWithMetadata(), + BuildDate: displayConfig.BuildDate.Format(time.RFC3339), } return Self{ Config: config, - DebugConfig: s.agent.config.Sanitized(), - Coord: cs[s.agent.config.SegmentName], + DebugConfig: displayConfig.Sanitized(), + Coord: cs[displayConfig.SegmentName], Member: s.agent.AgentLocalMember(), Stats: s.agent.Stats(), Meta: s.agent.State.Metadata(),