config/remote/service: no need for mutex around api.API

This dependency is never changed.

Author: ajwerner

pkg/config/remote/service/service.go

@@ -123,6 +123,7 @@ type CoreAgentService struct {
 	// help when triaging via logs.
 	rcType string
 
+	api           api.API
 	clock         clock.Clock
 	hostname      string
 	tagsGetter    func() []string
@@ -162,7 +163,6 @@ type CoreAgentService struct {
 		sync.Mutex
 
 		uptane coreAgentUptaneClient
-		api    api.API
 
 		firstUpdate bool
 
@@ -248,13 +248,15 @@ func newOrgStatusPoller(refreshInterval time.Duration) *orgStatusPoller {
 }
 
 // start begins the periodic polling of org status
-func (p *orgStatusPoller) start(clock clock.Clock, getAPI func() api.API, rcType string) {
+func (p *orgStatusPoller) start(clock clock.Clock, apiClient api.API, rcType string) {
 	go func() {
-		p.poll(getAPI(), rcType)
+		timer := clock.Timer(0)
+		defer timer.Stop()
 		for {
 			select {
-			case <-clock.After(p.refreshInterval):
-				p.poll(getAPI(), rcType)
+			case <-timer.C:
+				p.poll(apiClient, rcType)
+				timer.Reset(p.refreshInterval)
 			case <-p.stopChan:
 				log.Infof("[%s] Stopping Remote Config org status poller", rcType)
 				return
@@ -601,6 +603,7 @@ func NewService(cfg model.Reader, rcType, baseRawURL, hostname string, tagsGette
 
 	now := clock.Now().UTC()
 	cas := &CoreAgentService{
+		api:                   http,
 		rcType:                rcType,
 		startupTime:           now,
 		hostname:              hostname,
@@ -635,7 +638,6 @@ func NewService(cfg model.Reader, rcType, baseRawURL, hostname string, tagsGette
 	cas.mu.backoffErrorCount = 0
 	cas.mu.products = make(map[rdata.Product]struct{})
 	cas.mu.newProducts = make(map[rdata.Product]struct{})
-	cas.mu.api = http
 	cas.mu.uptane = uptaneClient
 
 	cfg.OnUpdate(cas.apiKeyUpdateCallback())
@@ -655,7 +657,7 @@ func newRCBackendOrgUUIDProvider(http api.API) uptane.OrgUUIDProvider {
 func (s *CoreAgentService) Start() {
 	refreshBypassCh := make(chan chan<- struct{})
 	s.refreshBypassCh = refreshBypassCh
-	s.orgStatusPoller.start(s.clock, s.getAPI, s.rcType)
+	s.orgStatusPoller.start(s.clock, s.api, s.rcType)
 
 	go func() {
 		if s.disableConfigPollLoop {
@@ -671,9 +673,7 @@ func (s *CoreAgentService) Start() {
 // UpdatePARJWT updates the stored JWT for Private Action Runners
 // for authentication to the remote config backend.
 func (s *CoreAgentService) UpdatePARJWT(jwt string) {
-	s.mu.Lock()
-	s.mu.api.UpdatePARJWT(jwt)
-	s.mu.Unlock()
+	s.api.UpdatePARJWT(jwt)
 }
 
 func startWithAgentPollLoop(s *CoreAgentService, refreshBypassRequests <-chan chan<- struct{}) {
@@ -762,12 +762,6 @@ func (s *CoreAgentService) Stop() error {
 	return err
 }
 
-func (s *CoreAgentService) getAPI() api.API {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	return s.mu.api
-}
-
 func (s *CoreAgentService) calculateRefreshInterval() time.Duration {
 	s.mu.Lock()
 	backoffErrorCount := s.mu.backoffErrorCount
@@ -824,7 +818,7 @@ func (s *CoreAgentService) refresh() error {
 		s.mu.Unlock()
 		defer s.mu.Lock()
 		ctx := context.Background()
-		response, err = s.mu.api.Fetch(ctx, request)
+		response, err = s.api.Fetch(ctx, request)
 	}()
 	s.mu.lastUpdateErr = nil
 	if err != nil {
@@ -1091,7 +1085,7 @@ func (s *CoreAgentService) apiKeyUpdateCallback() func(string, model.Source, any
 		s.mu.Lock()
 		defer s.mu.Unlock()
 
-		s.mu.api.UpdateAPIKey(newKey)
+		s.api.UpdateAPIKey(newKey)
 
 		// Verify that the Org UUID hasn't changed
 		storedOrgUUID, err := s.mu.uptane.StoredOrgUUID()
@@ -1101,7 +1095,7 @@ func (s *CoreAgentService) apiKeyUpdateCallback() func(string, model.Source, any
 		}
 
 		// TODO: Do not hold the mutex while calling FetchOrgData.
-		newOrgUUID, err := s.mu.api.FetchOrgData(context.Background())
+		newOrgUUID, err := s.api.FetchOrgData(context.Background())
 		if err != nil {
 			log.Warnf("Could not get org uuid: %s", err)
 			return
@@ -1161,7 +1155,7 @@ func (s *CoreAgentService) ConfigResetState() (*pbgo.ResetStateConfigResponse, e
 	}
 	uptaneClient, err := uptane.NewCoreAgentClientWithRecreatedTransactionalStore(
 		metadata,
-		newRCBackendOrgUUIDProvider(s.mu.api),
+		newRCBackendOrgUUIDProvider(s.api),
 		opt...,
 	)
 	if err != nil {

pkg/config/remote/service/service_test.go

@@ -213,7 +213,7 @@ func newTestService(t *testing.T, api *mockAPI, coreAgentUptane *mockCoreAgentUp
 	service, err := NewService(cfg, "Remote Config", baseRawURL, "localhost", getHostTags, mockTelemetryReporter, agentVersion, options...)
 	require.NoError(t, err)
 	t.Cleanup(func() { service.Stop() })
-	service.mu.api = api
+	service.api = api
 	service.clock = clock
 	service.mu.uptane = coreAgentUptane
 	return service
@@ -317,13 +317,13 @@ func TestFetchOrgStatus503And504IncrementsErrCount(t *testing.T) {
 	assert.Equal(t, service.orgStatusPoller.mu.fetchOrgStatus503And504ErrCount, uint64(0))
 
 	api.On("FetchOrgStatus", mock.Anything).Return(response, httpapi.ErrGatewayTimeout)
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	assert.Equal(t, service.orgStatusPoller.mu.fetchOrgStatus503And504ErrCount, uint64(1))
 
 	assert.Nil(t, service.orgStatusPoller.getPreviousStatus())
 	api.On("FetchOrgStatus", mock.Anything).Return(response, httpapi.ErrGatewayTimeout)
 
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	assert.Equal(t, service.orgStatusPoller.mu.fetchOrgStatus503And504ErrCount, uint64(2))
 }
 
@@ -344,7 +344,7 @@ func TestFetchOrgStatusSuccessResetsErrorCount(t *testing.T) {
 	assert.Nil(t, service.orgStatusPoller.getPreviousStatus())
 	api.On("FetchOrgStatus", mock.Anything).Return(response, nil)
 
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	assert.Equal(t, service.orgStatusPoller.mu.fetchOrgStatus503And504ErrCount, uint64(0))
 }
 
@@ -460,7 +460,7 @@ func TestServiceBackoffFailureRecovery(t *testing.T) {
 	uptaneClient.On("TUFVersionState").Return(uptane.TUFVersions{}, nil)
 	uptaneClient.On("Update", lastConfigResponse).Return(nil)
 	uptaneClient.On("TargetsCustom").Return([]byte{}, nil)
-	service.mu.api = api
+	service.api = api
 
 	// Artificially set the backoff error count so we can test recovery
 	service.mu.backoffErrorCount = 3
@@ -978,7 +978,7 @@ func TestWithApiKeyUpdate(t *testing.T) {
 		assert.NoError(t, service.Stop())
 		assert.NoError(t, service.Stop()) // ensure idempotency
 	})
-	service.mu.api = api
+	service.api = api
 	service.mu.uptane = uptaneClient
 
 	cfg.SetWithoutSource("api_key", "updated")
@@ -1213,20 +1213,20 @@ func TestOrgStatus(t *testing.T) {
 	assert.Nil(t, service.orgStatusPoller.getPreviousStatus())
 	api.On("FetchOrgStatus", mock.Anything).Return(response, nil)
 
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	prev := service.orgStatusPoller.getPreviousStatus()
 	assert.True(t, prev.Enabled)
 	assert.True(t, prev.Authorized)
 
 	api.On("FetchOrgStatus", mock.Anything).Return(nil, fmt.Errorf("Error"))
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	prev = service.orgStatusPoller.getPreviousStatus()
 	assert.True(t, prev.Enabled)
 	assert.True(t, prev.Authorized)
 
 	response.Authorized = false
 	api.On("FetchOrgStatus", mock.Anything).Return(response, nil)
-	service.orgStatusPoller.poll(service.getAPI(), service.rcType)
+	service.orgStatusPoller.poll(service.api, service.rcType)
 	prev = service.orgStatusPoller.getPreviousStatus()
 	assert.True(t, prev.Enabled)
 	assert.False(t, prev.Authorized)