-
Notifications
You must be signed in to change notification settings - Fork 89
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
// ignoredCanonicalFieldPaths are the Canonical field paths to be skipped | ||
// during late-initialization. This is filled using the `IgnoredFields` | ||
// field which keeps Terraform paths by converting them to Canonical paths. | ||
ignoredCanonicalFieldPaths []string | ||
|
||
// conditionalIgnoredCanonicalFieldPaths are the Canonical field paths to be | ||
// 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 commentThe 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. |
||
} | ||
|
||
// GetIgnoredCanonicalFields returns the ignoredCanonicalFields | ||
|
@@ -239,6 +249,19 @@ func (l *LateInitializer) AddIgnoredCanonicalFields(cf string) { | |
l.ignoredCanonicalFieldPaths = append(l.ignoredCanonicalFieldPaths, cf) | ||
} | ||
|
||
// GetConditionalIgnoredCanonicalFields returns the conditionalIgnoredCanonicalFieldPaths | ||
func (l *LateInitializer) GetConditionalIgnoredCanonicalFields() []string { | ||
return l.conditionalIgnoredCanonicalFieldPaths | ||
} | ||
|
||
// AddConditionalIgnoredCanonicalFields sets conditional ignored canonical fields | ||
func (l *LateInitializer) AddConditionalIgnoredCanonicalFields(cf string) { | ||
if l.conditionalIgnoredCanonicalFieldPaths == nil { | ||
l.conditionalIgnoredCanonicalFieldPaths = make([]string, 0) | ||
} | ||
l.conditionalIgnoredCanonicalFieldPaths = append(l.conditionalIgnoredCanonicalFieldPaths, cf) | ||
} | ||
|
||
// GetFieldPaths returns the fieldPaths map for Sensitive | ||
func (s *Sensitive) GetFieldPaths() map[string]string { | ||
return s.fieldPaths | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 |
||
if err != nil { | ||
return false, errors.Wrapf(err, "cannot get init parameters for resource '%q'", tr.GetName()) | ||
} | ||
{{ range .LateInitializer.ConditionalIgnoredFields -}} | ||
opts = append(opts, resource.WithConditionalFilter("{{ . }}", initParams)) | ||
{{ end }} | ||
{{ end }} | ||
|
||
li := resource.NewGenericLateInitializer(opts...) | ||
return li.LateInitialize(&tr.Spec.ForProvider, params) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,12 +10,14 @@ import ( | |||||
"runtime/debug" | ||||||
"strings" | ||||||
|
||||||
"github.com/crossplane/crossplane-runtime/pkg/fieldpath" | ||||||
xpmeta "github.com/crossplane/crossplane-runtime/pkg/meta" | ||||||
xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" | ||||||
"github.com/pkg/errors" | ||||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||
|
||||||
"github.com/crossplane/upjet/pkg/config" | ||||||
"github.com/crossplane/upjet/pkg/types/name" | ||||||
) | ||||||
|
||||||
const ( | ||||||
|
@@ -43,8 +45,9 @@ const ( | |||||
|
||||||
// GenericLateInitializer performs late-initialization of a Terraformed resource. | ||||||
type GenericLateInitializer struct { | ||||||
valueFilters []ValueFilter | ||||||
nameFilters []NameFilter | ||||||
valueFilters []ValueFilter | ||||||
nameFilters []NameFilter | ||||||
conditionalFilters []ConditionalFilter | ||||||
} | ||||||
|
||||||
// SetCriticalAnnotations sets the critical annotations of the resource and reports | ||||||
|
@@ -175,6 +178,35 @@ func isZeroValueOmitted(tag string) bool { | |||||
return false | ||||||
} | ||||||
|
||||||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. We could make the
Suggested change
Here |
||||||
|
||||||
// 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 commentThe 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
as it would already be available in the desired object passed to the conditional filter. |
||||||
return func(l *GenericLateInitializer) { | ||||||
l.conditionalFilters = append(l.conditionalFilters, conditionalFilter(cName, initProvider)) | ||||||
} | ||||||
} | ||||||
|
||||||
func conditionalFilter(cName string, initProvider map[string]any) ConditionalFilter { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||
return func(cn string) bool { | ||||||
if cName != cn { | ||||||
return false | ||||||
} | ||||||
|
||||||
paved := fieldpath.Pave(initProvider) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
value, err := paved.GetValue(name.NewFromCamel(cName).Snake) | ||||||
if err != nil || value == nil { | ||||||
return false | ||||||
} | ||||||
return true | ||||||
} | ||||||
} | ||||||
|
||||||
// LateInitialize Copy unset (nil) values from responseObject to crObject | ||||||
// Both crObject and responseObject must be pointers to structs. | ||||||
// Otherwise, an error will be returned. Returns `true` if at least one field has been stored | ||||||
|
@@ -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 commentThe 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
|
||||||
filtered = true | ||||||
break | ||||||
} | ||||||
} | ||||||
if filtered { | ||||||
continue | ||||||
} | ||||||
|
||||||
observedStructField, _ := typeOfObservedObject.FieldByName(desiredStructField.Name) | ||||||
observedFieldValue := valueOfObservedObject.FieldByName(desiredStructField.Name) | ||||||
desiredKeepField := false | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,7 +104,7 @@ func getDocString(cfg *config.Resource, f *Field, tfPath []string) string { //no | |
} | ||
|
||
// NewField returns a constructed Field object. | ||
func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, snakeFieldName string, tfPath, xpPath, names []string, asBlocksMode bool) (*Field, error) { | ||
func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, snakeFieldName string, tfPath, xpPath, names []string, asBlocksMode bool) (*Field, error) { //nolint:gocyclo // easy to follow | ||
f := &Field{ | ||
Schema: sch, | ||
Name: name.NewFromSnake(snakeFieldName), | ||
|
@@ -162,6 +162,12 @@ func NewField(g *Builder, cfg *config.Resource, r *resource, sch *schema.Schema, | |
} | ||
} | ||
|
||
for _, ignoreField := range cfg.LateInitializer.ConditionalIgnoredFields { | ||
if ignoreField == traverser.FieldPath(f.TerraformPaths) { | ||
cfg.LateInitializer.AddConditionalIgnoredCanonicalFields(traverser.FieldPath(f.CanonicalPaths)) | ||
} | ||
} | ||
|
||
Comment on lines
+165
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
fieldType, initType, err := g.buildSchema(f, cfg, names, traverser.FieldPath(append(tfPath, snakeFieldName)), r) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "cannot infer type from schema of field %s", f.Name.Snake) | ||
|
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:
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.