Version protocol switch #3833
Version protocol switch #3833
Conversation
helper/pluginutil/runner.go
Outdated
| @@ -70,6 +71,7 @@ func (r *PluginRunner) runCommon(ctx context.Context, wrapper RunnerUtil, plugin | |||
| if wrapper.MlockEnabled() { | |||
| cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", PluginMlockEnabled, "true")) | |||
| } | |||
| cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", PluginVaultVersionEnv, version.GetVersion().VersionNumber())) | |||
There was a problem hiding this comment.
version.GetVersion().VersionNumber() would return "(version unknown)" if if Version == "unknown" && VersionPrerelease == "unknown" is satisfied. The check on https://github.com/hashicorp/vault/pull/3833/files#diff-cedffd7b22c2dfe1ab831e6c9f38e45dR19 would need to be changed to check for that instead.
There was a problem hiding this comment.
Is the "unknown" check below not sufficient?
There was a problem hiding this comment.
version.GetVersion().VersionNumber() would never return the string "unknown" (it returns "(version unknown)") so verString != "unknown" will always be true, even when the version is unknown.
helper/pluginutil/version.go
Outdated
| return constraint.Check(ver) | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
I think we should default to using/supporting gRPC instead of the other way around.
There was a problem hiding this comment.
In this case if the version is unknown (e.g. from a master build that didn't inherit version from a git tag), it will end up will using netrpc.
There was a problem hiding this comment.
Actually I see why you're doing this (to maintain backward compatibility in the case PluginVaultVersionEnv is not provided due to older versions of the plugin). Maybe if verString == "" should be its own check.
There was a problem hiding this comment.
Unfortunately we can't because older versions of vault won't be setting the ENV variable. If the variable is empty we can't use gRPC
There was a problem hiding this comment.
Right so check for verString == "" early and short-circuit by returning false. We can then return true here and in the error cases.
| true, | ||
| }, | ||
| { | ||
| "", |
There was a problem hiding this comment.
This shows up somewhat weird on the test output, wondering if it's worth adding a name field for this
=== RUN TestGRPCSupport
=== RUN TestGRPCSupport/0.8.3
=== RUN TestGRPCSupport/0.9.2
=== RUN TestGRPCSupport/0.9.2+ent
=== RUN TestGRPCSupport/0.9.2-beta
=== RUN TestGRPCSupport/unknown
=== RUN TestGRPCSupport/#00
--- PASS: TestGRPCSupport (0.00s)
--- PASS: TestGRPCSupport/0.8.3 (0.00s)
--- PASS: TestGRPCSupport/0.9.2 (0.00s)
--- PASS: TestGRPCSupport/0.9.2+ent (0.00s)
--- PASS: TestGRPCSupport/0.9.2-beta (0.00s)
--- PASS: TestGRPCSupport/unknown (0.00s)
--- PASS: TestGRPCSupport/#00 (0.00s)
PASS
ok github.com/hashicorp/vault/helper/pluginutil 0.014s
No description provided.