Skip to content

Commit fbcc60b

Browse files
zhaohuabingdavem-git
authored andcommitted
Fix flaky wasm permission cache tests (envoyproxy#6085)
* Revert "fix flaky wasm permission check (envoyproxy#5837)" This reverts commit 645b65f. * Fix flaky wasm permission cache check Signed-off-by: Huabing (Robin) Zhao <[email protected]> --------- Signed-off-by: Huabing (Robin) Zhao <[email protected]>
1 parent 38e9146 commit fbcc60b

File tree

2 files changed

+293
-225
lines changed

2 files changed

+293
-225
lines changed

internal/wasm/premissioncache.go

Lines changed: 41 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,6 @@ type permissionCache struct {
7171

7272
cache map[string]*permissionCacheEntry
7373
logger logging.Logger
74-
75-
// For tests
76-
triggerChan chan struct{}
7774
}
7875

7976
// permissionCacheEntry is an entry in the permission cache.
@@ -128,7 +125,6 @@ func newPermissionCache(options permissionCacheOptions, logger logging.Logger) *
128125
cache: make(map[string]*permissionCacheEntry),
129126
permissionCacheOptions: options,
130127
logger: logger,
131-
triggerChan: make(chan struct{}),
132128
}
133129
}
134130

@@ -148,60 +144,55 @@ func (p *permissionCache) Start(ctx context.Context) {
148144
defer ticker.Stop()
149145
for {
150146
select {
151-
// explicitly trigger a check for tests
152-
case <-p.triggerChan:
153-
p.checkPermissions(ctx)
154147
case <-ticker.C:
155-
p.checkPermissions(ctx)
148+
func() {
149+
p.Lock()
150+
defer p.Unlock()
151+
for _, e := range p.cache {
152+
if e.isCacheExpired(p.cacheExpiry) {
153+
p.logger.Info("removing permission cache entry", "image", e.image.String())
154+
delete(p.cache, e.key())
155+
continue
156+
}
157+
if e.isPermissionExpired(p.permissionExpiry) {
158+
const retryAttempts = 3
159+
const retryDelay = 1 * time.Second
160+
p.logger.Info("rechecking permission for image", "image", e.image.String())
161+
err := retry.Do(
162+
func() error {
163+
err := p.checkAndUpdatePermission(ctx, e)
164+
if err != nil && isRetriableError(err) {
165+
p.logger.Error(
166+
err,
167+
"failed to check permission for image, will retry again",
168+
"image",
169+
e.image.String())
170+
return err
171+
}
172+
return nil
173+
},
174+
retry.Attempts(retryAttempts),
175+
retry.DelayType(retry.BackOffDelay),
176+
retry.Delay(retryDelay),
177+
retry.Context(ctx),
178+
)
179+
if err != nil {
180+
p.logger.Error(
181+
err,
182+
fmt.Sprintf("failed to recheck permission for image after %d attempts", retryAttempts),
183+
"image",
184+
e.image.String())
185+
}
186+
}
187+
}
188+
}()
156189
case <-ctx.Done():
157190
return
158191
}
159192
}
160193
}()
161194
}
162195

163-
func (p *permissionCache) checkPermissions(ctx context.Context) {
164-
p.Lock()
165-
defer p.Unlock()
166-
for _, e := range p.cache {
167-
if e.isCacheExpired(p.cacheExpiry) {
168-
p.logger.Info("removing permission cache entry", "image", e.image.String())
169-
delete(p.cache, e.key())
170-
continue
171-
}
172-
if e.isPermissionExpired(p.permissionExpiry) {
173-
const retryAttempts = 3
174-
const retryDelay = 1 * time.Second
175-
p.logger.Info("rechecking permission for image", "image", e.image.String())
176-
err := retry.Do(
177-
func() error {
178-
err := p.checkAndUpdatePermission(ctx, e)
179-
if err != nil && isRetriableError(err) {
180-
p.logger.Error(
181-
err,
182-
"failed to check permission for image, will retry again",
183-
"image",
184-
e.image.String())
185-
return err
186-
}
187-
return nil
188-
},
189-
retry.Attempts(retryAttempts),
190-
retry.DelayType(retry.BackOffDelay),
191-
retry.Delay(retryDelay),
192-
retry.Context(ctx),
193-
)
194-
if err != nil {
195-
p.logger.Error(
196-
err,
197-
fmt.Sprintf("failed to recheck permission for image after %d attempts", retryAttempts),
198-
"image",
199-
e.image.String())
200-
}
201-
}
202-
}
203-
}
204-
205196
// isRetriableError checks if the error is retriable.
206197
// If the error is a permission error, it's not retriable. For example, 401 and 403 HTTP status code.
207198
func isRetriableError(err error) bool {

0 commit comments

Comments
 (0)