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.

Sign up for GitHub

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

Jump to bottom

Add a new late-init configuration to skip already filled field in spec.initProvider #407

Merged
merged 2 commits into from
May 24, 2024
Merged

Add a new late-init configuration to skip already filled field in spec.initProvider #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eadba75
Add a new late-init API to skip already filled field in spec.initProv…
sergenyalcin May 22, 2024
be0cde1
Add conditionalIgnoredCanonicalFieldPaths to IgnoreFields to fix the …
sergenyalcin May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/config/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func TestDefaultResource(t *testing.T) {
// TODO(muvaf): Find a way to compare function pointers.
ignoreUnexported := []cmp.Option{
cmpopts.IgnoreFields(Sensitive{}, "fieldPaths", "AdditionalConnectionDetailsFn"),
cmpopts.IgnoreFields(LateInitializer{}, "ignoredCanonicalFieldPaths"),
cmpopts.IgnoreFields(LateInitializer{}, "ignoredCanonicalFieldPaths", "conditionalIgnoredCanonicalFieldPaths"),
cmpopts.IgnoreFields(ExternalName{}, "SetIdentifierArgumentFn", "GetExternalNameFn", "GetIDFn"),
cmpopts.IgnoreUnexported(Resource{}),
cmpopts.IgnoreUnexported(reflect.ValueOf(identityConversion).Elem().Interface()),
Expand Down
23 changes: 23 additions & 0 deletions pkg/config/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


// 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
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.

}

// GetIgnoredCanonicalFields returns the ignoredCanonicalFields
Expand All @@ -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
Expand Down
9 changes: 9 additions & 0 deletions pkg/pipeline/templates/terraformed.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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)
Expand Down
3 changes: 2 additions & 1 deletion pkg/pipeline/terraformed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ func (tg *TerraformedGenerator) Generate(cfgs []*terraformedInput, apiVersion st
"Fields": cfg.Sensitive.GetFieldPaths(),
}
vars["LateInitializer"] = map[string]any{
"IgnoredFields": cfg.LateInitializer.GetIgnoredCanonicalFields(),
"IgnoredFields": cfg.LateInitializer.GetIgnoredCanonicalFields(),
"ConditionalIgnoredFields": cfg.LateInitializer.GetConditionalIgnoredCanonicalFields(),
}

if err := trFile.Write(filePath, vars, os.ModePerm); err != nil {
Expand Down
46 changes: 44 additions & 2 deletions pkg/resource/lateinit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
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.

return func(l *GenericLateInitializer) {
l.conditionalFilters = append(l.conditionalFilters, conditionalFilter(cName, initProvider))
}
}

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 func(cn string) bool {
if cName != cn {
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)

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
Expand Down Expand Up @@ -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) {

filtered = true
break
}
}
if filtered {
continue
}

observedStructField, _ := typeOfObservedObject.FieldByName(desiredStructField.Name)
observedFieldValue := valueOfObservedObject.FieldByName(desiredStructField.Name)
desiredKeepField := false
Expand Down
8 changes: 7 additions & 1 deletion pkg/types/field.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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
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.

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)
Expand Down
Loading