Added audit activities when secret variables are upserted - #49412
Merged
juan-fdz-hawa merged 1 commit intoJul 16, 2026
Conversation
juan-fdz-hawa
commented
Jul 16, 2026
| results, err := ds.GetSecretVariables(ctx, nil) | ||
| assert.NoError(t, err) | ||
| assert.Empty(t, results) | ||
| require.NoError(t, err) |
Contributor
Author
There was a problem hiding this comment.
changed assert -> require for consistency sake
juan-fdz-hawa
commented
Jul 16, 2026
| ctx, | ||
| user, | ||
| fleet.ActivityCreatedCustomVariable{ | ||
| CustomVariableName: name, |
Contributor
Author
There was a problem hiding this comment.
The variable ID is not used anywhere so omitting it here is fine
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49412 +/- ##
========================================
Coverage 68.16% 68.16%
========================================
Files 3855 3856 +1
Lines 243602 244207 +605
Branches 13008 13010 +2
========================================
+ Hits 166045 166468 +423
- Misses 62559 62695 +136
- Partials 14998 15044 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
WalkthroughSecret variable upserts now return created and updated variable names. The service emits audit activities for each changed variable, including a new updated-custom-variable activity payload. Backend, datastore, mock, and integration tests cover creation, updates, unchanged values, dry runs, and GitOps execution. The frontend adds the activity type, filter label, and activity-feed rendering. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)server/service/integration_core_test.goast-grep timed out on this file server/service/integration_mdm_test.goast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/secret_variables_test.go`:
- Around line 102-116: In the test flow around GetSecretVariables, change the
post-update length assertion to validate the newly fetched updated slice rather
than rechecking original, ensuring updated[0] is guarded before the equality
assertion.
In `@server/service/secret_variables.go`:
- Around line 59-88: The secret-variable activity loops currently return on the
first svc.NewActivity failure after persistence, preventing remaining audit
events from being emitted. Update the created and updated loops to log
activity-emission errors and continue processing all names, while preserving the
successful upsert result and avoiding a whole-request failure caused solely by
NewActivity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2919b101-3d65-4ae4-bbe4-6cecb1a7095f
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/audit-logs.mdis excluded by!**/*.md
📒 Files selected for processing (12)
changes/16779-secret-variable-spec-activitiesfrontend/interfaces/activity.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxserver/datastore/mysql/secret_variables.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/software_installers_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_core_test.goserver/service/secret_variables.goserver/service/secret_variables_test.go
juan-fdz-hawa
force-pushed
the
16779-emit-activities-on-secret-upsert-endpoint
branch
from
July 16, 2026 17:22
1ef7578 to
f4c9b27
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/service/secret_variables_test.go`:
- Around line 110-115: Remove t.Parallel() from
TestCreateSecretVariablesEmitsActivities so it cannot race with
TestCreateSecretVariables while that test changes the package-global
testSetEmptyPrivateKey; preserve the test’s existing setup and assertions.
- Around line 117-140: Update CreateSecretVariables and its test coverage so
secret-variable persistence and all corresponding activity records are
failure-atomic: when any NewActivity call fails, neither the upsert nor earlier
activities may remain committed, or the missing activity must be durably
retried. Add a test that injects a later ActivityMock.NewActivity failure and
verifies the required rollback or durable retry behavior, while preserving
separate created and updated activity emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af51b154-d9b4-43fc-accb-3768b3b6e2bf
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/audit-logs.mdis excluded by!**/*.md
📒 Files selected for processing (14)
changes/16779-secret-variable-spec-activitiescmd/fleetctl/fleetctl/gitops_test.gofrontend/interfaces/activity.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxserver/datastore/mysql/secret_variables.goserver/datastore/mysql/secret_variables_test.goserver/datastore/mysql/software_installers_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_core_test.goserver/service/integration_mdm_test.goserver/service/secret_variables.goserver/service/secret_variables_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- changes/16779-secret-variable-spec-activities
- server/fleet/activities.go
- server/fleet/datastore.go
- server/datastore/mysql/secret_variables.go
- server/service/secret_variables.go
- server/mock/datastore_mock.go
- server/service/integration_core_test.go
Comment on lines
+110
to
+115
| func TestCreateSecretVariablesEmitsActivities(t *testing.T) { | ||
| t.Parallel() | ||
| ds := new(mock.Store) | ||
| opts := &TestServerOpts{} | ||
| svc, ctx := newTestService(t, ds, nil, nil, opts) | ||
| ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ID: 1, GlobalRole: new(fleet.RoleAdmin)}}) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not run this test in parallel with the global private-key override.
TestCreateSecretVariables also runs in parallel and temporarily sets package-global testSetEmptyPrivateKey. This test can consequently fail with the missing-private-key error. Remove t.Parallel() or replace the global override with per-service state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/service/secret_variables_test.go` around lines 110 - 115, Remove
t.Parallel() from TestCreateSecretVariablesEmitsActivities so it cannot race
with TestCreateSecretVariables while that test changes the package-global
testSetEmptyPrivateKey; preserve the test’s existing setup and assertions.
Comment on lines
+117
to
+140
| t.Run("emits a created activity per created variable and an updated activity per updated variable", func(t *testing.T) { | ||
| ds.UpsertSecretVariablesFunc = func(ctx context.Context, secrets []fleet.SecretVariable) (created []string, updated []string, err error) { | ||
| return []string{"CREATED"}, []string{"UPDATED"}, nil | ||
| } | ||
| var activities []activity_api.ActivityDetails | ||
| opts.ActivityMock.NewActivityFunc = func(_ context.Context, _ *activity_api.User, activity activity_api.ActivityDetails) error { | ||
| activities = append(activities, activity) | ||
| return nil | ||
| } | ||
| err := svc.CreateSecretVariables(ctx, []fleet.SecretVariable{ | ||
| {Name: "FLEET_SECRET_CREATED", Value: "a"}, | ||
| {Name: "FLEET_SECRET_UPDATED", Value: "b"}, | ||
| }, false) | ||
| require.NoError(t, err) | ||
| require.Len(t, activities, 2) | ||
|
|
||
| createdActivity, ok := activities[0].(fleet.ActivityCreatedCustomVariable) | ||
| require.True(t, ok) | ||
| require.Equal(t, "CREATED", createdActivity.CustomVariableName) | ||
|
|
||
| updatedActivity, ok := activities[1].(fleet.ActivityUpdatedCustomVariable) | ||
| require.True(t, ok) | ||
| require.Equal(t, "UPDATED", updatedActivity.CustomVariableName) | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make secret changes and audit records failure-atomic.
The upsert commits before activities are written individually. If a later NewActivity fails, secrets and earlier activities remain persisted; retrying sees unchanged variables and never emits the missing audit record. Add failure coverage and use a transaction, outbox, or durable retry mechanism.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/service/secret_variables_test.go` around lines 117 - 140, Update
CreateSecretVariables and its test coverage so secret-variable persistence and
all corresponding activity records are failure-atomic: when any NewActivity call
fails, neither the upsert nor earlier activities may remain committed, or the
missing activity must be durably retried. Add a test that injects a later
ActivityMock.NewActivity failure and verifies the required rollback or durable
retry behavior, while preserving separate created and updated activity emission.
dantecatalfamo
approved these changes
Jul 16, 2026
rachaelshaw
approved these changes
Jul 16, 2026
juan-fdz-hawa
deleted the
16779-emit-activities-on-secret-upsert-endpoint
branch
July 16, 2026 22:54
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added audit activities when secret variables are created or updated through the
PUT /api/latest/fleet/spec/secret_variablesendpointChecklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit