Skip to content
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

Merged
merged 2 commits into from
May 24, 2024

Conversation

sergenyalcin
Copy link
Member

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 in forProvider can be skipped for fields set in initProvider.

I have:

  • Read and followed Upjet's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added 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:

	p.AddResourceConfigurator("aws_subnet", func(r *config.Resource) {
		r.LateInitializer = config.LateInitializer{
			ConditionalIgnoredFields: []string{
				"cidr_block",
			},
		}
	})

Generated LateInitialize function:

func (tr *Subnet) LateInitialize(attrs []byte) (bool, error) {
	params := &SubnetParameters_2{}
	if err := json.TFParser.Unmarshal(attrs, params); err != nil {
		return false, errors.Wrap(err, "failed to unmarshal Terraform state parameters for late-initialization")
	}
	opts := []resource.GenericLateInitializerOption{resource.WithZeroValueJSONOmitEmptyFilter(resource.CNameWildcard)}
	initParams, err := tr.GetInitParameters()
	if err != nil {
		return false, errors.Wrapf(err, "cannot get init parameters for resource '%q'", tr.GetName())
	}
	opts = append(opts, resource.WithConditionalFilter("CidrBlock", initParams))

	li := resource.NewGenericLateInitializer(opts...)
	return li.LateInitialize(&tr.Spec.ForProvider, params)
}

@ulucinar ulucinar changed the title Add a new late-init API to skip already filled field in spec.initProvider Add a new late-init configuration to skip already filled field in spec.initProvider May 22, 2024
Copy link
Collaborator

@ulucinar ulucinar left a 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
Copy link
Collaborator

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:

Suggested change
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
Copy link
Collaborator

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()
Copy link
Collaborator

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
Copy link
Collaborator

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:

Suggested change
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 {
Copy link
Collaborator

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:

Suggested change
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 {
Copy link
Collaborator

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:

Suggested change
func conditionalFilter(cName string, initProvider map[string]any) ConditionalFilter {
func initProviderFilter(cName string, desired, _ *fieldpath.Paved) ConditionalFilter {

return false
}

paved := fieldpath.Pave(initProvider)
Copy link
Collaborator

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:

Suggested change
paved := fieldpath.Pave(initProvider)

@@ -230,6 +262,16 @@ func (li *GenericLateInitializer) handleStruct(parentName string, desiredObject
continue
}

for _, f := range li.conditionalFilters {
if f(cName) {
Copy link
Collaborator

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:

Suggested change
if f(cName) {
if f(cName, desiredPaved, actualPaved) {

Comment on lines +165 to +170
for _, ignoreField := range cfg.LateInitializer.ConditionalIgnoredFields {
if ignoreField == traverser.FieldPath(f.TerraformPaths) {
cfg.LateInitializer.AddConditionalIgnoredCanonicalFields(traverser.FieldPath(f.CanonicalPaths))
}
}

Copy link
Collaborator

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.

Copy link
Collaborator

@ulucinar ulucinar left a 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.

@sergenyalcin sergenyalcin merged commit a444cc1 into crossplane:main May 24, 2024
6 checks passed
@sergenyalcin sergenyalcin deleted the cond-late-init branch May 24, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants