feat: plugins as services #16852

Joibel · 2024-01-12T16:00:36Z

This commit makes the repo server able to use cmp plugins via kubernetes services.

Closes #14132 - read that for details of the design

Some discussion points for this PR:

Plugin name: Currently determined by the port name in the service. One service can have many ports, and this feels somewhat useful, so I chose the port name. My guess is the correct thing to do is add another gRPC call, which could also return the parameters (future UI work could then expose them for those people who use it to create apps). We'd then query on first noticing and then poll every 30 seconds. I'd like feedback on whether this is the right approach and desired - if so I'll add it.
We cache the service list, but not the sidecar list. I'm assuming some plugins could take time to initialise, so may not have created a socket file in time for a cache at startup approach. It would be trivial to switch though.
SA Token + role + rolebindings in the repo server: I've added it as default mounted now because it makes service plugins just work. Happy not to do this though.
Namespace configuration for where to find services is via en environment variable. I didn't originally propose it, but I think it's the right approach now.
No tests written yet whilst we ascertain what we want.

Checklist:

tico24

Some docs change requests

docs/operator-manual/config-management-plugins.md

tico24

minor nit

docs/operator-manual/config-management-plugins.md

codecov · 2024-01-15T10:14:55Z

Codecov Report

Attention: Patch coverage is 21.64502% with 181 lines in your changes are missing coverage. Please review.

Project coverage is 49.10%. Comparing base (397063f) to head (df2caf8).
Report is 106 commits behind head on master.

❗ Current head df2caf8 differs from pull request most recent head 42761cf. Consider uploading reports for the commit 42761cf to get more accurate results

Files	Patch %	Lines
util/app/discovery/services.go	0.00%	81 Missing ⚠️
util/app/discovery/plugin.go	32.91%	52 Missing and 1 partial ⚠️
util/app/discovery/discovery.go	27.90%	31 Missing ⚠️
reposerver/repository/repository.go	38.88%	11 Missing ⚠️
cmd/argocd/commands/app.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16852      +/-   ##
==========================================
- Coverage   49.21%   49.10%   -0.12%     
==========================================
  Files         274      276       +2     
  Lines       48111    48276     +165     
==========================================
+ Hits        23677    23704      +27     
- Misses      22090    22227     +137     
- Partials     2344     2345       +1

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

docs/operator-manual/config-management-plugins.md

util/app/discovery/discovery.go

util/app/discovery/plugin.go

util/app/discovery/services.go

crenshaw-dev

Overall, I'm liking the ideas.

I do think we should implement some form of auth on v1. Other unauth'ed components are a problem, and it's difficult to retroactively add that.

I also think some of the new files could use more test coverage.

But generally, looking good!

Joibel · 2024-02-26T14:46:21Z

How about this thing from above:
"Plugin name: Currently determined by the port name in the service. One service can have many ports, and this feels somewhat useful, so I chose the port name. My guess is the correct thing to do is add another gRPC call, which could also return the parameters (future UI work could then expose them for those people who use it to create apps). We'd then query on first noticing and then poll every 30 seconds. I'd like feedback on whether this is the right approach and desired - if so I'll add it."

Do you have ideas on how you'd like auth to work?

crenshaw-dev · 2024-02-26T15:22:42Z

Might have to hop on a call to 100% understand the port issue. Gotta focus on ArgoCon stuff for now.

re: auth - I think we should probably ask the user to generate a Secret and mount it to the FS of both the repo-server and the plugins. The plugin server would require that the password be present in gRPC call metadata for every call.

This commit makes the repo server able to use cmp plugins via kubernetes services. It implements argoproj#14132 - read that for details Signed-off-by: Alan Clucas <alan@clucas.org>

Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Alan Clucas <alan@clucas.org>

Signed-off-by: Alan Clucas <alan@clucas.org>

Joibel · 2024-07-11T08:03:08Z

This is ready for re-review. The CI license check is failing, but it is for everything these days.

jsoref · 2024-08-30T14:20:59Z

cmd/argocd-cmp-server/commands/authSecret.go

