From eb1c319774c845814102208f7f37d361b1aec84b Mon Sep 17 00:00:00 2001 From: hc-github-team-consul-core Date: Tue, 16 Jan 2024 11:11:32 -0600 Subject: [PATCH] Backport of agent: remove data race in agent config into release/1.15.x (#20201) [1.15.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. Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com> --- agent/agent.go | 27 +++++++++++++++++++++------ agent/agent_endpoint.go | 41 +++++++++++++++++++++-------------------- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index fb248c28c558..134e016c54ef 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" @@ -221,6 +222,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 @@ -4250,11 +4254,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 f50d8e218574..de068ea6e264 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -11,14 +11,12 @@ import ( "strings" "time" - "github.com/hashicorp/consul/envoyextensions/xdscommon" + "github.com/hashicorp/go-bexpr" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" - "github.com/mitchellh/hashstructure" - - "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" @@ -29,6 +27,7 @@ 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" @@ -87,6 +86,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{ @@ -94,15 +95,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 } } @@ -117,22 +118,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(),