Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

arunsathiya
Copy link
Contributor

@arunsathiya arunsathiya commented May 31, 2023

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)

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
image

Changelog

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

@arunsathiya arunsathiya self-assigned this May 31, 2023
@arunsathiya arunsathiya added the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label May 31, 2023
@arunsathiya
Copy link
Contributor Author

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!

@arunsathiya arunsathiya marked this pull request as ready for review May 31, 2023 03:36
@hculea
Copy link
Member

hculea commented May 31, 2023

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
Copy link
Contributor Author

@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 arunsathiya changed the title new(redis-cli): Add support for Redis CLI with environment variable-based provisioning new(redis-cli): Add support for Redis CLI with password as environment variable and command-line flags Jun 1, 2023
@arunsathiya
Copy link
Contributor Author

arunsathiya commented Jun 1, 2023

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
Copy link
Contributor Author

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.

plugins/redis/redis-cli.go Outdated Show resolved Hide resolved
@AndyTitu
Copy link
Contributor

AndyTitu commented Jun 2, 2023

Screenshot 2023-06-02 at 19 14 29

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:
Screenshot 2023-06-02 at 19 16 40

@accraw
Copy link
Contributor

accraw commented Jun 2, 2023

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

@arunsathiya
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

plugins/redis/redis-cli.go Outdated Show resolved Hide resolved
sdk/provisioner.go Outdated Show resolved Hide resolved
@arunsathiya
Copy link
Contributor Author

@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
Copy link
Contributor Author

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.

Copy link
Contributor

@AndyTitu AndyTitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting very close! Nice work @arunsathiya !

sdk/provision/chained_provisioner.go Outdated Show resolved Hide resolved
sdk/provision/chained_provisioner.go Outdated Show resolved Hide resolved
sdk/provisioner.go Outdated Show resolved Hide resolved
sdk/provision/args_provisioner.go Outdated Show resolved Hide resolved
@AndyTitu
Copy link
Contributor

AndyTitu commented Jun 7, 2023

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
Copy link
Contributor Author

@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!

arunsathiya and others added 28 commits August 17, 2023 05:20
…ed\, because multi-credential support is beginning to be possible
…ter the executable name, or at the end, because specific index provisioning breaks some cases
… seem consistent, at least in the case of user key
…ause by default, a redis server does not require an username or password to connect to it.
…ately after the executable name, or at the end, because specific index provisioning breaks some cases"

This reverts commit fe969f8.
…isioner, build a safeguard to ensure that arguments are not provisioned out of bounds.
…arguments provision that injects arguments at the first index. This works along with the environment variable provisioner as a chained provision. This is a temporary arrangement until the Arguments Provision is reintroduced at the SDK-level.
…n names: Account Key and User Key. Credential name remains API Key
Co-authored-by: Floris van der Grinten <floris.vandergrinten@agilebits.com>
…re defined\, because multi-credential support is beginning to be possible"

This reverts commit 0100c22.
…lows for more control over the order in which the arguments are provisioned
…line arguments but it's not working just yet due to slice range issues
…rors, but this is not necessarily a concern in redis-cli because we always provision at index 1 and redis-cli is the minimum required command
@arunsathiya
Copy link
Contributor Author

Been a while since I last worked on this. Some progress notes and testing instructions below!

New changes:

  • Changes made in 47d5292 ensure that secrets are sourced from 1Password, but if an user provisions them manually as command line arguments, they are prioritized.
  • Authentication is not skipped by using these arguments.

Testing instructions:

  • Set up redis shell plugin with op plugin init redis-cli
  • Import host, port, username and password as part of the setup flow
  • Run a test command like redis-cli incr key. What happens behind the scenes is redis-cli -h <value> -p <value> --user <value> incr key (redis CLI requires that these flags are immediately after redis-cli and before the executing command. That's why we provision these flags at index 1), and the password is provisioned as a REDISCLI_AUTH environment variable.
  • Run a test command like redis-cli incr key -p 19261 or redis-cli -p 19261 incr key. These cases should be handled as well. Behind the scenes, the provisioner does redis-cli -h <value> -p 19261 --user <value> incr key and REDISCLI_AUTH for password.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress this PR is being worked on/comments are in the process of being addressed by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New plugin: Redis CLI
6 participants