-
Notifications
You must be signed in to change notification settings - Fork 38
Start shared native provider using a magic cookie #275
Conversation
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went over the PR offline and this method seems to work for all but with more granular configuration. Thanks for digging and coming up with a solution that works for all the three!
pkg/terraform/provider_runner.go
Outdated
// NewSharedProvider instantiates a SharedProvider with an | ||
// OS executor using the supplied logger | ||
func NewSharedProvider(l logging.Logger, nativeProviderPath string, opts ...SharedGRPCRunnerOption) *SharedProvider { | ||
func NewSharedProvider(l logging.Logger, nativeProviderPath, nativeProviderName string, protocolVersion int, opts ...SharedGRPCRunnerOption) *SharedProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this offline and it seems protocolVersion
is not easily found by users and it's 5
for all the big three. So, having this value as default and overridden optionally similar to magic cookie could make it easier for folks to use this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @muvaf! We now default to 5 as the gRPC protocol version.
pkg/terraform/provider_runner.go
Outdated
fmtSetEnv = "%s=%s" | ||
envReattachConfig = "TF_REATTACH_PROVIDERS" | ||
envMagicCookie = "TF_PLUGIN_MAGIC_COOKIE" | ||
defaultMagicCookie = "d602bf8f470bc67ca7faa0386276bbdd4330efaf76d1a219cb4d6991ca9872b2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment about how we found this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…er to 5 Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
c80bf16
to
508b40c
Compare
Description of your changes
It turns out that we cannot run the Terraform GCP provider as a shared server like we do for the AWS and Azure providers (by invoking the plugin binary with certain command-line options). We have identified another way in which we set a cookie in the native provider's environment to keep it running as a shared server. This PR switches to this method in the
SharedProvider
implementation.I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
provider-jet-aws
both locally & in-cluster with a provider package by provisioning and destroying a VPC. Related PR: Switch to shared gRPC server implementation crossplane-contrib/provider-jet-aws#182provider-jet-gcp
both locally & in-cluster with a provider package by provisioning and destroying a ServiceAccount. Related PR: Switch to shared gRPC server implementation crossplane-contrib/provider-jet-gcp#58provider-jet-azure
both locally & in-cluster with a provider package by importing a ResourceGroup. Related PR: Fork shared gRPC server with the magic cookie crossplane-contrib/provider-jet-azure#179