+	path := fmt.Sprintf("%s/%s", strings.TrimRight(a.filePath, "/"), common.PluginAuthSecretName)
+	content, err := os.ReadFile(path)
+	if err != nil {
+		log.Errorf("No authentication secret present at %s", path)


As someone who spends an inordinate amount of time reading error messages that involve lousy inputs (especially leading/trailing whitespace), I'd like to encourage people to consistently wrap string variables in a quotation character.

Suggested change

log.Errorf("No authentication secret present at %s", path)

log.Errorf("No authentication secret present at '%s'", path)

Note: ideally argocd would be consistent about whether it's using ", ', or "`", but it isn't right now, so if you pick the one that is used the most or by things closest to this code, that'd be great.

jsoref · 2024-08-30T14:21:29Z

cmpserver/apiclient/.gitignore

@@ -0,0 +1 @@
+tests/unreadable


Can you add a newline at EOF? (I'm not sure how to get github to let me suggest this...)

jsoref · 2024-08-30T14:22:44Z

cmpserver/apiclient/clientset.go

 	if err != nil {
-		log.Errorf("Unable to connect to config management plugin service with address %s", address)
+		log.Errorf("Unable to connect to config management plugin with address %s (type %s)", c.address, c.clientType.String())


If address has a risk of having garbage characters, then quotes would be good. I'm assuming that type is a relatively constrained thing and thus shouldn't need them.

jsoref · 2024-08-30T14:23:09Z

cmpserver/apiclient/clientset.go

+	return &clientSet{address: address, secretPath: secretPath, clientType: clientType}
+}
+
+// wrappedStream  wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and


Unless there's a stylistic thing for the double space here...

Suggested change

// wrappedStream wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and

// wrappedStream wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and

jsoref · 2024-08-30T14:26:02Z

cmpserver/apiclient/clientset.go

+func (c *clientSet) readAuthSecret(root string) (string, error) {
+	tryPath := c.secretPath
+	for {
+		path := fmt.Sprintf(filepath.Join(root, tryPath, common.PluginAuthSecretName))


This should make the linter happy:

Suggested change

path := fmt.Sprintf(filepath.Join(root, tryPath, common.PluginAuthSecretName))

path := filepath.Join(root, tryPath, common.PluginAuthSecretName)

The alternative would be:

path := fmt.Sprintf("%s", filepath.Join(root, tryPath, common.PluginAuthSecretName))

The current code is risky in C languages if someone could construct root, tryPath, or common.PluginAuthSecretName to include %... (I expect go might not count as a C language here, but it's good practice to avoid risky style...)

jsoref · 2024-08-30T14:26:58Z

cmpserver/apiclient/clientset.go

+		}
+		tryPath = filepath.Dir(tryPath)
+	}
+	return ``, status.Errorf(codes.Unauthenticated, "No authentication secret present at %s or parents", filepath.Join(root, c.secretPath))


I'd probably want quotes around %s.

I'm not sure what parents is in context. (It's also possible it needs a possessive ' marker.)

jsoref · 2024-08-30T14:28:15Z

common/common.go

 	// PluginConfigFileName is the Plugin Config File is a ConfigManagementPlugin manifest located inside the plugin container
 	PluginConfigFileName = "plugin.yaml"
+	// PluginAuthSecretName is the name of the authentication secret located inside the plugin container
+	PluginAuthSecretName = "secret"
+	// PluginAuthTokenHeader is the name of the header to be used for auth. It must be lower case.


Suggested change

// PluginAuthTokenHeader is the name of the header to be used for auth. It must be lower case.

// PluginAuthTokenHeader is the name of the header to be used for auth. It must be lowercase.

jsoref · 2024-09-30T21:40:09Z

docs/operator-manual/config-management-plugins.md

+
+!!! Ensure that:
+    1. The argocd-repo-server deployment has `automountServiceAccountToken: true`
+	2. The argocd-repo-server's service account has a role binding allowing `get`, `list` and `watch` on services.


Suggested change

2. The argocd-repo-server's service account has a role binding allowing `get`, `list` and `watch` on services.

2. The argocd-repo-server's service account has a role binding allowing `get`, `list` and `watch` on services.

Joibel requested review from a team as code owners January 12, 2024 16:00

tico24 suggested changes Jan 15, 2024

View reviewed changes

tico24 reviewed Jan 15, 2024

View reviewed changes

docs/operator-manual/config-management-plugins.md Outdated Show resolved Hide resolved

Joibel marked this pull request as draft January 15, 2024 15:20

Joibel force-pushed the plugins-as-services branch 5 times, most recently from 782c4e7 to 4709953 Compare January 16, 2024 09:51

Joibel marked this pull request as ready for review January 16, 2024 11:36

jsoref reviewed Jan 19, 2024

View reviewed changes

Joibel force-pushed the plugins-as-services branch from df2caf8 to a8b8175 Compare January 22, 2024 10:35

crenshaw-dev reviewed Feb 26, 2024

View reviewed changes

leoluz added the component:cmp Config Management Plugin related issues label Feb 28, 2024

Joibel force-pushed the plugins-as-services branch from 42761cf to 7dfbd61 Compare July 9, 2024 15:10

Joibel and others added 4 commits July 9, 2024 16:13

feat: plugins as services

ca3a108

This commit makes the repo server able to use cmp plugins via kubernetes services. It implements argoproj#14132 - read that for details Signed-off-by: Alan Clucas <alan@clucas.org>

docs: update docs in line with Josh's review

330ac44

Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com> Signed-off-by: Alan Clucas <alan@clucas.org>

add GetSpecification and authentication

8f01115

Signed-off-by: Alan Clucas <alan@clucas.org>

chore: some tests and fixes

504d406

Signed-off-by: Alan Clucas <alan@clucas.org>

Joibel force-pushed the plugins-as-services branch from 7dfbd61 to 504d406 Compare July 9, 2024 15:13

leoluz added the ready-for-review label Sep 19, 2024

jsoref reviewed Sep 30, 2024

View reviewed changes

leoluz mentioned this pull request Oct 4, 2024

chore(cmp): refactor and test plugin discovery #20217

Open

blakepettersson removed the ready-for-review label Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: plugins as services #16852

feat: plugins as services #16852

Joibel commented Jan 12, 2024 •

edited

Loading

tico24 left a comment

tico24 left a comment

codecov bot commented Jan 15, 2024 •

edited

Loading

crenshaw-dev left a comment

Joibel commented Feb 26, 2024

crenshaw-dev commented Feb 26, 2024

Joibel commented Jul 11, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Aug 30, 2024

jsoref Sep 30, 2024

	log.Errorf("No authentication secret present at %s", path)
	log.Errorf("No authentication secret present at '%s'", path)

	// wrappedStream wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and
	// wrappedStream wraps around the embedded grpc.ClientStream, and intercepts the RecvMsg and

	path := fmt.Sprintf(filepath.Join(root, tryPath, common.PluginAuthSecretName))
	path := filepath.Join(root, tryPath, common.PluginAuthSecretName)

	// PluginAuthTokenHeader is the name of the header to be used for auth. It must be lower case.
	// PluginAuthTokenHeader is the name of the header to be used for auth. It must be lowercase.

	2. The argocd-repo-server's service account has a role binding allowing `get`, `list` and `watch` on services.
	2. The argocd-repo-server's service account has a role binding allowing `get`, `list` and `watch` on services.

		@@ -0,0 +1 @@
		tests/unreadable

feat: plugins as services #16852

Are you sure you want to change the base?

feat: plugins as services #16852

Conversation

Joibel commented Jan 12, 2024 • edited Loading

tico24 left a comment

Choose a reason for hiding this comment

tico24 left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2024 • edited Loading

Codecov Report

crenshaw-dev left a comment

Choose a reason for hiding this comment

Joibel commented Feb 26, 2024

crenshaw-dev commented Feb 26, 2024

Joibel commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joibel commented Jan 12, 2024 •

edited

Loading

codecov bot commented Jan 15, 2024 •

edited

Loading