Merge #149869

149869: sql,externalconn: add ALTER EXTERNAL CONNECTION implmentation r=edwardguo-crl a=edwardguo-crl

### Summary

 Add implementation for ALTER EXTERNAL CONNECTION

 Fixes #98610
 
 Release note (sql change): Users can now ALTER EXTERNAL CONNECTION to change the external connection URI when granted UPDATE privilege on EXTERNAL CONNECTION.

 
 ### Local testing
 
 ```shell

root@localhost:26257/defaultdb> SHOW EXTERNAL CONNECTIONS;
  connection_name |     connection_uri      | connection_type
------------------+-------------------------+------------------
  test            | nodelocal://1/alterguo  | STORAGE
  test1           | nodelocal://1/alterguo1 | STORAGE
(2 rows)

Time: 10ms total (execution 10ms / network 0ms)

root@localhost:26257/defaultdb> ALTER EXTERNAL CONNECTION IF EXISTS test1 AS 'nodelocal://1/alterguo2';
ALTER EXTERNAL CONNECTION

Time: 146ms total (execution 146ms / network 0ms)

root@localhost:26257/defaultdb> SHOW EXTERNAL CONNECTIONS;
  connection_name |     connection_uri      | connection_type
------------------+-------------------------+------------------
  test            | nodelocal://1/alterguo  | STORAGE
  test1           | nodelocal://1/alterguo2 | STORAGE
(2 rows)

Time: 3ms total (execution 3ms / network 1ms)

root@localhost:26257/defaultdb> ALTER EXTERNAL CONNECTION IF EXISTS test2 AS 'nodelocal://1/alterguo2';
ALTER EXTERNAL CONNECTION

Time: 2ms total (execution 2ms / network 0ms)

root@localhost:26257/defaultdb> SHOW EXTERNAL CONNECTIONS;
  connection_name |     connection_uri      | connection_type
------------------+-------------------------+------------------
  test            | nodelocal://1/alterguo  | STORAGE
  test1           | nodelocal://1/alterguo2 | STORAGE
(2 rows)

Time: 2ms total (execution 2ms / network 0ms)

root@localhost:26257/defaultdb> ALTER EXTERNAL CONNECTION test2 AS 'nodelocal://1/alterguo2';
ERROR: external connection with name test2 does not exist
```


Co-authored-by: Edward Guo <[email protected]>

Author: craig[bot]

pkg/cloud/externalconn/record.go

@@ -72,14 +72,14 @@ func NewExternalConnection(connDetails connectionpb.ConnectionDetails) ExternalC
 	return ec
 }
 
-// externalConnectionNotFoundError is returned from load when the external
+// ExternalConnectionNotFoundError is returned from load when the external
 // connection does not exist.
