Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
53974: sql: improve error message for search_path with commas r=jordanlewis a=jordanlewis

It's easy to accidentally surround your search path with quotes when
setting it, because you'd think that most things in `SET` syntax are
quoted. But you are not supposed to quote things in set search_path, and
it can lead to confusing scenarios. Now, if you try to set search_path
to a string containing a comma, which we don't support anyway, the error
message will be a bit friendlier.

Release note (sql change): improve error message when people use set
search_path incorrectly, or with a schema that legitimately has a comma
in its name

Release justification: error-message-only change

57519: auth: add region-based callback URLs for OIDC r=dhartunian a=dhartunian

Modifies `server.oidc_authentication.redirect_url` cluster setting to
accept valid JSON strings with a `redirect_urls` field that can support
region-based OIDC auth flows.

In addition to a simple string callback URL, here is an example of
valid JSON that this setting can accept:

```
'{
  "redirect_urls": {
    "us-east-1": "https://localhost:8080/oidc/v1/callback",
    "eu-west-1": "example.com"
  }
}'
```

Prerequisites to using the multi-region callback URLs:
1. `region` locality flag is available and set
2. `server.oidc_authentication.redirect_url` setting is set as valid
JSON containing the `redirect_urls` object with a key that matches the
`region` locality flag value on this node

When prerequisites above are met, the `callback_uri` OAuth param is set
to the region-specific value from the JSON setting upon redirect to the
auth provider.

If you are using region-specific configuration, and do not have the
`region` locality set, or try using OIDC in a region without a
corresponding entry in the JSON, OIDC will fail to run.

If you are using simple string-based configuration of a single redirect
URL, OIDC will always use it regardless of your region locality
configuration.

Be aware that the auth provider will likely need to be updated to know
about all possible redirect URLs it may get triggered with.

Resolves #56517

Release note (security update): Adds ability to set region-specific
callback URLs in the OIDC config. The
`server.oidc_authentication.redirect_url` cluster setting can now accept
JSON as an alternative to the basic URL string setting. If a JSON value
is set, it *must* contain a `redirect_url` key that maps to an object
with key, value pairs where the key is a `region` matching an existing
locality setting, and the value is a callback URL.

59256: sql: Implement ALTER TABLE from REGIONAL BY TABLE to GLOBAL r=arulajmani,otan a=ajstorm

Implement ALTER TABLE ... SET LOCALITY GLOBAL for tables starting as
REGIONAL BY ROW.

Release note (sql change): Implement ALTER TABLE ... SET LOCALITY GLOBAL
for tables starting as REGIONAL BY ROW.

Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Adam Storm <[email protected]>
  • Loading branch information
