[CLI] - Add commands for config set-all and rm-all

[komalali]

Fixes: #6329

[justinvp]

LGTM

[EvanBoyle]

One edge case for parsing that we should add test cases for at a minimum.

That sort of leads me to the question of whether or not we should be using the formal:
--plaintext k=v --secret plain=v

You could imagine it also looking like:
k v k1 v2 --secret k3 v3 k4 v4 --secret k5 v5

But this would require setting DisableFlagParsing and then manually handling the flags via the args function. Definitely reasons not to do this, and not sure if it would be considered idiomatic or not. Requiring --plaintext is a little annoying, but the cases we have in mind are more machine-oriented so I'm a little less concerned about it.

I could really see things going either way, curious to hear your thoughts.

[EvanBoyle]

I am having a hard time parsing this. Just to clarify this means that without the path key, these keys ("outer.inner", "foo[0]", ..) are treated as string literals and are not evaluated for index expressions?

[komalali]

Yes. There's a little more context over in the issue around how we got here. If you have ideas on how to make this clearer, let me know.

[EvanBoyle]

It isn't entirely clear to me from reading the code, but where does the underlying network traffic come from? Is it ps.Config.Remove or saveProjectStack? At a surface level it looks like both of them are just writing to disk.

[komalali]

There's a network call every time there's a secret encrypted or decrypted... but other than that it seems that the network call only happens when there's an up.

[EvanBoyle]

I read through the code again, and I don't think this is going to help with the underlying latency issues. Secrets are the bottle-neck here it seems and this implementation loops through encrypting values one at a time for set-all --secret, which should be more or less equivalent to running pulumi config set --secret sequentially. We might need to think about spawing a go routine to do this in parallel, or creating a bulk config encryption endpoint in the service if one doesn't already exist.

It would be helpful to do some benchmarking here to understand performance impacts for a few different scenarios (bulk set plaintext, bulk set a bunch of secrets, etc).

In addition, the other underlying issue that I thought we were tackling with this, but didn't see mentioned in #6329 was the parallelism issues with setting config. Running config command async in a loop across stacks was causing things to get garbled because stack selection was changing. Does this change help with that? I notice that these two methods use setCurrent true when getting the stack so I'm not sure. Would probably be good to test as well.

[komalali]

I know with certainty that this greatly improves the latency experienced by automation api: #6042 (comment)
But yes, I'll do additional benchmarking with plaintext only + secret only bulk setting and consider the go routine as an additional improvement if this isn't satisfactory.

I notice that these two methods use setCurrent true when getting the stack so I'm not sure.

That's a mistake, it should be false. In fact, set-all is false, I just made an error here. The consuming functions in automation api use --stack to specify the stack rather than selectStack before setting the config values, so stack selection does not change and we avoid the aforementioned parallelization issues.

[komalali]

Okay, since my other profiling test was in python, I did some in Go this time.

Before

• Mixed (10 secret, 10 plain): 11.69s
• Mixed (20 secret, 20 plain): 24.85s
• Plain (20 plain): 11.25s
• Plain (40 plain): 26.39s
• Secret (20 secret): 12.29s
• Secret (40 secret): 25.36s

After

• Mixed (10 secret, 10 plain): 1.18s
• Mixed (20 secret, 20 plain): 1.55s
• Plain (20 plain): 1.05s
• Plain (40 plain): 1.20s
• Secret (20 secret): 1.51s
• Secret (40 secret): 2.54s

I think the results speak for themselves. This mirrors what I saw in python and leads me to conclude that the majority of the time lost here is in running the pulumi [blah] commands serially, not in the network calls.

[EvanBoyle]

Sorry I missed that profiling data in the linked issue. Glad to hear we've got this sorted out!

[stack72]

My only feedback here is that the use of --plaintext feels like it's a bit more overhead for users - I'm not disagreeing with the implementation but just thinking from a user perspective

[komalali]

Fair feedback @stack72 and @EvanBoyle.

The reason behind enforcing --plaintext has to do with making the user explicitly acknowledge that they are not storing this value as a secret (since we aren't doing any interactive checking to see if the value looks like a secret or not), but I agree it is a little kludgey and removing it doesn't necessarily complicate things so I could be convinced. The question is do we value being explicit about secret vs non-secret more than the burden of having to type --plaintext every time 🤷 - I could be convinced either way.

The edge case that @EvanBoyle brings up (where there is a = in the key) is a little grosser to manage. We could certainly go with what you're suggesting, (i.e. k v k1 v2 --secret k3 v3 k4 v4 --secret k5 v5) but it certainly complicates the implementation. Let me try to work through the edge case with the current key=value structure and if that proves impossible we can consider the cost of the complicated implementation vs the value of permitting keys with =.

[stack72]

Fair feedback @stack72 and @EvanBoyle.

The reason behind enforcing --plaintext has to do with making the user explicitly acknowledge that they are not storing this value as a secret (since we aren't doing any interactive checking to see if the value looks like a secret or not), but I agree it is a little kludgey and removing it doesn't necessarily complicate things so I could be convinced. The question is do we value being explicit about secret vs non-secret more than the burden of having to type --plaintext every time 🤷 - I could be convinced either way.

The edge case that @EvanBoyle brings up (where there is a = in the key) is a little grosser to manage. We could certainly go with what you're suggesting, (i.e. k v k1 v2 --secret k3 v3 k4 v4 --secret k5 v5) but it certainly complicates the implementation. Let me try to work through the edge case with the current key=value structure and if that proves impossible we can consider the cost of the complicated implementation vs the likelihood of a key including =.

Totally not something I am going to die on either :) I was just thinking about the existing functionality of config set but this has a slightly different use case

[EvanBoyle]

🚀