-type externalConnectionNotFoundError struct {
+type ExternalConnectionNotFoundError struct {
 	connectionName string
 }
 
 // Error makes scheduledJobNotFoundError an error.
-func (e *externalConnectionNotFoundError) Error() string {
+func (e *ExternalConnectionNotFoundError) Error() string {
 	return fmt.Sprintf("external connection with name %s does not exist", e.connectionName)
 }
 
@@ -97,10 +97,10 @@ func LoadExternalConnection(
 		fmt.Sprintf("SELECT * FROM system.external_connections WHERE connection_name = '%s'", name))
 
 	if err != nil {
-		return nil, errors.CombineErrors(err, &externalConnectionNotFoundError{connectionName: name})
+		return nil, errors.CombineErrors(err, &ExternalConnectionNotFoundError{connectionName: name})
 	}
 	if row == nil {
-		return nil, &externalConnectionNotFoundError{connectionName: name}
+		return nil, &ExternalConnectionNotFoundError{connectionName: name}
 	}
 
 	ec := NewMutableExternalConnection()
@@ -354,6 +354,26 @@ func (e *MutableExternalConnection) Create(ctx context.Context, txn isql.Txn) er
 	return e.InitFromDatums(row, retCols)
 }
 
+func (e *MutableExternalConnection) Update(ctx context.Context, txn isql.Txn) error {
+	cols, qargs, err := e.marshalChanges()
+	if err != nil {
+		return err
+	}
+
+	var setClauses []string
+	for i, col := range cols {
+		setClauses = append(setClauses, fmt.Sprintf("%s = $%d", col, i+1))
+	}
+
+	updateQuery := fmt.Sprintf(`UPDATE system.external_connections 
+															SET %s, updated = now() 
+		                          WHERE connection_name = '%s'`,
+		strings.Join(setClauses, ", "), e.ConnectionName())
+
+	_, err = txn.ExecEx(ctx, "ExternalConnection.Update", txn.KV(), sessiondata.NodeUserSessionDataOverride, updateQuery, qargs...)
+	return err
+}
+
 // marshalChanges marshals all changes in the in-memory representation and returns
 // the names of the columns and marshaled values.
 func (e *MutableExternalConnection) marshalChanges() ([]string, []interface{}, error) {

pkg/sql/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
         "alter_column_type.go",
         "alter_database.go",
         "alter_default_privileges.go",
+        "alter_external_connection.go",
         "alter_function.go",
         "alter_index.go",
         "alter_index_visible.go",

pkg/sql/alter_external_connection.go

@@ -0,0 +1,105 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package sql
+
+import (
+	"context"
+
+	"github.com/cockroachdb/cockroach/pkg/cloud/externalconn"
+	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
+	"github.com/cockroachdb/errors"
+)
+
+const alterExternalConnectionOp = "ALTER EXTERNAL CONNECTION"
+
+type alterExternalConnectionNode struct {
+	zeroInputPlanNode
+	n *tree.AlterExternalConnection
+}
+
+func (p *planner) AlterExternalConnection(
+	ctx context.Context, n *tree.AlterExternalConnection,
+) (planNode, error) {
+	return &alterExternalConnectionNode{n: n}, nil
+}
+
+func (alt *alterExternalConnectionNode) startExec(params runParams) error {
+	return params.p.alterExternalConnection(params, alt.n)
+}
+
+func (p *planner) alterExternalConnection(params runParams, n *tree.AlterExternalConnection) error {
+	txn := p.InternalSQLTxn()
+
+	exprEval := p.ExprEvaluator(alterExternalConnectionOp)
+	name, err := exprEval.String(params.ctx, n.ConnectionLabelSpec.Label)
+	if err != nil {
+		return err
+	}
+	endpoint, err := exprEval.String(params.ctx, n.As)
+	if err != nil {
+		return err
+	}
+
+	ecPrivilege := &syntheticprivilege.ExternalConnectionPrivilege{
+		ConnectionName: name,
+	}
+	if err := p.CheckPrivilege(params.ctx, ecPrivilege, privilege.UPDATE); err != nil {
+		return err
+	}
+
+	existingConn, err := externalconn.LoadExternalConnection(params.ctx, name, txn)
+	var notFoundErr *externalconn.ExternalConnectionNotFoundError
+	if err != nil {
+		if errors.As(err, &notFoundErr) && n.IfExists {
+			return nil
+		}
+		return err
+	}
+
+	ex, ok := existingConn.(*externalconn.MutableExternalConnection)
+	if !ok {
+		return errors.AssertionFailedf("Failed to cast externalConnection (%s) to MutableExtneralConnection type", existingConn.ConnectionName())
+	}
+
+	if err = logAndSanitizeExternalConnectionURI(params.ctx, endpoint); err != nil {
+		return errors.Wrap(err, "failed to log and santitize External Connection")
+	}
+
+	env := externalconn.MakeExternalConnEnv(
+		params.ExecCfg().Settings,
+		&params.ExecCfg().ExternalIODirConfig,
+		params.ExecCfg().InternalDB,
+		p.User(),
+		params.ExecCfg().DistSQLSrv.ExternalStorageFromURI,
+		false,
+		false,
+		&params.ExecCfg().DistSQLSrv.ServerConfig,
+	)
+
+	alterConn, err := externalconn.ExternalConnectionFromURI(
+		params.ctx, env, endpoint,
+	)
+	if err != nil {
+		return errors.Wrap(err, "failed to construct External Connection details")
+	}
+
+	ex.SetConnectionDetails(*alterConn.ConnectionProto())
+
+	if err := ex.Update(params.ctx, txn); err != nil {
+		return errors.Wrap(err, "failed to alter external connection")
+	}
+
+	return nil
+
+}
+
+func (alt *alterExternalConnectionNode) Close(_ context.Context) {}
+
+func (alt *alterExternalConnectionNode) Next(params runParams) (bool, error) { return false, nil }
+
+func (alt *alterExternalConnectionNode) Values() tree.Datums { return nil }

pkg/sql/logictest/testdata/logic_test/alter_external_connection

@@ -0,0 +1,60 @@
+# LogicTest: local
+
+statement ok
+CREATE EXTERNAL CONNECTION conn_1 AS 'nodelocal://1/conn_1';
+CREATE EXTERNAL CONNECTION conn_2 AS 'nodelocal://1/conn_2';
+
+query TTT colnames,rowsort
+SHOW EXTERNAL CONNECTIONS
+----
+connection_name connection_uri       connection_type
+conn_1          nodelocal://1/conn_1 STORAGE
+conn_2          nodelocal://1/conn_2 STORAGE
+
+
+user testuser
+
+statement error pq: user testuser does not have UPDATE privilege on external_connection conn_1
+ALTER EXTERNAL CONNECTION conn_1 AS 'nodelocal://1/conn_update';
+
+user root
+
+statement ok
+GRANT UPDATE ON EXTERNAL CONNECTION conn_1 TO testuser;
+GRANT USAGE ON EXTERNAL CONNECTION conn_1 TO testuser;
+
+user testuser
+
+statement ok
+ALTER EXTERNAL CONNECTION conn_1 AS 'nodelocal://1/conn_update_with_privilege';
+
+query TTT colnames
+SHOW EXTERNAL CONNECTION conn_1
+----
+connection_name connection_uri                connection_type
+conn_1   nodelocal://1/conn_update_with_privilege STORAGE
+
+
+
+statement error pq: user testuser does not have UPDATE privilege on external_connection conn_2
+ALTER EXTERNAL CONNECTION conn_2 AS 'nodelocal://1/conn_update';
+
+user root
+
+statement ok
+GRANT UPDATE ON EXTERNAL CONNECTION conn_2 TO testuser;
+GRANT USAGE ON EXTERNAL CONNECTION conn_2 TO testuser;
+
+user testuser
+
+statement error pq: user testuser does not have UPDATE privilege on external_connection conn_not_exist
+ALTER EXTERNAL CONNECTION IF EXISTS conn_not_exist AS 'nodelocal://1/not_exist';
+
+statement ok
+ALTER EXTERNAL CONNECTION conn_2 AS 'nodelocal://1/connection_2_alter';
+
+query TTT colnames
+SHOW EXTERNAL CONNECTION conn_2
+----
+connection_name connection_uri                   connection_type
+conn_2          nodelocal://1/connection_2_alter STORAGE

pkg/sql/logictest/testdata/logic_test/external_connection_privileges

@@ -15,57 +15,61 @@ GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser
 statement error pq: failed to resolve External Connection: external connection with name foo does not exist
 GRANT DROP ON EXTERNAL CONNECTION foo TO testuser
 
+
+statement error pq: failed to resolve External Connection: external connection with name foo does not exist
+GRANT UPDATE ON EXTERNAL CONNECTION foo TO testuser
+
 statement ok
 CREATE EXTERNAL CONNECTION foo AS 'nodelocal://1/foo'
 
 statement ok
-GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser
+GRANT USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo TO testuser
 
 query TTTT
 SELECT username, path, privileges, grant_options FROM system.privileges ORDER by username
 ----
 root      /externalconn/foo  {ALL}         {}
-testuser  /externalconn/foo  {DROP,USAGE}  {}
+testuser  /externalconn/foo  {DROP,UPDATE,USAGE}  {}
 
 statement ok
-REVOKE USAGE,DROP ON EXTERNAL CONNECTION foo FROM testuser
+REVOKE USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo FROM testuser
 
 query TTTT
 SELECT username, path, privileges, grant_options FROM system.privileges ORDER by username
 ----
 root  /externalconn/foo  {ALL}  {}
 
 statement ok
-GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser
+GRANT USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo TO testuser
 
 statement ok
 CREATE USER bar
 
 # Attempt to grant usage as testuser, this should fail since we did not specify WITH GRANT OPTION
 user testuser
 
-statement error pq: user testuser missing WITH GRANT OPTION privilege on one or more of USAGE, DROP
-GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO bar
+statement error pq: user testuser missing WITH GRANT OPTION privilege on one or more of USAGE, DROP, UPDATE
+GRANT USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo TO bar
 
 user root
 
 statement ok
-GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO testuser WITH GRANT OPTION
+GRANT USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo TO testuser WITH GRANT OPTION
 
 # Attempt to grant usage as testuser, this should succeed since we did specify WITH GRANT OPTION
 user testuser
 
 statement ok
-GRANT USAGE,DROP ON EXTERNAL CONNECTION foo TO bar
+GRANT USAGE,DROP,UPDATE ON EXTERNAL CONNECTION foo TO bar
 
 user root
 
 query TTTT
 SELECT username, path, privileges, grant_options FROM system.privileges ORDER BY username
 ----
-bar       /externalconn/foo  {DROP,USAGE}  {}
+bar       /externalconn/foo  {DROP,UPDATE,USAGE}  {}
 root      /externalconn/foo  {ALL}         {}
-testuser  /externalconn/foo  {DROP,USAGE}  {DROP,USAGE}
+testuser  /externalconn/foo  {DROP,UPDATE,USAGE}  {DROP,UPDATE,USAGE}
 
 # Invalid grants on external connections.
 
@@ -79,25 +83,30 @@ statement ok
 CREATE ROLE testuser2
 
 statement ok
-GRANT DROP, USAGE ON EXTERNAL CONNECTION foo TO testuser2 WITH GRANT OPTION
+GRANT DROP,UPDATE,USAGE ON EXTERNAL CONNECTION foo TO testuser2 WITH GRANT OPTION
 
 query TTTB colnames,rowsort
 SHOW GRANTS ON EXTERNAL CONNECTION foo
 ----
 connection_name  grantee    privilege_type  is_grantable
 foo              bar        DROP            false
+foo              bar        UPDATE          false
 foo              bar        USAGE           false
 foo              root       ALL             false
 foo              testuser   DROP            true
+foo              testuser   UPDATE          true
 foo              testuser   USAGE           true
 foo              testuser2  DROP            true
+foo              testuser2  UPDATE          true
 foo              testuser2  USAGE           true
 
 query TTTB colnames,rowsort
 SHOW GRANTS ON EXTERNAL CONNECTION foo FOR testuser, testuser2
 ----
 connection_name  grantee    privilege_type  is_grantable
 foo              testuser   DROP            true
+foo              testuser   UPDATE          true
 foo              testuser   USAGE           true
 foo              testuser2  DROP            true
+foo              testuser2  UPDATE          true
 foo              testuser2  USAGE           true

pkg/sql/logictest/tests/local/generated_test.go

@@ -100,6 +100,8 @@ func planOpaque(ctx context.Context, p *planner, stmt tree.Statement) (planNode,
 		return p.AlterDatabaseSetZoneConfigExtension(ctx, n)
 	case *tree.AlterDefaultPrivileges:
 		return p.alterDefaultPrivileges(ctx, n)
+	case *tree.AlterExternalConnection:
+		return p.AlterExternalConnection(ctx, n)
 	case *tree.AlterFunctionOptions:
 		return p.AlterFunctionOptions(ctx, n)
 	case *tree.AlterRoutineRename:
@@ -371,6 +373,7 @@ func init() {
 		&tree.CreateDatabase{},
 		&tree.CreateExtension{},
 		&tree.CreateExternalConnection{},
+		&tree.AlterExternalConnection{},
 		&tree.CreateTenant{},
 		&tree.CreateIndex{},
 		&tree.CreatePolicy{},

pkg/sql/opaque.go

@@ -58,6 +58,7 @@ var planNodeNames = map[reflect.Type]string{
 	reflect.TypeOf(&alterDatabaseDropSecondaryRegion{}):        "alter database secondary region",
 	reflect.TypeOf(&alterDatabaseSetZoneConfigExtensionNode{}): "alter database configure zone extension",
 	reflect.TypeOf(&alterDefaultPrivilegesNode{}):              "alter default privileges",
+	reflect.TypeOf(&alterExternalConnectionNode{}):             "alter external connection",
 	reflect.TypeOf(&alterFunctionOptionsNode{}):                "alter function",
 	reflect.TypeOf(&alterFunctionRenameNode{}):                 "alter function rename",
 	reflect.TypeOf(&alterFunctionSetOwnerNode{}):               "alter function owner",

pkg/sql/plan_names.go

@@ -103,7 +103,7 @@ var (
 		REPAIRCLUSTER, BYPASSRLS, REPLICATIONDEST, REPLICATIONSOURCE,
 	}
 	VirtualTablePrivileges       = List{ALL, SELECT}
-	ExternalConnectionPrivileges = List{ALL, USAGE, DROP}
+	ExternalConnectionPrivileges = List{ALL, USAGE, DROP, UPDATE}
 )
 
 // Mask returns the bitmask for a given privilege.