Skip to content

Commit d63fa63

Browse files
rgarciargarcia
committed
Simplify configure extension staging
1 parent 40d73b4 commit d63fa63

3 files changed

Lines changed: 70 additions & 64 deletions

File tree

‎server/cmd/api/api/chromium.go‎

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -108,15 +108,15 @@ func (s *ApiService) uploadExtensions(ctx context.Context, mr *multipart.Reader,
108108
s.chromiumConfigMu.Lock()
109109
defer s.chromiumConfigMu.Unlock()
110110

111-
requiresRestart, transaction, reqMsg, err := s.commitPreparedExtensions(ctx, prepared)
111+
transaction, reqMsg, err := s.commitPreparedExtensions(ctx, prepared)
112112
if reqMsg != "" {
113113
return badExtensionUpload(reqMsg)
114114
}
115115
if err != nil {
116116
return internalExtensionUpload(err.Error())
117117
}
118118

119-
restarted := forceRestart || requiresRestart
119+
restarted := forceRestart || prepared.requiresRestart
120120
var loadErr error
121121
if restarted {
122122
if err := s.restartChromiumAndWait(ctx, "extension upload"); err != nil {
@@ -259,22 +259,25 @@ func (batch *preparedExtensionBatch) cleanup() {
259259
}
260260
}
261261

262-
// applyExtensionZipItems installs name+zipTemp extension pairs and persists their startup
263-
// configuration. The boolean result reports whether enterprise policy requires a restart.
264-
func (s *ApiService) applyExtensionZipItems(ctx context.Context, items []extensionZipItem) (bool, string, error) {
262+
// installExtensionZipItems extracts, validates, and persists extension archives.
263+
// The caller must hold chromiumConfigMu while it commits the prepared batch.
264+
func (s *ApiService) installExtensionZipItems(ctx context.Context, items []extensionZipItem) (string, error) {
265+
if len(items) == 0 {
266+
return "", nil
267+
}
265268
prepared, reqMsg, err := s.prepareExtensionZipItems(ctx, items)
266269
if prepared != nil {
267270
defer prepared.cleanup()
268271
}
269272
if reqMsg != "" || err != nil {
270-
return false, reqMsg, err
273+
return reqMsg, err
271274
}
272-
requiresRestart, _, reqMsg, err := s.commitPreparedExtensions(ctx, prepared)
273-
return requiresRestart, reqMsg, err
275+
_, reqMsg, err = s.commitPreparedExtensions(ctx, prepared)
276+
return reqMsg, err
274277
}
275278

276-
// prepareExtensionZipItems performs archive extraction and validation before the global Chromium
277-
// configuration lock is acquired. commitPreparedExtensions rechecks destination names under lock.
279+
// prepareExtensionZipItems extracts and validates archives into staging paths.
280+
// commitPreparedExtensions rechecks destination names while the caller holds the config lock.
278281
func (s *ApiService) prepareExtensionZipItems(ctx context.Context, items []extensionZipItem) (*preparedExtensionBatch, string, error) {
279282
log := logger.FromContext(ctx)
280283
if err := os.MkdirAll(extensionsBaseDir, 0o755); err != nil {
@@ -438,22 +441,25 @@ func (batch *committedExtensionBatch) rollback() error {
438441
return rollbackErr
439442
}
440443

441-
func (s *ApiService) commitPreparedExtensions(ctx context.Context, batch *preparedExtensionBatch) (requiresRestart bool, transaction *committedExtensionBatch, reqMsg string, err error) {
444+
// commitPreparedExtensions moves a prepared batch into place and updates policy and
445+
// flags, rolling back partial commit failures. Callers may retain the returned
446+
// transaction to roll back a later activation failure.
447+
func (s *ApiService) commitPreparedExtensions(ctx context.Context, batch *preparedExtensionBatch) (transaction *committedExtensionBatch, reqMsg string, err error) {
442448
for _, extension := range batch.extensions {
443449
if _, statErr := os.Stat(extension.finalPath); statErr == nil {
444-
return false, nil, fmt.Sprintf("extension name already exists: %s", extension.name), nil
450+
return nil, fmt.Sprintf("extension name already exists: %s", extension.name), nil
445451
} else if !os.IsNotExist(statErr) {
446-
return false, nil, "", fmt.Errorf("failed to check extension dir: %w", statErr)
452+
return nil, "", fmt.Errorf("failed to check extension dir: %w", statErr)
447453
}
448454
}
449455

450456
flagsSnapshot, err := captureOptionalFileSnapshot(chromiumFlagsPath)
451457
if err != nil {
452-
return false, nil, "", fmt.Errorf("failed to snapshot chromium flags: %w", err)
458+
return nil, "", fmt.Errorf("failed to snapshot chromium flags: %w", err)
453459
}
454460
policySnapshot, err := captureOptionalFileSnapshot(policy.PolicyPath)
455461
if err != nil {
456-
return false, nil, "", fmt.Errorf("failed to snapshot chromium policy: %w", err)
462+
return nil, "", fmt.Errorf("failed to snapshot chromium policy: %w", err)
457463
}
458464

459465
transaction = &committedExtensionBatch{
@@ -477,7 +483,7 @@ func (s *ApiService) commitPreparedExtensions(ctx context.Context, batch *prepar
477483
registrations := make([]policy.ExtensionRegistration, 0, len(batch.extensions))
478484
for _, extension := range batch.extensions {
479485
if err := os.Rename(extension.stagingPath, extension.finalPath); err != nil {
480-
return false, nil, "", fmt.Errorf("commit extension directory %s: %w", extension.name, err)
486+
return nil, "", fmt.Errorf("commit extension directory %s: %w", extension.name, err)
481487
}
482488
transaction.paths = append(transaction.paths, extension.finalPath)
483489
registrations = append(registrations, policy.ExtensionRegistration{
@@ -488,15 +494,15 @@ func (s *ApiService) commitPreparedExtensions(ctx context.Context, batch *prepar
488494
}
489495

490496
if err := s.policy.AddExtensions(registrations); err != nil {
491-
return false, nil, "", fmt.Errorf("failed to update enterprise policy: %w", err)
497+
return nil, "", fmt.Errorf("failed to update enterprise policy: %w", err)
492498
}
493499

494500
var newTokens []string
495501
if len(batch.flagPaths) > 0 {
496502
newTokens = []string{fmt.Sprintf("--load-extension=%s", strings.Join(batch.flagPaths, ","))}
497503
}
498504
if _, err := s.mergeAndWriteChromiumFlags(ctx, newTokens); err != nil {
499-
return false, nil, "", err
505+
return nil, "", err
500506
}
501507

502508
committed = true
@@ -506,7 +512,7 @@ func (s *ApiService) commitPreparedExtensions(ctx context.Context, batch *prepar
506512
"chromeExtensionID", extension.chromeExtensionID,
507513
"requiresEnterprisePolicy", extension.requiresEnterprisePolicy)
508514
}
509-
return batch.requiresRestart, transaction, "", nil
515+
return transaction, "", nil
510516
}
511517

512518
func (s *ApiService) loadUnpackedExtensions(ctx context.Context, extensions []preparedExtension) error {

‎server/cmd/api/api/chromium_configure.go‎

Lines changed: 43 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -82,17 +82,36 @@ func (s *ApiService) ChromiumConfigure(ctx context.Context, request oapi.Chromiu
8282
return cfg400("no configuration fields provided"), nil
8383
}
8484

85+
mode := chromiumConfigureModeFor(st, extensionLoadStrategy)
86+
var preparedExtensions *preparedExtensionBatch
87+
if mode == chromiumConfigureModePreferCDPExtensions {
88+
var reqMsg string
89+
var err error
90+
preparedExtensions, reqMsg, err = s.prepareExtensionZipItems(ctx, st.extItems)
91+
if preparedExtensions != nil {
92+
defer preparedExtensions.cleanup()
93+
}
94+
if reqMsg != "" {
95+
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsg)), nil
96+
}
97+
if err != nil {
98+
return cfg500ConfigureStep(chromiumConfigureStepExtensions, err.Error()), nil
99+
}
100+
}
101+
85102
s.chromiumConfigMu.Lock()
86103
defer s.chromiumConfigMu.Unlock()
87104

88105
var configureResp oapi.ChromiumConfigureResponseObject
89-
switch chromiumConfigureModeFor(st, extensionLoadStrategy) {
106+
switch mode {
90107
case chromiumConfigureModeLive:
91108
configureResp = s.chromiumConfigureLive(ctx, st, spec)
92109
case chromiumConfigureModeRestart:
93-
configureResp = s.chromiumConfigureRestart(ctx, st, spec, nil)
94-
case chromiumConfigureModeCandidateCDPExtensions:
95-
configureResp = s.chromiumConfigureCandidateCDPExtensions(ctx, st, spec)
110+
configureResp = s.chromiumConfigureRestart(ctx, st, spec, func() (string, error) {
111+
return s.installExtensionZipItems(ctx, st.extItems)
112+
})
113+
case chromiumConfigureModePreferCDPExtensions:
114+
configureResp = s.chromiumConfigurePreferCDPExtensions(ctx, st, spec, preparedExtensions)
96115
default:
97116
return cfg500Configure("unhandled configure mode"), nil
98117
}
@@ -109,7 +128,7 @@ type chromiumConfigureMode uint8
109128
const (
110129
chromiumConfigureModeLive chromiumConfigureMode = iota
111130
chromiumConfigureModeRestart
112-
chromiumConfigureModeCandidateCDPExtensions
131+
chromiumConfigureModePreferCDPExtensions
113132
)
114133

115134
func chromiumConfigureExtensionLoadStrategy(params oapi.ChromiumConfigureParams) (oapi.ChromiumConfigureParamsExtensionLoadStrategy, string) {
@@ -130,7 +149,7 @@ func chromiumConfigureModeFor(st *chromiumConfigureState, strategy oapi.Chromium
130149
}
131150
if len(st.extItems) > 0 {
132151
if strategy == oapi.PreferCdp {
133-
return chromiumConfigureModeCandidateCDPExtensions
152+
return chromiumConfigureModePreferCDPExtensions
134153
}
135154
return chromiumConfigureModeRestart
136155
}
@@ -163,25 +182,18 @@ func chromiumConfigureNavigate(ctx context.Context, s *ApiService, spec startURL
163182
}
164183
}
165184

166-
func (s *ApiService) chromiumConfigureCandidateCDPExtensions(ctx context.Context, st *chromiumConfigureState, spec startURLParsed) oapi.ChromiumConfigureResponseObject {
167-
prepared, reqMsg, err := s.prepareExtensionZipItems(ctx, st.extItems)
168-
if prepared != nil {
169-
defer prepared.cleanup()
185+
func (s *ApiService) chromiumConfigurePreferCDPExtensions(ctx context.Context, st *chromiumConfigureState, spec startURLParsed, prepared *preparedExtensionBatch) oapi.ChromiumConfigureResponseObject {
186+
commitExtensions := func() (string, error) {
187+
_, reqMsg, err := s.commitPreparedExtensions(ctx, prepared)
188+
return reqMsg, err
170189
}
171-
if reqMsg != "" {
172-
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsg))
173-
}
174-
if err != nil {
175-
return cfg500ConfigureStep(chromiumConfigureStepExtensions, err.Error())
176-
}
177-
178190
if prepared.requiresRestart {
179-
return s.chromiumConfigureRestart(ctx, st, spec, prepared)
191+
return s.chromiumConfigureRestart(ctx, st, spec, commitExtensions)
180192
}
181193

182194
// Configure keeps the default restart path's non-transactional install semantics.
183195
// Rolling back after fallback verification would require a second restart.
184-
_, _, reqMsg, err = s.commitPreparedExtensions(ctx, prepared)
196+
_, reqMsg, err := s.commitPreparedExtensions(ctx, prepared)
185197
if reqMsg != "" {
186198
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, reqMsg))
187199
}
@@ -195,11 +207,9 @@ func (s *ApiService) chromiumConfigureCandidateCDPExtensions(ctx context.Context
195207
}
196208
logger.FromContext(ctx).Warn("CDP extension load failed during configure, restarting Chromium", "error", loadErr)
197209

198-
restartState := *st
199210
// Extensions are already persisted. The fallback re-launches Chromium with
200211
// the merged flags and still applies any pending display change.
201-
restartState.extItems = nil
202-
if resp := s.chromiumConfigureRestart(ctx, &restartState, spec, nil); resp != nil {
212+
if resp := s.chromiumConfigureRestart(ctx, st, spec, nil); resp != nil {
203213
return resp
204214
}
205215
if err := s.verifyUnpackedExtensions(ctx, prepared.extensions); err != nil {
@@ -208,7 +218,9 @@ func (s *ApiService) chromiumConfigureCandidateCDPExtensions(ctx context.Context
208218
return nil
209219
}
210220

211-
func (s *ApiService) chromiumConfigureRestart(ctx context.Context, st *chromiumConfigureState, spec startURLParsed, preparedExtensions *preparedExtensionBatch) (resp oapi.ChromiumConfigureResponseObject) {
221+
type chromiumConfigureExtensionInstaller func() (string, error)
222+
223+
func (s *ApiService) chromiumConfigureRestart(ctx context.Context, st *chromiumConfigureState, spec startURLParsed, installExtensions chromiumConfigureExtensionInstaller) (resp oapi.ChromiumConfigureResponseObject) {
212224
var stoppedRecordings []stoppedRecordingInfo
213225
chromiumStopped := false
214226
restartAfterStop := func() error {
@@ -251,18 +263,14 @@ func (s *ApiService) chromiumConfigureRestart(ctx context.Context, st *chromiumC
251263
return cfgResponseFromStepError(chromiumConfigureStepPolicies, err)
252264
}
253265

254-
var extensionReqMsg string
255-
var extensionErr error
256-
if preparedExtensions == nil {
257-
extensionReqMsg, extensionErr = chromiumApplyExtensions(ctx, s, st.extItems)
258-
} else {
259-
_, _, extensionReqMsg, extensionErr = s.commitPreparedExtensions(ctx, preparedExtensions)
260-
}
261-
if extensionReqMsg != "" {
262-
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, extensionReqMsg))
263-
}
264-
if extensionErr != nil {
265-
return cfg500ConfigureStep(chromiumConfigureStepExtensions, extensionErr.Error())
266+
if installExtensions != nil {
267+
extensionReqMsg, extensionErr := installExtensions()
268+
if extensionReqMsg != "" {
269+
return cfg400(fmt.Sprintf("%s: %s", chromiumConfigureStepExtensions, extensionReqMsg))
270+
}
271+
if extensionErr != nil {
272+
return cfg500ConfigureStep(chromiumConfigureStepExtensions, extensionErr.Error())
273+
}
266274
}
267275

268276
if st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "" {
@@ -880,14 +888,6 @@ func chromiumApplyPolicies(ctx context.Context, s *ApiService, overrides policy.
880888
return nil
881889
}
882890

883-
func chromiumApplyExtensions(ctx context.Context, s *ApiService, items []extensionZipItem) (string, error) {
884-
if len(items) == 0 {
885-
return "", nil
886-
}
887-
_, reqMsg, err := s.applyExtensionZipItems(ctx, items)
888-
return reqMsg, err
889-
}
890-
891891
func chromiumValidateFlags(raw *string) (*chromiumFlagsPlan, error) {
892892
if raw == nil || strings.TrimSpace(*raw) == "" {
893893
return nil, nil

‎server/cmd/api/api/chromium_configure_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,8 @@ func TestChromiumConfigureModeFor(t *testing.T) {
5050
{name: "invalid flags", state: chromiumConfigureState{chromiumFlagsJSON: stringPtr(`{bad-json`)}, strategy: oapi.PreferCdp, want: chromiumConfigureModeRestart},
5151
{name: "profile", state: chromiumConfigureState{hasProfile: true}, strategy: oapi.PreferCdp, want: chromiumConfigureModeRestart},
5252
{name: "extensions default restart", state: chromiumConfigureState{extItems: []extensionZipItem{{name: "test"}}}, strategy: oapi.Restart, want: chromiumConfigureModeRestart},
53-
{name: "extensions prefer CDP", state: chromiumConfigureState{extItems: []extensionZipItem{{name: "test"}}}, strategy: oapi.PreferCdp, want: chromiumConfigureModeCandidateCDPExtensions},
54-
{name: "display and extensions prefer CDP", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`), extItems: []extensionZipItem{{name: "test"}}}, strategy: oapi.PreferCdp, want: chromiumConfigureModeCandidateCDPExtensions},
53+
{name: "extensions prefer CDP", state: chromiumConfigureState{extItems: []extensionZipItem{{name: "test"}}}, strategy: oapi.PreferCdp, want: chromiumConfigureModePreferCDPExtensions},
54+
{name: "display and extensions prefer CDP", state: chromiumConfigureState{displayJSON: stringPtr(`{"width":1280}`), extItems: []extensionZipItem{{name: "test"}}}, strategy: oapi.PreferCdp, want: chromiumConfigureModePreferCDPExtensions},
5555
}
5656

5757
for _, tt := range tests {

0 commit comments

Comments
 (0)