Add shared gRPC mode to run the native provider #267

ulucinar · 2022-03-21T15:46:55Z

Description of your changes

Fixes #261
Adds a config.Provider.SharedGRPC provider configuration option to configure the shared gRPC native plugin mode.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Write tests for pkg/terraform/shared_grpc_server.go
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested against provider-jet-template and with provider-jet-azure.

pkg/terraform/store.go

pkg/terraform/shared_grpc_server.go

muvaf · 2022-03-22T21:48:38Z

pkg/terraform/shared_grpc_server.go

+			ws.mu.Unlock()
+		}()
+		//#nosec G204 no user input
+		cmd := ws.executor.Command(ws.nativeProviderPath, ws.nativeProviderArgs...)


A reminder for my comment here. We should remove env var-based auth slowly since it won't work with this setup.

Commented on the corresponding code section in jet-template as indicated together with an explanation:
https://github.com/crossplane-contrib/provider-jet-template/blob/ee4af597e5780c8feed2659b634909c011f0dbce/internal/clients/template.go#L89

It looks like the current upstream (native) provider used in the template no longer requires credentials. But still I have kept the credential injection sections but commented them out.

Actually I was meaning deprecating this field pointing that the configuration should be used instead of env.

Oh, I see. I was considering Setup.Env as a general purpose configuration field that would allow us to inject arbitrary env. variables, not just those used for injecting credentials. What do you think?

I'm not sure. It's only used for credentials and they have to go to provider, so there isn't a use case coming to my mind that you want to affect TF CLI's env vars.

muvaf

Looking pretty good, most of the comments are about styling. Thanks @ulucinar !

pkg/terraform/provider_runner.go

pkg/terraform/shared_grpc_server.go

pkg/terraform/provider_runner.go

pkg/terraform/store.go

pkg/terraform/provider_runner.go

pkg/terraform/shared_grpc_server.go

muvaf

LGTM! Thank you @ulucinar !

muvaf · 2022-03-31T06:58:21Z

pkg/terraform/store.go

 	w.env = ts.Env
+	w.env = append(w.env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))


Suggested change

w.env = ts.Env

w.env = append(w.env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))

w.env = append(ts.Env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))

nitpick

Funny thing is I had initially done it this way. But it results in a linter warning: appendAssign: append result not assigned to the same slice.

muvaf · 2022-03-31T06:59:58Z

pkg/terraform/store.go

+		mu:             sync.Mutex{},
+		fs:             afero.Afero{Fs: afero.NewOsFs()},
+		executor:       exec.New(),
+		providerRunner: NewNoOpProviderRunner(),


nitpick: since jet-template uses grpc by default, we can also use it by default, too, and document in the guide that you can use NoOp one if shared grpc doesn't work for you for some reason.

I would prefer to have an initial set of providers in the wild for some time before making this the default. What do you think?

muvaf · 2022-03-31T07:03:34Z

pkg/terraform/provider_runner.go

+	if len(sr.nativeProviderPath) == 0 {
+		return "", errors.New(errNativeProviderPath)
+	}


We can require this by taking it as direct argument on NewSharedProvider, hence remove the if condition check.

ulucinar · 2022-03-31T19:33:25Z

Thanks @muvaf. I will test the latest changes in this PR against crossplane-contrib/provider-jet-azure#166 before merging.

…ode for a provider Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

- Add tests Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

…erRunner Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

muvaf · 2022-04-01T14:13:48Z

pkg/terraform/provider_runner.go