4 people committed Jan 22, 2021
4 parents 23efbe8 + 73d3246 + 32e1c77 + 5898f27 commit 3593655
Show file tree
Hide file tree
Showing 14 changed files with 610 additions and 176 deletions.
2 changes: 1 addition & 1 deletion docs/generated/settings/settings.html
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
<tr><td><code>server.oidc_authentication.enabled</code></td><td>boolean</td><td><code>false</code></td><td>enables or disabled OIDC login for the DB Console (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.principal_regex</code></td><td>string</td><td><code>(.+)</code></td><td>regular expression to apply to extracted principal (see claim_json_key setting) to translate to SQL user (golang regex format, must include 1 grouping to extract) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.provider_url</code></td><td>string</td><td><code></code></td><td>sets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.redirect_url</code></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>sets OIDC redirect URL (base HTTP URL, likely your load balancer, must route to the path /oidc/v1/callback) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.redirect_url</code></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>sets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.scopes</code></td><td>string</td><td><code>openid</code></td><td>sets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`) (this feature is experimental)</td></tr>
<tr><td><code>server.rangelog.ttl</code></td><td>duration</td><td><code>720h0m0s</code></td><td>if nonzero, range log entries older than this duration are deleted every 10m0s. Should not be lowered below 24 hours.</td></tr>
<tr><td><code>server.remote_debugging.mode</code></td><td>string</td><td><code>local</code></td><td>set to enable remote debugging, localhost-only or disable (any, local, off)</td></tr>
Expand Down
91 changes: 86 additions & 5 deletions pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,36 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

statement error unimplemented: implementation pending
statement ok
ALTER TABLE regional_by_table_in_primary_region SET LOCALITY GLOBAL

query TT
SHOW CREATE TABLE regional_by_table_in_primary_region
----
regional_by_table_in_primary_region CREATE TABLE public.regional_by_table_in_primary_region (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY GLOBAL

query TT
SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_in_primary_region
----
TABLE regional_by_table_in_primary_region ALTER TABLE regional_by_table_in_primary_region CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# Drop the table and recreate it to get back to original state (this is required because alter table
# from global to regional by table is not yet implemented).
statement ok
DROP TABLE regional_by_table_in_primary_region

statement ok
CREATE TABLE regional_by_table_in_primary_region (i int) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION

statement error unimplemented: implementation pending
ALTER TABLE regional_by_table_in_primary_region SET LOCALITY REGIONAL BY ROW

Expand All @@ -347,7 +374,7 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# drop and recreate the table to get it back to the "no region" state
# Drop and recreate the table to get it back to the "no region" state.
statement ok
DROP TABLE regional_by_table_no_region

Expand Down Expand Up @@ -376,7 +403,7 @@ TABLE regional_by_table_no_region ALTER TABLE regional_by_table_no_region CONFI
constraints = '{+region=ap-southeast-2: 3}',
lease_preferences = '[[+region=ap-southeast-2]]'

# drop and recreate the table to get it back to the "no region" state
# Drop and recreate the table to get it back to the "no region" state.
statement ok
DROP TABLE regional_by_table_no_region

Expand Down Expand Up @@ -405,9 +432,36 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

statement error unimplemented: implementation pending
statement ok
ALTER TABLE regional_by_table_no_region SET LOCALITY GLOBAL

query TT
SHOW CREATE TABLE regional_by_table_no_region
----
regional_by_table_no_region CREATE TABLE public.regional_by_table_no_region (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY GLOBAL

query TT
SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_no_region
----
TABLE regional_by_table_no_region ALTER TABLE regional_by_table_no_region CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# Drop the table and recreate it to get back to original state (this is required because alter table
# from global to regional by table is not yet implemented).
statement ok
DROP TABLE regional_by_table_no_region

statement ok
CREATE TABLE regional_by_table_no_region (i int) LOCALITY REGIONAL BY TABLE

statement error unimplemented: implementation pending
ALTER TABLE regional_by_table_no_region SET LOCALITY REGIONAL BY ROW

Expand Down Expand Up @@ -483,8 +537,35 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

statement error unimplemented: implementation pending
statement ok
ALTER TABLE regional_by_table_in_us_east SET LOCALITY GLOBAL

query TT
SHOW CREATE TABLE regional_by_table_in_us_east
----
regional_by_table_in_us_east CREATE TABLE public.regional_by_table_in_us_east (
i INT8 NULL,
FAMILY "primary" (i, rowid)
) LOCALITY GLOBAL

query TT
SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_in_us_east
----
TABLE regional_by_table_in_us_east ALTER TABLE regional_by_table_in_us_east CONFIGURE ZONE USING
range_min_bytes = 134217728,
range_max_bytes = 536870912,
gc.ttlseconds = 90000,
num_replicas = 3,
constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}',
lease_preferences = '[[+region=ca-central-1]]'

# Drop the table and recreate it to get back to original state (this is required because alter table
# from global to regional by table is not yet implemented).
statement ok
DROP TABLE regional_by_table_in_us_east

statement ok
CREATE TABLE regional_by_table_in_us_east (i int) LOCALITY REGIONAL BY TABLE IN "us-east-1"

statement error unimplemented: implementation pending
ALTER TABLE regional_by_table_in_us_east SET LOCALITY REGIONAL BY ROW
6 changes: 5 additions & 1 deletion pkg/ccl/oidcccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/ccl/utilccl",
"//pkg/roachpb",
"//pkg/server",
"//pkg/server/serverpb",
"//pkg/server/telemetry",
Expand All @@ -30,7 +31,10 @@ go_library(

go_test(
name = "oidcccl_test",
srcs = ["authentication_oidc_test.go"],
srcs = [
"authentication_oidc_test.go",
"settings_test.go",
],
embed = [":oidcccl"],
deps = [
"//pkg/base",
Expand Down
Loading

0 comments on commit 3593655

Please sign in to comment.