Resolve references on every reconcile #66
Merged
Conversation
This file contains 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
|
|
4 tasks
|
|
This commit refactors ResolveReferencers to allow the code that finds types within a struct that satisfy AttributeReferencer to be swapped out. It also updates the default AttributeReferencerFinder to avoid checking struct tags. Previously errors were returned when: 1. A struct field tagged as a referencer did not satisfy AttributeReferencer 2. A struct field not tagged as a referencer satisfied AttributeReferencer If either of these scenarios occurred, ResolveReferences would panic with the returned error the first time it encountered an incorrectly written API type. My feeling is that both of these conditions are testing for programmer errors that would be better caught at build time than at runtime. Signed-off-by: Nic Cope <negz@rk0n.org>
Signed-off-by: Nic Cope <negz@rk0n.org>
Signed-off-by: Nic Cope <negz@rk0n.org>
the established convention is for the managed resource reconciler to requeue after a short wait (typically 30 seconds) when it knows it is waiting for an operation. Signed-off-by: Nic Cope <negz@rk0n.org>
The mock binding status was identical to the real one, while the mock conditioned status set only the most recent condition, leading to a few slightly broken managed resource reconciler tests. Signed-off-by: Nic Cope <negz@rk0n.org>
negz
changed the title
|
|
hasheddan
reviewed
Oct 31, 2019
Reference resolution is now a no-op if nothing changes, so we run it on every reconcile. We also run it after delete has been handled, so unresolved references will only block creates and updates. This commit means we'll make more get calls to the cache (or API) in order to resolve our references each reconcile, and also risk potentially changing the values of 'immutable' fields automatically if and when our references resources change. I believe we should address this by having referencers be no-ops when the field value they would set is already set. I attempted to move reference resolution to right before we call create or update (i.e. after observe and delete), but it turns out certain resources (specifically GCP Connections) could need references to be resolved in order to observe the external resource. Signed-off-by: Nic Cope <negz@rk0n.org>
We no longer need CanReference types to satisfy the metav1.Object interface. It was used only to determine the namespace of the referencing object before all such objects became cluster scoped. Signed-off-by: Nic Cope <negz@rk0n.org>
|
|
hasheddan
approved these changes
Oct 31, 2019
There was a problem hiding this comment.
This LGTM and it appears that though the controllers will not have to change for existing managed reconcilers unless they want to implement a custom Finder! That's pretty cool! We should think about how we want to notify stack owners on the next release of crossplane-runtime that their Assign() methods may need updating.
Agreed. Now that we have semantic versioning for crossplane-runtime I think we could introduce this as 0.1.1 but we'll want to include this detail in the release notes. I'll add a label to this PR in the hopes that will help us remember it at release time. |
negz
added
the
release/behaviour-change
This was referenced Nov 4, 2019
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.
Description of your changes
Fixes #53
This PR updates the managed resource reconciler to resolve references on every reconcile, not just the first one. The default reference resolver implementation is updated to avoid updating the managed resource if it doesn't change during the reference resolution process.
Note that this subtly changes the behaviour stacks will see when using crossplane-runtime if this change is merged; they will need to ensure their reference value assignment functions are idempotent. Currently the assignment functions that handle a slice of values append the resolved value, even if it's already set, thus growing the slice of values with every reconcile.
The PR also updates the attribute referencer finder to avoid checking struct tags. Previously errors were returned when:
AttributeReferencerAttributeReferencerIf either of these scenarios occurred, ResolveReferences would panic with the returned error the first time it encountered an incorrectly written API type. I can see the value in using struct tags to document the intent that a type satisfies the
AttributeReferencerinterface, but my feeling is that both of these conditions are testing for programmer errors that would be better caught at build time than at runtime.Checklist
I have:
make reviewableto ensure this PR is ready for review.clusterrole.yamlto include any new types.