feat(controller): added nameOverride to buckets (#111)

[Junjus]

Adds a new field to bucket CRD, that allows users to create buckets where the global alias of the bucket is equal to the nameOverride.

Implements #111

[bmarinov]

Hey thanks for the PR! I will look into it later this week, I think that this will make the cut for the next release.

Can you revert the change to the Helm chart? The change to the chart dir requires a new helm release version to be set. The release process is in two stages, there was no way for you to know that.

Release flows (for context):

• app: commit merged on main -> tag v x.y.z -> app release triggered for v x.y.z;
• chart: reference new app version (x.y.z) -> bump chart version -> sync CRDs

The Helm release is only possible once the app has released.

[codecov]

Codecov Report

❌ Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/controller/bucket_controller.go	77.41%	5 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[bmarinov]

Thanks for the change and a quick update with the current plan! While I think that this PR is ready for merging, there are a few changes I need to merge and release first (asap). These include bucket & key events, as well as fixes to the object status in the case of reconciliation failure. There is a gap in the status model and important context is missing.

These were supposed to release as v0.5 even before your bug report from a few weeks back. So that increment is a bit behind schedule already.

My current plan (PR queue):

• merge the high-prio improvement (fix?): feat(controller): Emit Warning events when namespace RBAC denies access #118
• merge a small fix: fix(accesskey): Persist status conditions when reconcile fails #120
• small wording fix in status conditions: feat(controller): report specific rbac reason in error conditions #121
• this PR

I think that your branch should rebase cleanly on top of the change from the events PR I linked above.

As for the next release: my current plan is to tag 0.5 with no CRD changes, and then immediately release your PR + this tiny bucket CRD change together as a 0.6 preview.
That way im not delaying 0.5 further, and CRD changes are better batched together.

ETA for both is by the end of the week :)

[bmarinov]

I went through the PR and it is in a good shape. Thanks for taking the time to prepare tests :D The CRD validation also looks good, maybe it can be tightened a bit but nothing which cannot wait.
There might be a gap in the new reconcile run, I've left a comment.

Also since you experienced some issues around the make targets I cleaned them up a bit and added docs. Make manifests should be safe now.

[Junjus]

Thanks! I may be missing it, but I cannot find the review comment. The review comments panel currently says “No comments yet.” Could it still be part of a pending review?

[bmarinov]

Question on this BucketID != "" check in general (not only in this location): here the empty BucketID is used as a signal that the bucket does not exist on Garage. So check -> bucket missing externally -> create a new one on line 337. Then we store the ID in the status field.
What will be the result if the controller fails to persist this ID? For example, take a look what happens on conflict - on line 125. Conflicts here are routine rather than an edge case (a second reconcile is in flight). The patch can fail for other reasons too, or the controller process may get terminated.

Unless I am missing something, the next reconcile run sees BucketID == "" and gets into the branch at line 348. The nameOverride field is correctly immutable, but the user is now stuck with an impossible to reconcile bucket.

The difiference with resolveNewBucket is that the unique UID suffix ties a Garage bucket to the Bucket API resource we are reconciling. This guarantees ownership and readopting is safe. The same is not true for this new branch.

How would you approach it? This is otherwise sound, just the failure paths need to be more robust.

[bmarinov]

Thanks! I may be missing it, but I cannot find the review comment. The review comments panel currently says “No comments yet.” Could it still be part of a pending review?

GitHub's horrible UX strikes again. Comment vs review comment, I had to "finish" the review so the comment appears. 🤦

[Junjus]

Yes, I agree. I probably would have encountered the same issue as well.

Regarding your review comment:

I am also concerned about this failure path, and I agree that the current implementation is not robust enough. I initially thought it might be sufficient to document in the CRD that this failure can occur. However, I was not aware that status update conflicts occur so frequently.

The fundamental issue here is a dual-write problem:

1. Create the bucket in Garage.
2. Persist its ID and binding in the Kubernetes object's status.

