From 91f4935429be6fe12af002b51f4084a463598354 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel=20Ducanto=20Hadad?= Date: Fri, 22 Nov 2024 14:07:56 +0100 Subject: [PATCH 1/4] fix: make regex strict by adding ^ & $, thus avoiding accidental match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miguel Ángel Ducanto Hadad --- fn.go | 5 ++++- fn_test.go | 14 +++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/fn.go b/fn.go index eae158b..518373a 100644 --- a/fn.go +++ b/fn.go @@ -63,7 +63,10 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1beta1.RunFunctionRequ continue } for _, before := range sequence[:i] { - re := regexp.MustCompile(string(before)) + // add the regex with ^ & $ to match the entire string + // possibly avoid using regex for matching literal strings + strictPattern := fmt.Sprintf("^%s$", string(before)) + re := regexp.MustCompile(strictPattern) keys := []resource.Name{} for k := range desiredComposed { if re.MatchString(string(k)) { diff --git a/fn_test.go b/fn_test.go index bfe2ffc..9c322d4 100644 --- a/fn_test.go +++ b/fn_test.go @@ -556,7 +556,7 @@ func TestRunFunction(t *testing.T) { Rules: []v1beta1.SequencingRule{ { Sequence: []resource.Name{ - "first-*", + "first-.*", "second", }, }, @@ -597,7 +597,7 @@ func TestRunFunction(t *testing.T) { Results: []*fnv1beta1.Result{ { Severity: fnv1beta1.Severity_SEVERITY_NORMAL, - Message: "Delaying creation of resource \"second\" because \"first-*\" is not fully ready (2 of 3)", + Message: "Delaying creation of resource \"second\" because \"first-.*\" is not fully ready (2 of 3)", }, }, Desired: &fnv1beta1.State{ @@ -629,8 +629,8 @@ func TestRunFunction(t *testing.T) { Rules: []v1beta1.SequencingRule{ { Sequence: []resource.Name{ - "first-*", - "second-*", + "first-.*", + "second-.*", "third", }, }, @@ -679,7 +679,7 @@ func TestRunFunction(t *testing.T) { Results: []*fnv1beta1.Result{ { Severity: fnv1beta1.Severity_SEVERITY_NORMAL, - Message: "Delaying creation of resource \"third\" because \"second-*\" is not fully ready (1 of 2)", + Message: "Delaying creation of resource \"third\" because \"second-.*\" is not fully ready (1 of 2)", }, }, Desired: &fnv1beta1.State{ @@ -720,7 +720,7 @@ func TestRunFunction(t *testing.T) { { Sequence: []resource.Name{ "first", - "second-*", + "second-.*", "third", }, }, @@ -761,7 +761,7 @@ func TestRunFunction(t *testing.T) { Results: []*fnv1beta1.Result{ { Severity: fnv1beta1.Severity_SEVERITY_NORMAL, - Message: "Delaying creation of resource \"third\" because \"second-*\" is not fully ready (1 of 2)", + Message: "Delaying creation of resource \"third\" because \"second-.*\" is not fully ready (1 of 2)", }, }, Desired: &fnv1beta1.State{ From 09cf0d60abd918d7f6411fa564e36142d8523321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel=20Ducanto=20Hadad?= Date: Fri, 22 Nov 2024 14:10:48 +0100 Subject: [PATCH 2/4] docs: correct regexp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miguel Ángel Ducanto Hadad --- README.md | 4 ++-- example/composition-regex.yaml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index d18485d..59bd448 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ See `example/composition.yaml` for a complete example. It also allows you to provide a regex to match and capture a whole group of resources and wait for them to be ready. -For example, the pipeline step below, will ensure that `second-resource` is not created until all `first-subresource-*` objects are ready. +For example, the pipeline step below, will ensure that `second-resource` is not created until all `first-subresource-.*` objects are ready. ```yaml - step: sequence-creation @@ -38,7 +38,7 @@ For example, the pipeline step below, will ensure that `second-resource` is not kind: Input rules: - sequence: - - first-subresource-* + - first-subresource-.* - second-resource ``` diff --git a/example/composition-regex.yaml b/example/composition-regex.yaml index 52550a9..1257c03 100644 --- a/example/composition-regex.yaml +++ b/example/composition-regex.yaml @@ -78,5 +78,5 @@ spec: kind: Input rules: - sequence: - - first-subresource-* + - first-subresource-.* - second-resource From 5765b24c249621dee682a520688fa0dcf4914af3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel=20Ducanto=20Hadad?= Date: Fri, 22 Nov 2024 14:30:29 +0100 Subject: [PATCH 3/4] fix: make regex strict only if not explicitly open-ended MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miguel Ángel Ducanto Hadad --- README.md | 9 ++++ example/composition-regex.yaml | 22 ++++++++-- fn.go | 18 ++++++-- fn_test.go | 80 ++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 59bd448..fea30fe 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,15 @@ For example, the pipeline step below, will ensure that `second-resource` is not - second-resource ``` +You can write the regex as strict as you want, but keep in mind that it defaults to strict matching (start and end are enforced). +In other words, the following rules apply: +```yaml +- resource # this has no explicit start or end, so it will match EXACTLY ^resource$ (normal behaviour) +- a-group-.* # this has no explicit start or end, so it will match EXACTLY ^a-group-.*$ +- ^a-group # this has an explicit start, so it will match EVERYTHING that starts with a-group +- a-group$ # this has an explicit end, so it will match EVERYTHING that ends with a-group +``` + See `example/composition-regex.yaml` for a complete example. ## Installation diff --git a/example/composition-regex.yaml b/example/composition-regex.yaml index 1257c03..7a588d2 100644 --- a/example/composition-regex.yaml +++ b/example/composition-regex.yaml @@ -52,12 +52,27 @@ spec: - time: 10s conditionType: Ready conditionStatus: "True" - - name: second-resource + - name: second-object base: apiVersion: nop.crossplane.io/v1alpha1 kind: NopResource metadata: - name: second-resource + name: second-object + spec: + forProvider: + conditionAfter: + - time: 5s + conditionType: Ready + conditionStatus: "False" + - time: 10s + conditionType: Ready + conditionStatus: "True" + - name: third-resource + base: + apiVersion: nop.crossplane.io/v1alpha1 + kind: NopResource + metadata: + name: third-resource spec: forProvider: conditionAfter: @@ -79,4 +94,5 @@ spec: rules: - sequence: - first-subresource-.* - - second-resource + - object$ # this will match everything that ends with "object" + - third-resource diff --git a/fn.go b/fn.go index 518373a..400edc4 100644 --- a/fn.go +++ b/fn.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "regexp" + "strings" "github.com/crossplane/crossplane-runtime/pkg/errors" "github.com/crossplane/crossplane-runtime/pkg/logging" @@ -21,6 +22,13 @@ type Function struct { log logging.Logger } +const ( + // START marks the start of a regex pattern. + START = "^" + // END marks the end of a regex pattern. + END = "$" +) + // RunFunction runs the Function. func (f *Function) RunFunction(_ context.Context, req *fnv1beta1.RunFunctionRequest) (*fnv1beta1.RunFunctionResponse, error) { //nolint:gocyclo // This function is unavoidably complex. f.log.Info("Running function", "tag", req.GetMeta().GetTag()) @@ -63,9 +71,13 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1beta1.RunFunctionRequ continue } for _, before := range sequence[:i] { - // add the regex with ^ & $ to match the entire string - // possibly avoid using regex for matching literal strings - strictPattern := fmt.Sprintf("^%s$", string(before)) + strictPattern := string(before) + if !strings.HasPrefix(strictPattern, START) && !strings.HasSuffix(strictPattern, END) { + // if the user provides a delimited regex, we'll use it as is + // if not, add the regex with ^ & $ to match the entire string + // possibly avoid using regex for matching literal strings + strictPattern = fmt.Sprintf("%s%s%s", START, string(before), END) + } re := regexp.MustCompile(strictPattern) keys := []resource.Name{} for k := range desiredComposed { diff --git a/fn_test.go b/fn_test.go index 9c322d4..00515df 100644 --- a/fn_test.go +++ b/fn_test.go @@ -785,6 +785,86 @@ func TestRunFunction(t *testing.T) { }, }, }, + "SequenceRegexAlreadyPrefixed": { + reason: "The function should delay the creation of second and fourth resources because the first and third are not ready", + args: args{ + req: &fnv1beta1.RunFunctionRequest{ + Input: resource.MustStructObject(&v1beta1.Input{ + Rules: []v1beta1.SequencingRule{ + { + Sequence: []resource.Name{ + "^first-.*$", + "^second-.*", + "third-.*$", + "fourth", + }, + }, + }, + }), + Observed: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{}, + }, + Desired: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{ + "first-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "first-1": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "second-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "third-0": { + Resource: resource.MustStructJSON(mr), + }, + }, + }, + }, + }, + want: want{ + rsp: &fnv1beta1.RunFunctionResponse{ + Meta: &fnv1beta1.ResponseMeta{Ttl: durationpb.New(response.DefaultTTL)}, + Results: []*fnv1beta1.Result{ + { + Severity: fnv1beta1.Severity_SEVERITY_NORMAL, + Message: "Delaying creation of resource \"fourth\" because \"third-.*$\" is not fully ready (0 of 1)", + }, + }, + Desired: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{ + "first-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "first-1": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "second-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "third-0": { + Resource: resource.MustStructJSON(mr), + }, + }, + }, + }, + }, + }, } for name, tc := range cases { From 7cadaa22ac35f9d86dbcd923144e9625964bc481 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel=20Ducanto=20Hadad?= Date: Mon, 25 Nov 2024 09:41:44 +0100 Subject: [PATCH 4/4] fix: return better error if regexp is invalid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miguel Ángel Ducanto Hadad --- fn.go | 6 ++++- fn_test.go | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/fn.go b/fn.go index 400edc4..68ff21b 100644 --- a/fn.go +++ b/fn.go @@ -78,7 +78,11 @@ func (f *Function) RunFunction(_ context.Context, req *fnv1beta1.RunFunctionRequ // possibly avoid using regex for matching literal strings strictPattern = fmt.Sprintf("%s%s%s", START, string(before), END) } - re := regexp.MustCompile(strictPattern) + re, err := regexp.Compile(strictPattern) + if err != nil { + response.Fatal(rsp, errors.Wrapf(err, "cannot compile regex %s", strictPattern)) + return rsp, nil + } keys := []resource.Name{} for k := range desiredComposed { if re.MatchString(string(k)) { diff --git a/fn_test.go b/fn_test.go index 00515df..32f97a5 100644 --- a/fn_test.go +++ b/fn_test.go @@ -786,7 +786,7 @@ func TestRunFunction(t *testing.T) { }, }, "SequenceRegexAlreadyPrefixed": { - reason: "The function should delay the creation of second and fourth resources because the first and third are not ready", + reason: "The function should not modify the sequence regex, since it's already prefixed", args: args{ req: &fnv1beta1.RunFunctionRequest{ Input: resource.MustStructObject(&v1beta1.Input{ @@ -865,6 +865,78 @@ func TestRunFunction(t *testing.T) { }, }, }, + "SequenceRegexInvalidRegex": { + reason: "The function should return a fatal error because the regex is invalid", + args: args{ + req: &fnv1beta1.RunFunctionRequest{ + Input: resource.MustStructObject(&v1beta1.Input{ + Rules: []v1beta1.SequencingRule{ + { + Sequence: []resource.Name{ + `^(`, + "second", + }, + }, + }, + }), + Observed: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{}, + }, + Desired: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{ + "first-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "first-1": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "second": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + }, + }, + }, + }, + want: want{ + rsp: &fnv1beta1.RunFunctionResponse{ + Meta: &fnv1beta1.ResponseMeta{Ttl: durationpb.New(response.DefaultTTL)}, + Results: []*fnv1beta1.Result{ + { + Severity: fnv1beta1.Severity_SEVERITY_FATAL, + Message: "cannot compile regex ^(: error parsing regexp: missing closing ): `^(`", + }, + }, + Desired: &fnv1beta1.State{ + Composite: &fnv1beta1.Resource{ + Resource: resource.MustStructJSON(xr), + }, + Resources: map[string]*fnv1beta1.Resource{ + "first-0": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "first-1": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + "second": { + Resource: resource.MustStructJSON(mr), + Ready: fnv1beta1.Ready_READY_TRUE, + }, + }, + }, + }, + }, + }, } for name, tc := range cases {