fix(tenant): always write an explicit S3 Object Lock retention ceiling - #155
Merged
yehlo merged 1 commit intoAug 29, 2026
Merged
Conversation
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
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.
Problem
Every tenant the operator created ended up with
maxRetentionYears: 100in the backend instead of a ceiling the operator controls. Verified against the dev grid — the pattern was unmistakable:maxRetentionDaysmaxRetentionYearsnull10090null11StorageGrid applies a 100 year ceiling to any tenant whose request body carries no retention field at all.
Three defects combined:
The field was omitted rather than set.
spec.s3ObjectLockis an optional pointer, so when absent the CRD defaults never materialise.desiredMaxRetentionDaysreturnednilfor bothspec == nilandmode == DisabledDrift detection was blind to
maxRetentionYears.GetConfiguredMaxRetentionDaysonly readMaxRetentionDays;MaxRetentionYearsappeared nowhere in the repo. A tenant atyears: 100comparednil == niland logged "already in sync" forever.When drift was detected, the write was a silent no-op that looped forever. For two adopted tenants with a hand-set
maxRetentionDays: 90, the operator issued a full PUT on every reconcile, loggedSuccessfully updated S3 Object Lock policy, and the grid still read90. StorageGrid refuses to leave a tenant with no ceiling at all, so the empty PUT changed nothing.Worth naming:
maxRetentionDayscaps Governance retention too —allowComplianceModeis a separate gate. A tenant admin can create a Governance object-lock bucket straight through the S3 API, bypassing theS3BucketCR mode gate, and the backend ceiling is the only backstop. That backstop was 100 years on every managed tenant.Change
The operator now always resolves and writes a concrete ceiling.
StorageGrid.spec.defaultMaxRetentionInDays— new grid-wide default,365(matching what the Tenant Manager writes), bounds 1–36500. Mirrored into status for visibility.EffectiveMaxRetentionInDaysininternal/controller— tenant value → grid default → 365. Never returns zero. Deliberately independent ofmode, since the ceiling binds Governance retention the bucket-level gate never sees.spec.s3ObjectLock.maxRetentionInDays(or0= inherit) overrides it.pkg/grid—CreateTenantandUpdateTenantObjectLockPolicytake a plainintinstead of*int, so a ceiling can no longer silently vanish throughomitempty.MaxRetentionYearsis reset to nil before the PUT: a freshly fetched tenant carries100, which is non-nil and would otherwise be written back alongside the days value.maxRetentionYears, which is what actually ends the infinite PUT loop.maxLoweredguard no longer skips when the mode isDisabled.No SDK change required: StorageGrid stores the ceiling as either days or years and writing one clears the other, so sending a concrete
maxRetentionDaysis sufficient on both paths.Verification
Deployed to the dev grid. All seven managed tenants went
days=null, years=100→days=365, years=nullin a single reconcile burst, including the two that were stuck in the PUT loop. The loop is gone.Backend semantics were confirmed directly against the grid API rather than assumed:
POSTwith no retention keys →days=null, years=100(reproduces the bug)POSTwithmaxRetentionDays: 365,maxRetentionYearsomitted →days=365, years=null(setting days clears years)Also added:
internal/controller/objectlock_retention_test.go)pkg/gridtests covering the years accessor, the clear-before-PUT, and theomitemptycontract that forces callers to resolve a concrete valuetest/e2e/chainsaw/s3tenant/objectlock-retention/— there was no tenant-level object-lock e2e test beforemake testandmake lintpass.Note for rollout
defaultMaxRetentionInDaysdefaults to 365, so the two tenants that carried a hand-set 90-day ceiling (x7g-loki-s3tenant-deployment-legacy,syx2-kubernetes-itop-sync) are now at 365. If 90 was deliberate, setspec.s3ObjectLock.maxRetentionInDays: 90on them and the next reconcile restores it.🤖 Generated with Claude Code