If the first operation succeeds and the second fails, the next reconciliation cannot distinguish between these two cases:

• The controller created this exact-name bucket during the previous reconciliation.
• An unrelated tenant already owns a bucket with that exact name.

As you mentioned, the UID-derived suffix solves this for the existing path because the external bucket name itself carries a binding to the Kubernetes object. A user-selected nameOverride, however, does not provide this property.

I do not think retrying the status patch alone can solve this because the controller may also terminate between the two writes. We need either a durable bucket reservation or ownership information that exists independently of the post-creation status update.

Possible solutions:

As mentioned before, we could use per-key local aliases to make buckets appear to have exact names. We could then continue using UID-derived suffixes for the underlying bucket names. But this isn't the same thing as a global nameOverride.

Another option might be to use the controller's master key as a durable metadata store. Specifically, a bucket could be created with an exact global alias and a "tag" such as garage-storage-controller-<object-uid>, implemented as a local alias on the master key.

Since the Garage API creates the bucket and this ownership alias consistently, the operation would either create both or fail without creating either. This would allow the controller to distinguish between:

• A bucket created by the controller for a particular Bucket resource during a previous reconciliation attempt.
• A bucket created by another tenant's Bucket resource.
• A bucket created externally.

This could provide the durable ownership information needed to recover safely when persisting the Kubernetes status fails.

[bmarinov]

@Junjus this is close to what I was considering.

In general, I see two ways we can approach this, both similar to yours in a way.

(1) Store the intent to create a new bucket in a new status field.

The order of operations:

• Check whether exactName exists
• Persist the intention to create in a new status field
• Patch the object
• Create on Garage
• Update status

Now if the reconcile gets interrupted, we have the intention to create (status) + the existing bucket. It is reasonably safe to assume that the discovered Garage bucket and the one ordered through the API resource are the same.
Assumptions:

• Leader election + Max concurrent reconciles = 1;
• Not a guarantee but reasonably safe with current settings.

(2) Use an additional global alias, derived by the resource UID (the default naming convention)

Basically we are giving the bucket two names: the ordered one, and the default / canonical one (desired + UID hash):

• Generate UID-suffixed bucket name (default naming convention bucket-<hash>)
• Check for bucket-<hash>
• Check for bucket with name == nameOverride
• If Found and aliases match?
  • Yes = thats our bucket
  • No = conflict, cannot reconcile
• If Not Found:
  • create bucket with default naming convention
  • add desired `nameOverride

The nice thing is that we are basically riding on top of the default naming guarantees. You cannot forge resource UIDs and the hashes are reasonably collision-safe.

Option (2) is effectively the same as the per-key aliases, but without the added complexity and fragility. The admin key used by the controller can change between installations, get lost, deleted etc. No idea what happens with the per-key aliases then.

This PR is very close to option one, and this is the easier feature to merge. A few adjustments will get you there and the question is whether Option (2) is doable within a reasonable timeframe or not. The extra status field for (1) will become obsolete once (2) hits.

Option two is quite a bit more involved and not what you signed up for when you opened this PR. But it feels like the more robust solution long-term. Overkill for smaller environments though.

[Junjus]

@bmarinov Thank you again for taking the time to think this through :))

A quick heads-up: I will not be able to work on this PR for the next couple of weeks. I did not want to leave the thread unattended without mentioning that. Once I am available again, I would be happy to pursue option 2.

After thinking about it further, I agree that the additional UID-derived global alias is probably the better fit for this PR. It stays close to the controller’s existing naming and ownership model, and it reuses the guarantees already provided by the default UID-derived name.

My only reservation is that every bucket using nameOverride would then have two global aliases: the user-requested alias and the controller’s canonical UID-derived alias. That consumes additional global alias names and may be slightly surprising from an API perspective, but I do not think that should block the approach.

I still think local aliases could be useful as a more general design direction, but using them for ownership would change the controller’s operational model quite significantly. That seems better suited to a separate issue or design discussion rather than expanding the scope of this PR.