-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new late-init configuration to skip already filled field in spec.initProvider #407
Conversation
…ider Signed-off-by: Sergen Yalçın <[email protected]>
…unit tests Signed-off-by: Sergen Yalçın <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sergenyalcin. Left some suggestions to generalize the implementation to support other types of conditional filters that we can introduce on top of this implementation. There are some other types of conditional filters that we need to prevent certain categorical errors in addition to the race-condition preventing filter that we are adding in this PR.
We may also decide to accept the approach proposed in this PR and do a generalization in a follow-up PR. Let's discuss.
@@ -220,10 +220,20 @@ type LateInitializer struct { | |||
// "block_device_mappings.ebs". | |||
IgnoredFields []string | |||
|
|||
// ConditionalIgnoredFields are the field paths to be skipped during | |||
// late-initialization if they are filled in spec.initProvider. | |||
ConditionalIgnoredFields []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about having a more generic configuration option on top of which we can implement this specific functionality? The idea is we could implement late-initialization filters based on the full resource specification and the canonical path being late-initialized.
We can also decide to tackle with a more generic conditional filter implementation in a follow-up PR. If we decide to do so, maybe it's better to give a more specific name to this configuration option that better reflects the specific filter it's implementing:
ConditionalIgnoredFields []string | |
InitProviderFields []string |
The idea is there are a variety of ways we can define meaningful conditional filters. A good example is to implement a conditional filter that filters the late-initialization of fields if another mutually exclusive field is already set. We cannot cover such a filter with this implementation and hence I suggest to rename the configuration option with a more specific name to better reflect its use case.
// skipped during late-initialization if they are filled in spec.initProvider. | ||
// This is filled using the `ConditionalIgnoredFields` field which keeps | ||
// Terraform paths by converting them to Canonical paths. | ||
conditionalIgnoredCanonicalFieldPaths []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above discussion on choosing a more specific name is also relevant here.
@@ -124,6 +124,15 @@ func (tr *{{ .CRD.Kind }}) LateInitialize(attrs []byte) (bool, error) { | |||
{{ range .LateInitializer.IgnoredFields -}} | |||
opts = append(opts, resource.WithNameFilter("{{ . }}")) | |||
{{ end }} | |||
{{- if gt (len .LateInitializer.ConditionalIgnoredFields) 0 -}} | |||
initParams, err := tr.GetInitParameters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to read the init parameters here? How about passing the whole resource representation to resource.ConditionalFilter
when calling those filters from resource.handleStruct
? Please also see the comment on resource.ConditionalFilter
below.
// ConditionalFilter defines a late-initialization filter on CR field canonical names. | ||
// Fields with matching cnames will not be processed during late-initialization | ||
// if they are filled in spec.initProvider. | ||
type ConditionalFilter func(string) bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the ConditionalFilter
accept the whole representation so that we can introduce different conditional filters using it:
type ConditionalFilter func(string) bool | |
type ConditionalFilter func(cName string, source, target *fieldpath.Paved) bool |
Here source
and target
are the Paved
representations of the source and the target objects (in the current jargon of the late-initialization library, it's the "actual" and "desired" objects, respectively). This would also allow us to get rid of reading the init parameters in the Terraformed
resource's generated LateInitialize
function (please see the respective comment above).
// WithConditionalFilter returns a GenericLateInitializer that causes to | ||
// skip initialization of the field with the specified canonical name | ||
// if the field is filled in spec.initProvider. | ||
func WithConditionalFilter(cName string, initProvider map[string]any) GenericLateInitializerOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we implement what's suggested above, we would not need the init provider map here:
func WithConditionalFilter(cName string, initProvider map[string]any) GenericLateInitializerOption { | |
func WithConditionalFilter(cName string) GenericLateInitializerOption { |
as it would already be available in the desired object passed to the conditional filter.
} | ||
} | ||
|
||
func conditionalFilter(cName string, initProvider map[string]any) ConditionalFilter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a specific conditional filter implementation for the very specific task of skipping a late-initialization of the spec.forProvider
field iff the corresponding spec.initProvider
field is already set. So, something like:
func conditionalFilter(cName string, initProvider map[string]any) ConditionalFilter { | |
func initProviderFilter(cName string, desired, _ *fieldpath.Paved) ConditionalFilter { |
return false | ||
} | ||
|
||
paved := fieldpath.Pave(initProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*fieldpath.Paved
would be passed from resource.LateInitialize
as an argument so we would not need to pave here:
paved := fieldpath.Pave(initProvider) |
@@ -230,6 +262,16 @@ func (li *GenericLateInitializer) handleStruct(parentName string, desiredObject | |||
continue | |||
} | |||
|
|||
for _, f := range li.conditionalFilters { | |||
if f(cName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we follow the above suggestions, this would turn into something like:
if f(cName) { | |
if f(cName, desiredPaved, actualPaved) { |
for _, ignoreField := range cfg.LateInitializer.ConditionalIgnoredFields { | ||
if ignoreField == traverser.FieldPath(f.TerraformPaths) { | ||
cfg.LateInitializer.AddConditionalIgnoredCanonicalFields(traverser.FieldPath(f.CanonicalPaths)) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about accepting the actual CRD canonical path during the configuration? Then we would not need to do the conversion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've sync'ed offline and decided to continue with this PR as is. We will consider the generalization of the conditions for skipping the late-initialization in future iterations. We've successfully validated this PR & #406 together in the context of crossplane-contrib/provider-upjet-aws#1311.
Description of your changes
This PR adds a new late-init API to skip already filled field in
spec.initProvider
.Even though a field is specified in initProvider, it is late-init for forProvider. This can cause problems in some cases because
forProvider
is more powerful. With this new configuration API, the late-init operation of the field inforProvider
can be skipped for fields set ininitProvider
.I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Tested in provider-aws by using this configuration. The example configuration for aws ec2.Subnet resource:
Generated
LateInitialize
function: