feat(sourceNamespace): Regex Support (#19016) #19017

ArthurVardevanyan · 2024-07-10T17:06:28Z

Here is a PR with my attempt at solving the below scenario: #19016

It seems to work, however I may not have accounted for everything..

Checklist:

bunnyshell · 2024-07-10T17:06:33Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

bunnyshell · 2024-07-10T17:06:33Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

jannfis

Hey @ArthurVardevanyan, thank you for trying to tackle this!

Instead of introducing a new command line option, WDYT of having a negation pattern (i.e. !) in the namespace list?

So, to specify all but a few namespaces, one could use:

--application-namespaces *,!foo,!bar

codecov · 2024-07-10T17:59:55Z

Codecov Report

Attention: Patch coverage is 72.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 55.82%. Comparing base (539a3e3) to head (707af58).
Report is 514 commits behind head on master.

Files with missing lines	Patch %	Lines
util/regex/regex.go	40.00%	4 Missing and 2 partials ⚠️
notification_controller/controller/controller.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #19017      +/-   ##
==========================================
- Coverage   55.84%   55.82%   -0.03%     
==========================================
  Files         315      316       +1     
  Lines       43654    43668      +14     
==========================================
- Hits        24377    24376       -1     
- Misses      16722    16731       +9     
- Partials     2555     2561       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ArthurVardevanyan · 2024-07-10T21:08:06Z

Hey @ArthurVardevanyan, thank you for trying to tackle this!

Instead of introducing a new command line option, WDYT of having a negation pattern (i.e. !) in the namespace list?

So, to specify all but a few namespaces, one could use:

--application-namespaces *,!foo,!bar

I agree, updated pr.

additionally, looks like the underlying glob library has a bug, otherwise this would have worked from day 1.
gobwas/glob#47

I wrote two solutions, this one, kinda shoehorns in the logic needed to make the negate work.

This is a regex re-implement:
ArthurVardevanyan@f8b431c
This would break people using globs though, as they no longer would match as expected.

jannfis · 2024-07-10T23:41:10Z

What would your thought be on just swapping it out (At least for the sourceNamespace portion), to using a proper regex library?

While I think it might be a good idea, you already mentioned that the change will be a breaking. I think the added value of using regexp is not sufficient to justify a breaking change. It might even have some weird side effects for people unaware of that change taking place, when their globs are now handled as regexps and could potentially introduce security issues or production outages in those environments. We can definitely keep that in mind for the next major version, though. If you'd like, you can go ahead and create an enhancement issue which I'll pin to the 3.0 milestone then.

Just snooping at what the changes in this PR look like, I think we're on a pretty good way now. I'll give it a thorough review tomorrow.

ArthurVardevanyan · 2024-07-11T00:08:06Z

What would your thought be on just swapping it out (At least for the sourceNamespace portion), to using a proper regex library?

While I think it might be a good idea, you already mentioned that the change will be a breaking. I think the added value of using regexp is not sufficient to justify a breaking change. It might even have some weird side effects for people unaware of that change taking place, when their globs are now handled as regexps and could potentially introduce security issues or production outages in those environments. We can definitely keep that in mind for the next major version, though. If you'd like, you can go ahead and create an enhancement issue which I'll pin to the 3.0 milestone then.

Just snooping at what the changes in this PR look like, I think we're on a pretty good way now. I'll give it a thorough review tomorrow.

I think I am content what the PR as it sits, I may add a few more tests for more scenarios, since technically this is not a "pure" glob implementation, want to make sure strange edge cases are accounted for.

However, it is not done like how the suggestion was:
--application-namespaces *,!foo,!bar (the test file has a multi-namespace negate example)
Since how the logic was written originally, it is, match if any are true, I didn't want to deviate to much from that.

So I wrote it as, anything with a negate out front, just inverts the output from the glob library for that line item.

jannfis

This test case I added fails:

{
	"allow anything prefixed test, but not test-test",
	"test-test",
	"argocd",
	[]string{"!test-test", "test-*"},
	false,
},

I was thinking that maybe we can have a function that takes a list of patterns, maybe with a similar signature to: MatchList(patterns []string, text string, separators ...rune).

It will then build two lists, one having the positive globs, the second having the negative globs. Then it will:

Match in the negative list. If there's a match, deny the namespace.
Match in the positive list. If there's a match, allow the namespace. If there's no match, deny the namespace.

This would have the benefit that patterns do not need to be ordered (e.g. negatives first, positives last) and be a little more predictable.

jannfis · 2024-07-11T19:51:40Z

util/glob/glob.go

 func Match(pattern, text string, separators ...rune) bool {
-	compiledGlob, err := glob.Compile(pattern, separators...)
-	if err != nil {
-		log.Warnf("failed to compile pattern %s due to error %v", pattern, err)
-		return false
+	if strings.HasPrefix(pattern, "!") {
+		compiledGlob, err := glob.Compile(pattern[1:], separators...)
+		if err != nil {
+			log.Warnf("failed to compile pattern %s due to error %v", pattern, err)
+			return false
+		}
+		return !compiledGlob.Match(text)
+	} else {
+		compiledGlob, err := glob.Compile(pattern, separators...)
+		if err != nil {
+			log.Warnf("failed to compile pattern %s due to error %v", pattern, err)
+			return false
+		}
+		return compiledGlob.Match(text)
 	}
-	return compiledGlob.Match(text)
 }


I think this change has too much scope, since other pieces in the code base will use this function too. This could potentially lead to undesired side effects.

IMO, it would make sense to add a new function to util/glob, let's say MatchWithInvert (just a proposal for the name), and adapt the pieces of the code that take care of matching source namespace globs to use that new function.

jannfis · 2024-07-11T21:20:46Z

The following is a simple, non-optimized and untested example to illustrate the idea: https://play.golang.com/p/9JxiCrJM8XC

ArthurVardevanyan · 2024-07-11T21:25:49Z

That is interesting, could try that as well,

But
What about a third option of supporting, regular strings, glob, and regex,

IE, if string starts with /, compute as regex ?

jannfis · 2024-07-11T21:32:45Z

That is interesting, could try that as well,

But What about a third option of supporting, regular strings, glob, and regex,

IE, if string starts with /, compute as regex ?

That is actually a brilliant idea IMO! This will allow for a non-breaking change too, since / is not a valid character for a namespace and shouldn't be in use in any existing (working) glob pattern.

ArthurVardevanyan · 2024-07-12T20:12:16Z

Testing with operator: ArthurVardevanyan/HomeLab@f1843ce?diff=split&w=0

ArthurVardevanyan · 2024-08-05T17:49:30Z

Updated Doc

jannfis · 2024-08-12T14:49:10Z

Sorry for the delay here, @ArthurVardevanyan and thanks for your patience.

I think it would make most sense to modify MatchStringInList to replace the exactMatch bool parameter with something that specifies the type of the match (i.e. exact, glob or regexp) and then use the appropriate matcher function accordingly for matching the tokens in the list.

MatchStringInList is used in different parts of the code base, and I'd not like to have those places change their behavior all at once.

Signed-off-by: Arthur <[email protected]>

ArthurVardevanyan · 2024-08-12T17:20:46Z

Sorry for the delay here, @ArthurVardevanyan and thanks for your patience.

I think it would make most sense to modify MatchStringInList to replace the exactMatch bool parameter with something that specifies the type of the match (i.e. exact, glob or regexp) and then use the appropriate matcher function accordingly for matching the tokens in the list.

MatchStringInList is used in different parts of the code base, and I'd not like to have those places change their behavior all at once.

Sounds good, took a stab at it.

jannfis

You're quick :) Thanks. I only have a final comment now.

jannfis · 2024-08-12T19:59:38Z

util/glob/list.go

+		if patternMatch == "regexp" && strings.HasPrefix(ll, "/") && strings.HasSuffix(ll, "/") && regex.Match(ll[1:len(ll)-1], item) {
+			return true
+		} else if (patternMatch == "regexp" || patternMatch == "glob") && Match(ll, item) {
+			return true
+		} else if patternMatch == "exact" && item == ll {
 			return true
 		}


I think the only thing left to do is to make the "exact", "glob" and "regexp" strings constants and then use them throughout the code. And then should be good to go!

Updated to use constants .

Signed-off-by: Arthur <[email protected]>

jannfis

LGTM!

Thank you and congratulations on your first contribution to Argo CD, @ArthurVardevanyan 🎉

* feat(sourceNamespace): Regex Support Signed-off-by: Arthur <[email protected]> * feat(sourceNamespace): Separate exactMatch into patternMatch Signed-off-by: Arthur <[email protected]> --------- Signed-off-by: Arthur <[email protected]>

* feat(sourceNamespace): Regex Support Signed-off-by: Arthur <[email protected]> * feat(sourceNamespace): Separate exactMatch into patternMatch Signed-off-by: Arthur <[email protected]> --------- Signed-off-by: Arthur <[email protected]> Signed-off-by: ChichiCaleb <ChichiCaleb@[email protected]>

* feat(sourceNamespace): Regex Support Signed-off-by: Arthur <[email protected]> * feat(sourceNamespace): Separate exactMatch into patternMatch Signed-off-by: Arthur <[email protected]> --------- Signed-off-by: Arthur <[email protected]> Signed-off-by: ChichiCaleb <ChichiCaleb@[email protected]> Signed-off-by: ChichiCaleb <[email protected]>

jannfis · 2024-08-23T11:58:27Z

/cherry-pick release-2.12

* feat(sourceNamespace): Regex Support Signed-off-by: Arthur <[email protected]> * feat(sourceNamespace): Separate exactMatch into patternMatch Signed-off-by: Arthur <[email protected]> --------- Signed-off-by: Arthur <[email protected]>

* feat(sourceNamespace): Regex Support * feat(sourceNamespace): Separate exactMatch into patternMatch --------- Signed-off-by: Arthur <[email protected]> Co-authored-by: Arthur Vardevanyan <[email protected]>

ArthurVardevanyan requested review from a team as code owners July 10, 2024 17:06

ArthurVardevanyan mentioned this pull request Jul 10, 2024

AnyNamespace Multi ArgoCD Instance WildCard Application Resource Monitoring Overlap #19016

Closed

3 tasks

jannfis reviewed Jul 10, 2024

View reviewed changes

ArthurVardevanyan force-pushed the argo-in-any-ns branch from 36e76eb to 201d849 Compare July 10, 2024 21:06

ArthurVardevanyan force-pushed the argo-in-any-ns branch 2 times, most recently from 73a0165 to 437e2f0 Compare July 10, 2024 21:22

ArthurVardevanyan changed the title ~~feat(sourceNamespace): Exclusion (#19016)~~ feat(sourceNamespace): Regex Support (#19016) Jul 10, 2024

ArthurVardevanyan force-pushed the argo-in-any-ns branch 6 times, most recently from f8b431c to 0432d76 Compare July 10, 2024 23:34

ArthurVardevanyan changed the title ~~feat(sourceNamespace): Regex Support (#19016)~~ feat(sourceNamespace): Glob Negate Support (#19016) Jul 10, 2024

ArthurVardevanyan force-pushed the argo-in-any-ns branch from 0432d76 to 514c48c Compare July 11, 2024 00:02

ArthurVardevanyan force-pushed the argo-in-any-ns branch from 514c48c to 17e087b Compare July 11, 2024 00:11

jannfis reviewed Jul 11, 2024

View reviewed changes

ArthurVardevanyan force-pushed the argo-in-any-ns branch 2 times, most recently from 0c967c6 to fe80772 Compare July 11, 2024 22:34

ArthurVardevanyan changed the title ~~feat(sourceNamespace): Glob Negate Support (#19016)~~ feat(sourceNamespace): Regex Support (#19016) Jul 11, 2024

ArthurVardevanyan force-pushed the argo-in-any-ns branch 2 times, most recently from 94f7c5b to 4ac948f Compare August 5, 2024 17:49

feat(sourceNamespace): Regex Support

b9acf0d

Signed-off-by: Arthur <[email protected]>

ArthurVardevanyan force-pushed the argo-in-any-ns branch 5 times, most recently from 0fc2753 to c9ebe95 Compare August 12, 2024 16:12

jannfis reviewed Aug 12, 2024

View reviewed changes

ArthurVardevanyan force-pushed the argo-in-any-ns branch from c9ebe95 to 9c51634 Compare August 12, 2024 20:48

feat(sourceNamespace): Separate exactMatch into patternMatch

707af58

Signed-off-by: Arthur <[email protected]>

ArthurVardevanyan force-pushed the argo-in-any-ns branch from 9c51634 to 707af58 Compare August 12, 2024 20:52

jannfis approved these changes Aug 13, 2024

View reviewed changes

jannfis merged commit 588b251 into argoproj:master Aug 13, 2024
28 of 29 checks passed

ArthurVardevanyan deleted the argo-in-any-ns branch August 13, 2024 13:48

ArthurVardevanyan added a commit to ArthurVardevanyan/HomeLab that referenced this pull request Aug 13, 2024

feat(ArgoCD): argoproj/argo-cd#19017

d4baf6e

ArthurVardevanyan mentioned this pull request Aug 13, 2024

fix(AnyNameSpaceRegex): Additional Functions Glob to Regexexp #19516

Merged

ArthurVardevanyan added a commit to ArthurVardevanyan/HomeLab that referenced this pull request Aug 13, 2024

feat(ArgoCD): argoproj/argo-cd#19017

3364c02

ArthurVardevanyan added a commit to ArthurVardevanyan/HomeLab that referenced this pull request Aug 13, 2024

feat(ArgoCD): argoproj/argo-cd#19017 && argoproj/argo-cd#19516

d78d525

gcp-cherry-pick-bot bot mentioned this pull request Aug 23, 2024

feat(sourceNamespace): Regex Support (#19016) (cherry-pick #19017) #19664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sourceNamespace): Regex Support (#19016) #19017

feat(sourceNamespace): Regex Support (#19016) #19017

ArthurVardevanyan commented Jul 10, 2024 •

edited

Loading

bunnyshell bot commented Jul 10, 2024 •

edited

Loading

bunnyshell bot commented Jul 10, 2024 •

edited

Loading

jannfis left a comment

codecov bot commented Jul 10, 2024 •

edited

Loading

ArthurVardevanyan commented Jul 10, 2024 •

edited

Loading

jannfis commented Jul 10, 2024

ArthurVardevanyan commented Jul 11, 2024 •

edited

Loading

jannfis left a comment

jannfis Jul 11, 2024

jannfis commented Jul 11, 2024

ArthurVardevanyan commented Jul 11, 2024

jannfis commented Jul 11, 2024

ArthurVardevanyan commented Jul 12, 2024

ArthurVardevanyan commented Aug 5, 2024

jannfis commented Aug 12, 2024

ArthurVardevanyan commented Aug 12, 2024 •

edited

Loading

jannfis left a comment

jannfis Aug 12, 2024

ArthurVardevanyan Aug 12, 2024

jannfis left a comment

jannfis commented Aug 23, 2024

feat(sourceNamespace): Regex Support (#19016) #19017

feat(sourceNamespace): Regex Support (#19016) #19017

Conversation

ArthurVardevanyan commented Jul 10, 2024 • edited Loading

bunnyshell bot commented Jul 10, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

bunnyshell bot commented Jul 10, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

jannfis left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

ArthurVardevanyan commented Jul 10, 2024 • edited Loading

jannfis commented Jul 10, 2024

ArthurVardevanyan commented Jul 11, 2024 • edited Loading

jannfis left a comment

Choose a reason for hiding this comment

jannfis Jul 11, 2024

Choose a reason for hiding this comment

jannfis commented Jul 11, 2024

ArthurVardevanyan commented Jul 11, 2024

jannfis commented Jul 11, 2024

ArthurVardevanyan commented Jul 12, 2024

ArthurVardevanyan commented Aug 5, 2024

jannfis commented Aug 12, 2024

ArthurVardevanyan commented Aug 12, 2024 • edited Loading

jannfis left a comment

Choose a reason for hiding this comment

jannfis Aug 12, 2024

Choose a reason for hiding this comment

ArthurVardevanyan Aug 12, 2024

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

jannfis commented Aug 23, 2024

ArthurVardevanyan commented Jul 10, 2024 •

edited

Loading

bunnyshell bot commented Jul 10, 2024 •

edited

Loading

bunnyshell bot commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

ArthurVardevanyan commented Jul 10, 2024 •

edited

Loading

ArthurVardevanyan commented Jul 11, 2024 •

edited

Loading

ArthurVardevanyan commented Aug 12, 2024 •

edited

Loading