new(redis-cli): Add support for Redis CLI with password as environment variable and command-line flags

[arunsathiya]

Overview

This PR adds support for redis-cli. The password for connecting to the redis server can be set as an environment variable and it'll be used by the shell plugin.

Type of change

• Created a new plugin
• Improved an existing plugin
• Fixed a bug in an existing plugin
• Improved contributor utilities or experience

Related Issue(s)

• Resolves: New plugin: Redis CLI #275

How To Test

• Sign up for a Redis hosting provider like Upstash.
• Create a Redis server.
• Make note of the password, hostname and port number shown by that provider.
• Clone this branch and run make redis/build
• Set up redis shell plugin by running op plugin init redis-cli. Set the password as a 1Password field item.
• Run redis-cli -h host-address-here.com -p port-number-here PING
• See response PONG

Changelog

Add support for Redis CLI with environment variable-based provisioning.

[arunsathiya]

A couple of quick notes:

• ManagementURL is not set for the credential because, depending on the redis hosting provider the link will be different.
• We provision using environment variables for now, because REDISCLI_AUTH is the only support variable, for the password. There isn't one for the host, port or the database.
• There isn't a config file provisioner for redis CLI. So, that's not an option.
• I was exploring a new arguments provisioner for tunnelto, but it's on hold at this time. I can take another stab at it soon and when it's ready, we can adopt the same concept here for redis, so that host, port and database can be set up as individual field items too.

Let me know what you think about this!

[hculea]

Any reason not to use, in conjunction with the envvar provisioner, a command line provisioner (i.e. adding the port and host flags within the provisioner itself). The entire provisioner should be the equivalent of:

REDISCLI_AUTH=<value> redis-cli -p <port> -h <host>

[arunsathiya]

@hculea, that's a great suggestion, thank you!

So, today's the first time I properly dived into the provisioner SDK and I see that it's possible to provision two types of credentials (environment variables and command-line flags, in this case) in a single provisioner. The current shell plugin supports this behavior with a new AddArgsImmediatelyAfterExecutableName provisioner function.

Examples to test with:

• redis-cli ping actually translates into redis-cli -h host.com -p 1234 ping behind the scenes, and outputs pong.
• redis-cli incr counter_value translates into redis-cli -h host.com -p 1234 incr counter_value behind the scenes, and outputs (integer) 1

While Redis CLI supports multiple command line flags, I have worked here with the assumption that host and port are minimum-required flags. If the user supplies additional flags, they'll be used properly.

Example:

• redis-cli -r 5 INCR counter_value translates into redis-cli -h host.com -p 1234 -r 5 INCR counter_value behind the scenes, and outputs the following:

(integer) 1
(integer) 2
(integer) 3
(integer) 4
(integer) 5

Also, if the user decides to use a custom value for the host and port, as opposed to the value we have on the 1Password item, that shouldn't be a problem either. The flag value entered by the user will be used and the one from the 1Password item be skipped.

Let me know what you think of this setup!

[arunsathiya]

The current shell plugin supports this behavior with a new AddArgsImmediatelyAfterExecutableName provisioner function.

For some additional context on why this new function is needed, AddArgs function appends the command-line arguments to the end of the user-inputted command. That is, redis-cli incr counter_value becomes redis-cli incr counter_value -h host.com -p 1234 but this is not supported by Redis CLI for some reason. It results in the following error:

AUTH failed: ERR AUTH <password> called without any password configured for the default user. Are you sure your configuration is correct?
(error) ERR wrong number of arguments for 'incr' command

Redis CLI expects the flags to be immediately after the executable name, thus the new function AddArgsImmediatelyAfterExecutableName. So, the same command will translate to redis-cli -h host.com -p 1234 incr counter_value and output the correct response.

[arunsathiya]

Couple of additional notes for all reviewers:

• 👍🏼 Since the last iteration, we now have username as a separate field, thus allowing scoped/ACL-based user accounts.
• 👍🏼 There isn't a way to set the "default" value for the username at this time: feat: Default value for a credential field #281 I am hoping redis users will be familiar with setting the standard "default" username as the default, when they don't have ACL-based accounts.
• 👍🏼 Tested with Upstash, Redis.com (enterprise Redis cloud, from the same makers of open source Redis software), works well.
• 👎🏼 Yet to test on AWS Elasticache. More significantly, yet to test/understand how AWS-hosted redis clusters work, because based on a quick glance, their access is locked down to EC2 instances.

For the near future:

• Investigate and fix optional setting, so that it can be used on the username field: Setting an optional field doesn't prompt for user input during "op plugin init" phase #280
• Decide on how we should approach redis-Terraform compatibility. I talked about this with @AndyTitu a bit already. The main blocker is that, for our Terraform shell plugin, we'll need to use environment variables instead of command-line arguments for authentication. But the environment variables for various redis providers are different:

