[v2] Set config all at once (#718)

We currently issue one `SetAllConfig` RPC for each user-specified
config. This is slow but it has important correctness guarantees:

1. The order we apply config matters -- if the user specifies `foo: foo`
followed by `foo: bar`, the net result must always be `foo: bar`.
2. The Pulumi CLI (and therefore Automation API) only allows specifying
`--path` on an all-or-nothing basis. This is bad for us because we
potentially have a blend of path and non-path keys.

Ideally we would be able to supply all of our configs to the automation
API in a single call, and in the case where _all_ of our config keys are
path-like (or all are not path-like) we actually can do that because we
no longer have limitation (2).

This PR makes that possible in the general case by transforming our
config keys in a way that allows us to treat them as if they are all
path-like.

In particular:
* The agent's `SetAllConfig` handler is modified to take a list of
configs instead of a map in order to preserve config order. The
top-level `path` param is also removed and handled on a per-key basis.
* While resolving configs, we escape any non-path keys so subsequent
path parsing treats them as verbatim. For example `foo.bar` gets escaped
as `["foo.bar"]`.
* We can then supply all of our keys at once to Automation API with
`Path: true`.
* If there are no configs to set then the operator doesn't invoke
`SetAllConfig`.

Fixes #650

agent/pkg/proto/agent.proto

@@ -91,24 +91,23 @@ message StackSummary {
 }
 
 message SetAllConfigRequest {
-  optional bool path =
-      1; // Parse the keys as paths in a map or list rather than raw strings
-  map<string, ConfigValue> config =
-      2; // a map of ConfigValue used by Pulumi programs
+  repeated ConfigItem config = 1;
 }
 
-message ConfigValue {
+message ConfigItem {
+  string key = 1; // The config's key.
+  optional bool path = 2; // Parse the key as a paths in a map or list rather than as a raw string.
   oneof v {
-    google.protobuf.Value value = 1;
-    ConfigValueFrom value_from = 2;
+    google.protobuf.Value value = 3; // A literal value.
+    ConfigValueFrom value_from = 4; // A value read from the environment or local disk.
   }
-  optional bool secret = 3;
+  optional bool secret = 5; // Whether the config is a secret.
 }
 
 message ConfigValueFrom {
   oneof f {
-    string env = 1;  // the name of an environment variable to read
-    string path = 2; // the path to a file to read
+    string env = 1;  // The name of an environment variable to read>
+    string path = 2; // The path to a file to read.
   }
 }
 

agent/pkg/server/grpc_test.go

@@ -13,9 +13,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 See the License for the specific language governing permissions and
 limitations under the License.
 */
-package server_test
-
-// TODO: Why are we using a black box test package?
+package server
 
 import (
 	"context"
@@ -25,7 +23,6 @@ import (
 	"time"
 
 	pb "github.com/pulumi/pulumi-kubernetes-operator/v2/agent/pkg/proto"
-	"github.com/pulumi/pulumi-kubernetes-operator/v2/agent/pkg/server"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
@@ -45,7 +42,7 @@ func TestGracefulShutdown(t *testing.T) {
 	tc := newTC(ctx, t, tcOptions{ProjectDir: "./testdata/hang", Stacks: []string{"test"}})
 	log := zap.L().Named("test").Sugar()
 	lis := bufconn.Listen(1024)
-	s := server.NewGRPC(log, tc.server, nil)
+	s := NewGRPC(log, tc.server, nil)
 	go func() {
 		// This should exit cleanly if we shut down gracefully.
 		if err := s.Serve(ctx, lis); err != nil {

agent/pkg/server/server.go

@@ -20,6 +20,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -41,6 +42,8 @@ import (
 	"github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh"
 	"github.com/pulumi/pulumi/sdk/v3/go/auto/optup"
 	"github.com/pulumi/pulumi/sdk/v3/go/common/apitype"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource/config"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/tokens"
 )
 
 const (
@@ -242,27 +245,67 @@ func (s *Server) SetAllConfig(ctx context.Context, in *pb.SetAllConfigRequest) (
 		return nil, err
 	}
 
-	config := make(map[string]auto.ConfigValue)
-	for k, inv := range in.GetConfig() {
-		if k == "" {
-			return nil, status.Errorf(codes.InvalidArgument, "invalid config key: %s", k)
-		}
-		v, err := unmarshalConfigValue(inv)
-		if err != nil {
-			return nil, err
-		}
-		config[k] = v
+	p, err := stack.Workspace().ProjectSettings(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("getting project settings: %w", err)
+	}
+
+	config, err := unmarshalConfigItems(p.Name.String(), in.Config)
+	if err != nil {
+		return nil, fmt.Errorf("config items: %w", err)
 	}
+
 	s.log.Debugw("setting all config", "config", config)
 
-	err = stack.SetAllConfigWithOptions(ctx, config, &auto.ConfigOptions{
-		Path: in.GetPath(),
-	})
+	err = stack.SetAllConfigWithOptions(ctx, config, &auto.ConfigOptions{Path: true})
 	if err != nil {
 		return nil, err
 	}
 
-	return nil, nil
+	return &pb.SetAllConfigResult{}, nil
+}
+
+// unmarshalConfigItems maps proto ConfigItems to an auto.ConfigMap whose
+// structure is appropriate for invoking with `pulumi config set-all --path`
+// for efficiency. Config items whose keys are not meant to be treated as paths
+// are escaped so they are treated as verbatim property paths.
+func unmarshalConfigItems(project string, items []*pb.ConfigItem) (auto.ConfigMap, error) {
+	out := auto.ConfigMap{}
+
+	for _, item := range items {
+		v, err := unmarshalConfigItem(item) // Resolves valueFrom.
+		if err != nil {
+			return nil, fmt.Errorf("unmarshalling %q config: %w", item.Key, err)
+		}
+
+		// ParseKey requires all keys to be explicitly namespaced, so scope any
+		// implicit keys to our project.
+		key := item.Key
+		if !strings.Contains(key, tokens.TokenDelimiter) {
+			key = project + ":" + key
+		}
+		k, err := config.ParseKey(key)
+		if err != nil {
+			return nil, fmt.Errorf("parsing %q key: %w", key, err)
+		}
+
+		// Keys which are not meant to be interpreted as paths as escaped so
+		// subsequent path parsing treats them as-is. In particular "foo"
+		// becomes `["<foo>"]`. Items which are already escaped in this way are
+		// untouched. See
+		// https://github.com/pulumi/pulumi/blob/4424d69177c47385ea4a01d8ad1b0b825d394f63/sdk/go/common/resource/properties_path.go#L32-L64
+		path := false
+		if item.Path != nil {
+			path = *item.Path
+		}
+		if !path && !(strings.HasPrefix(k.Name(), `["`) && strings.HasSuffix(k.Name(), `"]`)) {
+			k = config.MustMakeKey(k.Namespace(), fmt.Sprintf("[%q]", k.Name()))
+		}
+
+		out[k.String()] = v
+	}
+
+	return out, nil
 }
 
 func (s *Server) Install(ctx context.Context, in *pb.InstallRequest) (*pb.InstallResult, error) {
@@ -656,15 +699,15 @@ func (s *Server) Destroy(in *pb.DestroyRequest, srv pb.AutomationService_Destroy
 	return nil
 }
 
-func unmarshalConfigValue(inv *pb.ConfigValue) (auto.ConfigValue, error) {
+func unmarshalConfigItem(inv *pb.ConfigItem) (auto.ConfigValue, error) {
 	v := auto.ConfigValue{
 		Secret: inv.GetSecret(),
 	}
 	switch vv := inv.V.(type) {
-	case *pb.ConfigValue_Value:
+	case *pb.ConfigItem_Value:
 		// FUTURE: use JSON values
 		v.Value = fmt.Sprintf("%v", vv.Value.AsInterface())
-	case *pb.ConfigValue_ValueFrom:
+	case *pb.ConfigItem_ValueFrom:
 		switch from := vv.ValueFrom.F.(type) {
 		case *pb.ConfigValueFrom_Env:
 			data, ok := os.LookupEnv(from.Env)