+func (sr *SharedProvider) Start() (string, error) { //nolint:gocyclo
+	sr.mu.Lock()
+	defer sr.mu.Unlock()
+	log := sr.logger.WithValues("nativeProviderPath", sr.nativeProviderPath, "nativeProviderArgs", sr.nativeProviderArgs)


nitpick: we can do this initialization of log in NewSharedProvider so that it's done only once.

ulucinar mentioned this pull request Mar 21, 2022

Add shared gRPC mode to run the native provider crossplane-contrib/provider-jet-template#21

Merged

2 tasks

ulucinar changed the title ~~Add config.Provider.SharedGRPC config option~~ Shared gRPC server mode for the native provider Mar 22, 2022

ulucinar changed the title ~~Shared gRPC server mode for the native provider~~ Add shared gRPC mode to run the native provider Mar 22, 2022

ulucinar force-pushed the fix-261 branch from 6734389 to b8ace03 Compare March 22, 2022 11:30

ulucinar mentioned this pull request Mar 22, 2022

Switch to shared gRPC server implementation crossplane-contrib/provider-jet-azure#166

Merged

2 tasks

muvaf reviewed Mar 22, 2022

View reviewed changes

pkg/terraform/store.go Outdated Show resolved Hide resolved

pkg/terraform/shared_grpc_server.go Outdated Show resolved Hide resolved

muvaf reviewed Mar 22, 2022

View reviewed changes

ulucinar requested review from turkenh and muvaf March 28, 2022 06:39

ulucinar force-pushed the fix-261 branch from b8ace03 to 3f3dce8 Compare March 28, 2022 13:15

muvaf reviewed Mar 30, 2022

View reviewed changes

pkg/terraform/shared_grpc_server.go Outdated Show resolved Hide resolved

ulucinar force-pushed the fix-261 branch from a8e430d to 06b73b0 Compare March 30, 2022 15:06

muvaf approved these changes Mar 31, 2022

View reviewed changes

muvaf reviewed Mar 31, 2022

View reviewed changes

ulucinar force-pushed the fix-261 branch from 06b73b0 to 58a0ffc Compare March 31, 2022 19:31

ulucinar added 6 commits March 31, 2022 22:34

Add config.Provider.SharedGRPC config option to control shared gRPC m…

268f04a

…ode for a provider Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Implement shared gRPC server in terraform.WorkspaceStore

172f610

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Add NativeProviderRunner interface

5b31f82

- Add tests Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Use terraform.NoOpProviderRunner as the default WorkspaceStore.provid…

a9d407d

…erRunner Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Rename terraform.NativeProviderRunner as terraform.ProviderRunner

0da01cc

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Set TF_REATTACH_PROVIDERS env. variable per Workspace

4354101

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar force-pushed the fix-261 branch from 58a0ffc to 4354101 Compare March 31, 2022 19:35

muvaf reviewed Apr 1, 2022

View reviewed changes

ulucinar merged commit 6ef18d5 into crossplane:main Apr 2, 2022

ulucinar deleted the fix-261 branch April 2, 2022 20:50

ulucinar mentioned this pull request Apr 22, 2022

Rollout shared gRPC server support for jet-based AWS, GCP, Azure providers #278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shared gRPC mode to run the native provider #267

Add shared gRPC mode to run the native provider #267

ulucinar commented Mar 21, 2022 •

edited

Loading

muvaf Mar 22, 2022

ulucinar Mar 28, 2022 •

edited

Loading

muvaf Mar 28, 2022

ulucinar Mar 30, 2022 •

edited

Loading

muvaf Mar 31, 2022

muvaf left a comment

muvaf left a comment

muvaf Mar 31, 2022

ulucinar Mar 31, 2022

muvaf Mar 31, 2022

ulucinar Mar 31, 2022

muvaf Mar 31, 2022 •

edited

Loading

ulucinar commented Mar 31, 2022

muvaf Apr 1, 2022

		w.env = ts.Env
		w.env = append(w.env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))

	w.env = ts.Env
	w.env = append(w.env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))
	w.env = append(ts.Env, fmt.Sprintf(fmtEnv, envReattachConfig, attachmentConfig))

Add shared gRPC mode to run the native provider #267

Add shared gRPC mode to run the native provider #267

Conversation

ulucinar commented Mar 21, 2022 • edited Loading

Description of your changes

How has this code been tested

Choose a reason for hiding this comment

ulucinar Mar 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulucinar Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

muvaf left a 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

muvaf Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

ulucinar commented Mar 31, 2022

Choose a reason for hiding this comment

ulucinar commented Mar 21, 2022 •

edited

Loading

ulucinar Mar 28, 2022 •

edited

Loading

ulucinar Mar 30, 2022 •

edited

Loading

muvaf Mar 31, 2022 •

edited

Loading