Create options secret before bd #4284
Merged
+210
−245
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
1 task
weyfonk
force-pushed
the
create-options-secret-before-bd
branch
from
f32934c to
faf4260
Compare
0xavi0
approved these changes
Nov 7, 2025
Contributor
0xavi0
left a comment
0xavi0
left a comment
There was a problem hiding this comment.
lgtm... just a question in case I missed something
Comment on lines
+370
to
+373
| // reference from its creation or latest update. | ||
| if op == controllerutil.OperationResultCreated { | ||
| if err := r.ensureOwnerReferences(ctx, bd, optionsSecret); err != nil { | ||
| return r.computeResult(ctx, logger, bundleOrig, bundle, "failed to ensure owner references are set in options secret", err) |
Contributor
There was a problem hiding this comment.
Question: What’s the advantage of setting the owner reference when calling manageOptionsSecret? That we skip this call in here when updating?
Contributor
Author
There was a problem hiding this comment.
Yes, that's the idea:
// If the bundle deployment already existed and has simply been updated, the secret will already bear an owner
// reference from its creation or latest update.
If the bundle has been updated, its owner reference should already be set by manageOptionsSecret, which created or updated the secret. This is an optimisation.
On the other hand, if the bundle was created, it did then not yet exist when manageOptionsSecret was called, hence the need to ensure the owner reference is set here.
Getting a bundle and returning a bundle with a finalizer now lives in a separate function to make test cases more concise, as that logic is called a few times.
This should clarify, from one place, where content creation and update calls are made from and why.
Both cases are now covered, with comments for better readability.
A couple of comments clarify why those expectations exist, which should make tests easier to maintain.
Based on an idea from rancher#3889 by Alejandro Ruiz, the bundle controller now creates an options secret, if such a secret is needed, before the bundle deployment which will end up owning it. This should prevent some errors about missing secrets.
Setting an owner reference pointing to a bundle deployment on its options secret can be done when creating or updating the secret, if the bundle deployment already exists. This works even if the bundle deployment is outdated, to be subsequently updated, because its UID is immutable. This could save the bundle controller a few separate secret update requests when repeatedly updating multiple bundle deployments.
weyfonk
force-pushed
the
create-options-secret-before-bd
branch
from
faf4260 to
484774e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
In order to prevent errors triggered by a missing or outdated options secret, the bundle controller now creates/updates it before creating/updating the bundle deployment owning it.
Updating bundle controller unit tests to fit with this new logic was easier after a bit of simplification, hence the additional diff.
Supersedes #3889.
Additional Information
Checklist
- [ ] I have updated the documentation via a pull request in thefleet-docs repository.