Revert "Implement IsValueFromSecret and split Refresh into Refresh and RefreshNow" (#47072)

Reverts #46931

See incident-50229

Author: aiuto

cmd/process-agent/api/secrets.go

@@ -10,7 +10,7 @@ import (
 )
 
 func secretRefreshHandler(deps APIServerDeps, w http.ResponseWriter, _ *http.Request) {
-	response, err := deps.Secrets.RefreshNow()
+	response, err := deps.Secrets.Refresh(true)
 	if err != nil {
 		deps.Log.Errorf("error while refreshing secrets: %s", err)
 		writeError(err, http.StatusInternalServerError, w)

cmd/security-agent/api/agent/agent.go

@@ -146,7 +146,7 @@ func (a *Agent) makeFlare(w http.ResponseWriter, _ *http.Request) {
 }
 
 func (a *Agent) refreshSecrets(w http.ResponseWriter, _ *http.Request) {
-	res, err := a.secrets.RefreshNow()
+	res, err := a.secrets.Refresh(true)
 	if err != nil {
 		log.Errorf("error while refreshing secrets: %s", err)
 		w.Header().Set("Content-Type", "application/json")

comp/core/autodiscovery/autodiscoveryimpl/secrets_test.go

@@ -59,18 +59,10 @@ func (m *MockSecretResolver) SubscribeToChanges(callback secrets.SecretChangeCal
 	m.subscribers = append(m.subscribers, callback)
 }
 
-func (m *MockSecretResolver) Refresh() bool {
-	return false
-}
-
-func (m *MockSecretResolver) RefreshNow() (string, error) {
+func (m *MockSecretResolver) Refresh(_ bool) (string, error) {
 	return "", nil
 }
 
-func (m *MockSecretResolver) IsValueFromSecret(_ string) bool {
-	return false
-}
-
 func (m *MockSecretResolver) haveAllScenariosBeenCalled() bool {
 	for _, scenario := range m.scenarios {
 		if scenario.called == 0 {

comp/core/secrets/def/component.go

@@ -39,13 +39,10 @@ type Component interface {
 	Resolve(data []byte, origin string, imageName string, kubeNamespace string, notify bool) ([]byte, error)
 	// SubscribeToChanges registers a callback to be invoked whenever secrets are resolved or refreshed
 	SubscribeToChanges(callback SecretChangeCallback)
-	// Refresh schedules a throttled asynchronous secret refresh. Returns true if the
-	// secret refresh mechanism is enabled (backend configured and refresh interval set).
-	Refresh() bool
-	// RefreshNow performs an immediate blocking secret refresh, returning an informative message suitable for user display.
-	RefreshNow() (string, error)
-	// IsValueFromSecret returns true if the given value was ever resolved from a secret handle.
-	IsValueFromSecret(value string) bool
+	// Refresh will resolve secret handles again, notifying any subscribers of changed values.
+	// If updateNow is true, the function performs the refresh immediately and blocks, returning an informative message suitable for user display.
+	// If updateNow is false, the function will asynchronously perform a refresh, and may fail to refresh due to throttling. No message is returned, just an empty string.
+	Refresh(updateNow bool) (string, error)
 	// RemoveOrigin removes a origin from the internal cache of the secret component. This does not remove secrets
 	// from the cache but the reference where those secrets are used.
 	RemoveOrigin(origin string)

comp/core/secrets/impl/secrets.go

@@ -87,10 +87,6 @@ type secretResolver struct {
 	// list of handles and where they were found
 	origin handleToContext
 
-	// resolvedSecretValues is an append-only set of all secret values ever returned by the backend.
-	// old values are retained for IsValueFromSecret lookups.
-	resolvedSecretValues map[string]struct{}
-
 	backendType                     string
 	backendConfig                   map[string]interface{}
 	backendCommand                  string
@@ -143,7 +139,6 @@ func newEnabledSecretResolver(telemetry telemetry.Component) *secretResolver {
 	return &secretResolver{
 		cache:                   make(map[string]string),
 		origin:                  make(handleToContext),
-		resolvedSecretValues:    make(map[string]struct{}),
 		tlmSecretBackendElapsed: telemetry.NewGauge("secret_backend", "elapsed_ms", []string{"command", "exit_code"}, "Elapsed time of secret backend invocation"),
 		tlmSecretUnmarshalError: telemetry.NewCounter("secret_backend", "unmarshal_errors_count", []string{}, "Count of errors when unmarshalling the output of the secret binary"),
 		tlmSecretResolveError:   telemetry.NewCounter("secret_backend", "resolve_errors_count", []string{"error_kind", "handle"}, "Count of errors when resolving a secret"),
@@ -217,7 +212,7 @@ func (r *secretResolver) writeDebugInfo(w http.ResponseWriter, _ *http.Request)
 }
 
 func (r *secretResolver) handleRefresh(w http.ResponseWriter, _ *http.Request) {
-	result, err := r.RefreshNow()
+	result, err := r.Refresh(true)
 	if err != nil {
 		log.Infof("could not refresh secrets: %s", err)
 		setJSONError(w, err, 500)
@@ -672,42 +667,28 @@ func (r *secretResolver) processSecretResponse(secretResponse map[string]string,
 	}
 	// add results to the cache
 	stdmaps.Copy(r.cache, secretResponse)
-	// track all resolved values in an append-only set for IsValueFromSecret lookups
-	for _, secretValue := range secretResponse {
-		r.resolvedSecretValues[secretValue] = struct{}{}
-	}
 	// return info about the handles sorted by their name
 	sort.Slice(handleInfoList, func(i, j int) bool {
 		return handleInfoList[i].Name < handleInfoList[j].Name
 	})
 	return secretRefreshInfo{Handles: handleInfoList}
 }
 
-// Refresh schedules a throttled asynchronous secret refresh. Returns true if the
-// secret refresh mechanism is enabled (backend configured and refresh interval set).
-func (r *secretResolver) Refresh() bool {
-	if r.apiKeyFailureRefreshInterval == 0 || r.backendCommand == "" {
-		return false
+// Refresh will resolve secret handles again, notifying any subscribers of changed values.
+// If updateNow is true, the function performs the refresh immediately and blocks, returning an informative message suitable for user display.
+// If updateNow is false, the function will asynchronously perform a refresh, and may fail to refresh due to throttling. No message is returned, just an empty string.
+func (r *secretResolver) Refresh(updateNow bool) (string, error) {
+	if updateNow {
+		// blocking refresh
+		return r.performRefresh()
 	}
-	// non-blocking send, max 1 at a time, others dropped
+
+	// non-blocking refresh, max 1 at a time, others dropped
 	select {
 	case r.refreshTrigger <- struct{}{}:
 	default:
 	}
-	return true
-}
-
-// RefreshNow performs an immediate blocking secret refresh and returns an informative message suitable for user display.
-func (r *secretResolver) RefreshNow() (string, error) {
-	return r.performRefresh()
-}
-
-// IsValueFromSecret returns true if the given value was ever resolved from a secret handle.
-func (r *secretResolver) IsValueFromSecret(value string) bool {
-	r.lock.Lock()
-	defer r.lock.Unlock()
-	_, ok := r.resolvedSecretValues[value]
-	return ok
+	return "", nil
 }
 
 // RemoveOrigin removes a origin from the cache

comp/core/secrets/impl/secrets_test.go

@@ -591,7 +591,7 @@ func TestResolveThenRefresh(t *testing.T) {
 
 	// refresh the secrets and only collect newly updated keys
 	keysResolved = []string{}
-	output, err := resolver.RefreshNow()
+	output, err := resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, testConfNestedOriginMultiple, resolver.origin)
 	assert.Equal(t, []string{"some/second_level"}, keysResolved)
@@ -610,7 +610,7 @@ func TestResolveThenRefresh(t *testing.T) {
 
 	// refresh one last time and only those two handles have updated keys
 	keysResolved = []string{}
-	_, err = resolver.RefreshNow()
+	_, err = resolver.Refresh(true)
 	require.NoError(t, err)
 	slices.Sort(keysResolved)
 	assert.Equal(t, testConfNestedOriginMultiple, resolver.origin)
@@ -655,15 +655,15 @@ func TestRefreshAllowlist(t *testing.T) {
 	allowListPaths = []string{"api_key"}
 
 	// Refresh means nothing changes because allowlist doesn't allow it
-	_, err := resolver.RefreshNow()
+	_, err := resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, changes, []string{})
 
 	// now allow the config setting under scrutiny to change
 	allowListPaths = []string{"setting"}
 
 	// Refresh sees the change to the handle
-	_, err = resolver.RefreshNow()
+	_, err = resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, changes, []string{"second_value"})
 }
@@ -712,7 +712,7 @@ func TestRefreshAllowlistFromContainer(t *testing.T) {
 	})
 
 	// Refresh means nothing changes because allowlist doesn't allow it
-	_, err := resolver.RefreshNow()
+	_, err := resolver.Refresh(true)
 	require.NoError(t, err)
 	slices.Sort(changes)
 	assert.Equal(t, changes, []string{
@@ -759,7 +759,7 @@ func TestRefreshAllowlistAppliesToEachSettingPath(t *testing.T) {
 	}
 
 	// only 1 setting path got updated
-	_, err = resolver.RefreshNow()
+	_, err = resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, changedPaths, []string{"instances/0/password"})
 }
@@ -795,7 +795,7 @@ func TestRefreshAddsToAuditFile(t *testing.T) {
 	}
 
 	// Refresh the secrets, which will add to the audit file
-	_, err = resolver.RefreshNow()
+	_, err = resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, auditFileNumRows(tmpfile.Name()), 1)
 
@@ -806,7 +806,7 @@ func TestRefreshAddsToAuditFile(t *testing.T) {
 	}
 
 	// Refresh secrets again, which will add another row the audit file
-	_, err = resolver.RefreshNow()
+	_, err = resolver.Refresh(true)
 	require.NoError(t, err)
 	assert.Equal(t, auditFileNumRows(tmpfile.Name()), 2)
 
@@ -832,9 +832,9 @@ func TestRefreshModes(t *testing.T) {
 		return map[string]string{"api_key": "test_value"}, nil
 	}
 
-	t.Run("RefreshNow refreshes synchronously", func(t *testing.T) {
+	t.Run("updateNow=true refreshes synchronously", func(t *testing.T) {
 		calls.Store(0)
-		result, err := resolver.RefreshNow()
+		result, err := resolver.Refresh(true)
 		require.NoError(t, err)
 		assert.NotEmpty(t, result)
 		assert.Equal(t, int32(1), calls.Load())
@@ -848,16 +848,16 @@ func TestRefreshModes(t *testing.T) {
 		calls.Store(0)
 
 		// 1 refresh passes, 2 get dropped
-		resolver.Refresh()
-		resolver.Refresh()
-		resolver.Refresh()
+		resolver.Refresh(false)
+		resolver.Refresh(false)
+		resolver.Refresh(false)
 		time.Sleep(50 * time.Millisecond)
 
 		assert.Equal(t, int32(1), calls.Load(), "only first refresh should process")
 
 		// after interval, next should succeed
 		time.Sleep(100 * time.Millisecond)
-		resolver.Refresh()
+		resolver.Refresh(false)
 		time.Sleep(50 * time.Millisecond)
 
 		assert.Equal(t, int32(2), calls.Load(), "refresh after interval should process")
@@ -867,46 +867,14 @@ func TestRefreshModes(t *testing.T) {
 		resolver.apiKeyFailureRefreshInterval = 0
 
 		calls.Store(0)
-		resolver.Refresh()
-		resolver.Refresh()
+		resolver.Refresh(false)
+		resolver.Refresh(false)
 		time.Sleep(50 * time.Millisecond)
 
 		assert.Equal(t, int32(0), calls.Load(), "no refreshes when disabled")
 	})
 }
 
-func TestIsValueFromSecret(t *testing.T) {
-	tel := nooptelemetry.GetCompatComponent()
-	resolver := newEnabledSecretResolver(tel)
-	resolver.backendCommand = "some_command"
-
-	// initially no values are from secrets
-	assert.False(t, resolver.IsValueFromSecret("password1"))
-
-	resolver.fetchHookFunc = func([]string) (map[string]string, error) {
-		return map[string]string{"pass1": "password1"}, nil
-	}
-	_, err := resolver.Resolve(testSimpleConf, "test", "", "", true)
-	require.NoError(t, err)
-
-	assert.True(t, resolver.IsValueFromSecret("password1"))
-	assert.False(t, resolver.IsValueFromSecret("some_other_value"))
-
-	// mock key rotation
-	resolver.fetchHookFunc = func([]string) (map[string]string, error) {
-		return map[string]string{"pass1": "password2"}, nil
-	}
-	_, err = resolver.RefreshNow()
-	require.NoError(t, err)
-
-	// the new value is recognized
-	assert.True(t, resolver.IsValueFromSecret("password2"))
-	// the OLD value is still recognized
-	assert.True(t, resolver.IsValueFromSecret("password1"))
-
-	assert.False(t, resolver.IsValueFromSecret("some_other_hardcoded_key"))
-}
-
 func TestStartRefreshRoutineWithScatter(t *testing.T) {
 	testCases := []struct {
 		name                   string
@@ -1350,7 +1318,7 @@ func TestRefreshOutput(t *testing.T) {
 
 	password = "password2"
 
-	res, err := resolver.RefreshNow()
+	res, err := resolver.Refresh(true)
 	require.NoError(t, err)
 	res = strings.ReplaceAll(res, "\r", "") // templates use OS line breaks, removes \r line breaks from windows
 	assert.Equal(t, "=== Secret stats ===\nNumber of secrets reloaded: 1\nSecrets handle reloaded:\n\n- 'pass1':\n\tused in 'origin1' configuration in entry 'secret_backend_arguments/0'\n", res)

comp/core/secrets/mock/mock.go

@@ -19,12 +19,9 @@ import (
 
 // Mock is a mock of the secret Component useful for testing
 type Mock struct {
-	secretsCache          map[string]string
-	callbacks             []secrets.SecretChangeCallback
-	refreshHook           func() bool
-	refreshNowHook        func() (string, error)
-	isValueFromSecretHook func(string) bool
-	isValueFromSecretSet  map[string]struct{}
+	secretsCache map[string]string
+	callbacks    []secrets.SecretChangeCallback
+	refreshHook  func(bool) (string, error)
 }
 
 var _ secrets.Component = (*Mock)(nil)
@@ -92,55 +89,17 @@ func (m *Mock) SubscribeToChanges(callback secrets.SecretChangeCallback) {
 }
 
 // SetRefreshHook sets a hook function that will be called when Refresh is invoked
-func (m *Mock) SetRefreshHook(hook func() bool) {
+func (m *Mock) SetRefreshHook(hook func(bool) (string, error)) {
 	m.refreshHook = hook
 }
 
-// SetRefreshNowHook sets a hook function that will be called when RefreshNow is invoked
-func (m *Mock) SetRefreshNowHook(hook func() (string, error)) {
-	m.refreshNowHook = hook
-}
-
-// Refresh schedules an asynchronous secret refresh
-func (m *Mock) Refresh() bool {
+// Refresh will resolve secret handles again, notifying any subscribers of changed values
+func (m *Mock) Refresh(updateNow bool) (string, error) {
 	if m.refreshHook != nil {
-		return m.refreshHook()
-	}
-	return false
-}
-
-// RefreshNow performs an immediate blocking secret refresh
-func (m *Mock) RefreshNow() (string, error) {
-	if m.refreshNowHook != nil {
-		return m.refreshNowHook()
+		return m.refreshHook(updateNow)
 	}
 	return "", nil
 }
 
-// SetIsValueFromSecretHook sets a hook function that will be called when IsValueFromSecret is invoked
-func (m *Mock) SetIsValueFromSecretHook(hook func(string) bool) {
-	m.isValueFromSecretHook = hook
-}
-
-// SetSecretOriginatedValues sets a predefined set of values that IsValueFromSecret will recognize
-func (m *Mock) SetSecretOriginatedValues(values []string) {
-	m.isValueFromSecretSet = make(map[string]struct{}, len(values))
-	for _, v := range values {
-		m.isValueFromSecretSet[v] = struct{}{}
-	}
-}
-
-// IsValueFromSecret returns true if the given value was ever resolved from a secret handle
-func (m *Mock) IsValueFromSecret(value string) bool {
-	if m.isValueFromSecretHook != nil {
-		return m.isValueFromSecretHook(value)
-	}
-	if m.isValueFromSecretSet != nil {
-		_, ok := m.isValueFromSecretSet[value]
-		return ok
-	}
-	return false
-}
-
 // RemoveOrigin
 func (m *Mock) RemoveOrigin(_ string) {}

comp/core/secrets/noop-impl/types/types.go

@@ -24,20 +24,10 @@ func (r *SecretNoop) Resolve(data []byte, _ string, _ string, _ string, _ bool)
 	return data, nil
 }
 
-// Refresh does nothing and returns false
-func (r *SecretNoop) Refresh() bool {
-	return false
-}
-
-// RefreshNow does nothing
-func (r *SecretNoop) RefreshNow() (string, error) {
+// Refresh does nothing
+func (r *SecretNoop) Refresh(_ bool) (string, error) {
 	return "", nil
 }
 
-// IsValueFromSecret always returns false for noop
-func (r *SecretNoop) IsValueFromSecret(_ string) bool {
-	return false
-}
-
 // RemoveOrigin
 func (r *SecretNoop) RemoveOrigin(_ string) {}