Name	Provider	Link	Environment variables
rediscloud	redislabs	Cell	REDISCLOUD_URL, REDISCLOUD_ACCESS_KEY and REDISCLOUD_SECRET_KEY
redisdb	CruGlobal	Cell	REDISDB_DATABASE, REDISDB_HOSTNAME, REDISDB_PORT
rediscloud	DevopsHomeserve	Cell	REDISCLOUD_URL, REDISCLOUD_ACCESS_KEY and REDISCLOUD_SECRET_KEY

As we can see here, these environment variables vary between providers, and most importantly, vary from the open source redis software, which offers only REDISCLI_AUTH.

So, we will need to think of how to best-approach Terraform compatibility. In the meantime, I propose we push forward the current version, as there's significant value for Upstash and redis.com cloud users.

[AndyTitu]

This is what Arun and I discussed earlier today.

The approach is to use 2 different credential usages (for 2 different credentials), 1 for cloud and 1 for local, and to provision them using the above strategies. In this way, we enable the Redis cloud + terraform use case, and we also enable redis-cli authentication both via ACL and "legacy" password depending on whether the user specifies the optional username or not.

Here is each credential's structure:

[accraw]

I did a functional test with local docker + ACL and it worked beautifully.

[arunsathiya]

Updated thought process:

• REDISCLOUD_ACCESS_KEY and REDISCLOUD_SECRET_KEY environment variables from the rediscloud Terraform provider are not for connecting to a redis server hosted on Redis.com, as I had originally understood. They are for connecting to the redis.com enterprise platform itself, to create and manage servers, amidst various other features.
• It makes sense to support redis.com authentication on the same shell plugin instead of creating an entirely new shell plugin for it, even though it's only used with the Terraform provider and not on a CLI binary.

So, the updated credentials are as follows:

• User Credentials which is a combination of username, password, host and port. Only password is mandatory. Default username is "default", default host and port are 127.0.0.1 and 6379. If someone wants to connect to a cloud redis server, they'd obviously need to define the host and port.
• Secret Key which is a combination of Access Key and Secret Key fields, which are obtained from redis.com's API keys page, and facilitate connection to the redis.com cloud platform.

New testing instructions, which can be tested only by internal contributors:

• Fetch the internal CLI changes and run op plugin init redis-cli
• When prompted to choose one of the two credentials to set up, start with User Credentials. Configure username (either "default" or a custom user account created using ACLs), password, host and port.
• Make sure that redis-cli commands (some commands to run here) respond as expected.
• Run op plugin init redis-cli again and this time, choose "Secret Key" credential and save that credential to your 1Password work.
• Create main.tf in a folder and run the following commands in order: terraform init > terraform plan > terraform apply. All of these commands use the Secret Key credential created in the last step.

Sample main.tf file to test with:

terraform {
  required_providers {
    rediscloud = {
      source = "RedisLabs/rediscloud"
      version = "1.1.1"
    }
  }
}

provider "rediscloud" {}

data "rediscloud_regions" "example" {
}

output "all_regions" {
  value = data.rediscloud_regions.example.regions
}

[arunsathiya]

Those failing validation checks are tracked internally now. I think we'll need to discuss and conclude how we handle those as well before we can merge this PR.

[arunsathiya]

@AndyTitu, thanks for all the feedback! I trust all changes have been addressed but let me know if you spot anything off.

I have also done a functional testing at the latest commit, 92af35e, and all cases work well.

[arunsathiya]

Regarding the merge conflict, I am going to rebase with main once the reviews are done. I could do that right now, but I have a feeling that pushing after the rebase will change the commit IDs? 🤔 So, I'll hold off now.

[AndyTitu]

This is getting very close! Nice work @arunsathiya !

[AndyTitu]

Those failing validation checks are tracked internally now. I think we'll need to discuss and conclude how we handle those as well before we can merge this PR.

This is the failing check:
✘ Has no more than one credential type defined. Plugins with multiple credential types are not supported yet

We conceptually no longer need this. I have also confirmed that we don't need it from a technical point of view by testing the latest version of redis plugin with the latest cli.

@arunsathiya feel free to remove it.

[arunsathiya]

@AndyTitu Appreciate the feedback!

I built AddArgsAtIndex(1, argsMap) as suggested, but there's a sticky point with it. The only two valid indices would be: 1 and len(args), where 1 corresponds to immediately after the executable name and len(args) corresponds to the end of the command line. If one were to set a different value, including 0, provisioning breaks. This especially gets complicated when CLI programs stress importance on the order of flags.

One example is, redis handles redis-cli -h value -p value incr key and redis-cli incr hey -h value -p value differently. And if I were to do something like, AddArgsAtIndex(2, argsMap), that results in redis-cli incr -h value -p value key, which breaks entirely.

So, in fe969f8, I have taken a different approach: AddArgs(provisionImmediatelyAfterExecutable bool, args ...string). The value set for provisionImmediatelyAfterExecutable controls where the provisioned arguments go.

Let me know what you think of this!

[scottisloud]

Revitalizing this PR, did some substantial rebasing and improved certain aspects or updated them to align with current behaviours.

Re-requested review from Andy since Andy was an original reviewer but @AndyTitu don't feel obligated if this now falls way outside your responsibilities (or memory).

some requested changes lost in rebase and force push