AWS Connection Implementation #23282

moulimukherjee · 2023-11-17T17:46:34Z

Motivation

Implementation for https://github.com/MaterializeInc/database-issues/issues/6945
Depends upon https://github.com/MaterializeInc/cloud/pull/8224

Tips for reviewer

Comments inline.

Split out a PR to just accept the new environmentd cli arg #23626, in case the current PR is delayed. Will rebase once that's merged.

This PR is making backwards incompatible changes to the CREATE CONNECTION SQL for AWS connection. This has been put behind a feature flag and nobody's using it yet, so it should be fine.

Currently changes are still required on the cloud's side to pass a new AWS role arn to environmentd https://github.com/MaterializeInc/cloud/pull/8224. I am trying to get the materialize changes in before I go on a vacation next week.

I will create a separate docs PR when this goes to private preview.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
- Rendered design doc
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- No user facing changes yet. This feature has been put behind a feature flag, and nobody's using it.

cc @benesch @jubrad @alex-hunt-materialize @hlburak

moulimukherjee · 2023-12-01T19:14:52Z

src/environmentd/src/bin/environmentd/main.rs

+    #[clap(long, env = "AWS_EXTERNAL_CONNECTION_ROLE")]
+    aws_external_connection_role: Option<String>,


@jubrad Using AWS_EXTERNAL_CONNECTION_ROLE to pass the new role to assume. Lmk if you would prefer a different name.

shepherdlybot · 2023-12-01T19:17:03Z

Mitigations

Completing required mitigations increases Resilience Coverage.

Bug Hotspots:

What's This?

File	Percentile
../src/builtin.rs	96
../catalog/builtin_table_updates.rs	97
../src/parser.rs	97
../src/clusters.rs	91

jkosh44

Adapter/SQL changes LGTM.

src/adapter/src/catalog/config.rs

src/sql/src/plan/statement/ddl/connection.rs

benesch · 2023-12-04T18:57:06Z

Gonna take a look at this by EOD!

src/adapter/src/catalog/builtin_table_updates.rs

src/sql/src/catalog.rs

src/sql/src/plan/statement/ddl/connection.rs

src/adapter/src/catalog/config.rs

test/testdrive/mzcompose.py

I'd like to standardize on calling these "AWS connections" everywhere possible, without bringing "external" into the picture. "External" came from "external ID", but that's a standalone term of art, and I think it's clearer to call the role here just the "AWS connection role". (If anything, the "external connection role" seems like the customer supplied role, not our internal intermediary role.) Also unplumb the argument from the coordinator, since as mentioned in the review for MaterializeInc#23282 I think we'll want to instead plumb this argument through the ConnectionContext.

benesch

I pushed up a big batch of changes. This looks nearly mergeable. Outstanding blockers:

Fixing the SQL option parsing enum
Manual testing on staging

Other nice to haves:

Localstack tests for IAM (mis)configurations
Reference documentation for mz_aws_connections

src/storage-types/src/connections/aws.proto

benesch · 2023-12-12T07:10:49Z

src/adapter/src/catalog/builtin_table_updates.rs

+            Datum::from(assume_role_session_name),
+            Datum::from(principal),
+            Datum::from(external_id.as_deref()),
+            Datum::from(example_trust_policy.as_ref().map(|p| p.into_element())),


@guswynn I golfed this a little more to get rid of the holder entirely.

benesch · 2023-12-12T07:11:26Z

src/sql/src/plan/error.rs

@@ -231,6 +232,23 @@ pub enum PlanError {
    // TODO(benesch): eventually all errors should be structured.
    Unstructured(String),
 }
+#[derive(Clone, Debug)]
+pub enum ConnectionParsingError {


Why is this a separate type? We don't have any other example of splitting out PlanError within this crate.

benesch · 2023-12-12T07:11:44Z

src/sql/src/plan/error.rs

+            Self::ConflictingOptions => f.write_str("invalid CONNECTION: ASSUME ROLE ARN cannot be provided simultaneously with ACCESS KEY ID and SECRET ACCESS KEY"),
+            Self::MissingRequiredOptions => f.write_str("invalid CONNECTION: must specify either ASSUME ROLE ARN or both ACCESS KEY ID and SECRET ACCESS KEY"),


These two options have AWS specific error messages but the variants don't indicate that they are AWS specific.

benesch · 2023-12-12T07:13:04Z

test/aws/mzcompose.py

@@ -28,9 +28,11 @@
 NAMESPACE = ENVIRONMENT_NAME


I renamed this composition to "aws" to reflect its new purpose as a general test of AWS-related functionality.

benesch · 2023-12-12T07:14:44Z

src/storage-types/src/connections/aws.rs

+            }
+        }
+
+        Ok(())
    }

    pub(crate) fn validate_by_default(&self) -> bool {


I substantially refactored the code in this file to have more comments, produce better error messages (e.g., there were several instances of "error!" with no additional context in user-facing error messages), and to reduce duplication.

philip-stoev

Given that this introduces a new DDL syntax, it needs a misc/python/materialize/checks/all_checks/* test, to guard against Mz restarts and upgrades . Consider taking an existing test on secrets from that directory and cloning it a bit.

philip-stoev

Given that this introduces a new DDL syntax, it needs a misc/python/materialize/checks/all_checks/* test, to guard against Mz restarts and upgrades . Consider taking an existing test on secrets from that directory and cloning it a bit.

guswynn · 2023-12-12T18:51:12Z

pushing a not-quite-finished commit to test something

benesch

@guswynn I can't figure out why the AWS SDK isn't automatically figuring out the region? Do you have a reference for that?

It looks like maybe something got fixed in the latest version of the AWS SDK around regions and AssumeRole? Eyeballing smithy-lang/smithy-rs#3014 and it seems like it could definitely help us.

benesch · 2023-12-13T08:24:39Z

src/storage-types/src/connections/aws.rs

@@ -231,6 +241,9 @@ impl AwsAssumeRole {
        if let Some(external_id) = external_id {
            credentials = credentials.external_id(external_id);
        }
+        if let Some(region) = region {
+            credentials = credentials.region(region);
+        }


And especially spooked that we need to override the region in the AssumeRole builder. This feels like something is wrong!

I believe the first one on line 226 is required, but I agree that its cursed (perhaps fixed by the pr you linked above?)

I followed https://github.com/MaterializeInc/materialize/blob/main/src/persist/src/s3.rs#L134-L137, and based my understanding on seeing this: https://docs.rs/aws-config/0.55.1/src/aws_config/lib.rs.html#563-567

Its possible the second AssumeRoleProdider doesn't need it, do you want me to try that?

pushed a commit removing this second one to test this, after the build finishes ill test in staging

rip, its required

You didn't bump the AWS SDK version did you?

@benesch I didn't, because the next bump is from 0.x to 1.x and I don't want to block this pr on that change

benesch · 2023-12-13T08:24:49Z

src/storage-types/src/connections/aws.rs

@@ -341,14 +358,18 @@ impl AwsConnection {
        // case of failure.
        let _ = sts_client.get_caller_identity().send().await?;

+        let region = match &self.region {
+            Some(region_name) => Some(Region::new(region_name.clone())),
+            None => region::default_provider().region().await,


I'm a bit spooked that this was necessary

Changes to accept either credentials or assume role proto implement validate fix tests role chaining Fix validate connection Added table aws_connections cloud test Adding more columns to mz_aws_connections from updated design temp refactor Fix the access_key_id_secret_id Fixing the assume role validation Adding testdrive tests Added TODO Getting materialize external connection role from environmentd Added tests for failure scenarios Formatting docs python lint update design doc Fixing proto lint Adding #[serde(skip)] to aws_principal_context in catalog state Adding Deserialize trait update comment Fix tests

Co-authored-by: Nikhil Benesch <[email protected]> Removing mz_principal

revert Refactor aws connection Moving tests to secret-aws-secret-manager specifying column names Refactor parsing errors clippy fix Added TODOs to restructure errors revert not required changes fix test

guswynn · 2023-12-13T22:37:57Z

@philip-stoev pushed a commit with a basic check for these connections and their syntax

benesch · 2023-12-14T07:40:42Z

I didn't get a chance to do the AWS SDK region upgrade as I hoped, so I'm good with proceeding with this as is for now.

resolved

moulimukherjee · 2023-12-28T05:56:35Z

Thanks for all the efforts and getting this merged 🙏, this would have been deep in merge conflict land otherwise.

moulimukherjee force-pushed the aws-connections branch 2 times, most recently from 0226f3f to 2121c83 Compare November 22, 2023 00:46

moulimukherjee force-pushed the aws-connections branch 3 times, most recently from 9b4f8fa to 8734297 Compare November 30, 2023 16:45

moulimukherjee changed the title ~~WIP: AWS Connection~~ AWS Connection Implementation Dec 1, 2023

moulimukherjee commented Dec 1, 2023

View reviewed changes

moulimukherjee marked this pull request as ready for review December 1, 2023 19:15

moulimukherjee requested a review from a team December 1, 2023 19:15

moulimukherjee requested a review from a team as a code owner December 1, 2023 19:15

moulimukherjee requested review from a team and maddyblue December 1, 2023 19:15

moulimukherjee added the T-proto Theme: `$T ⇔ Proto$T` conversions and `*.proto` files label Dec 1, 2023

moulimukherjee requested review from benesch and jubrad December 1, 2023 19:39

dehume mentioned this pull request Dec 1, 2023

AWS Connection MaterializeInc/terraform-provider-materialize#395

Closed

moulimukherjee mentioned this pull request Dec 1, 2023

adapter: Changes to get aws_external_connection_role from args #23626

Merged

5 tasks

moulimukherjee force-pushed the aws-connections branch 3 times, most recently from 4631ad9 to c609e09 Compare December 2, 2023 00:25

moulimukherjee self-assigned this Dec 2, 2023

jkosh44 requested review from jkosh44 and removed request for maddyblue December 4, 2023 16:13

jkosh44 approved these changes Dec 4, 2023

View reviewed changes

src/adapter/src/catalog/config.rs Outdated Show resolved Hide resolved

src/sql/src/plan/statement/ddl/connection.rs Outdated Show resolved Hide resolved

moulimukherjee force-pushed the aws-connections branch from c609e09 to a5de426 Compare December 4, 2023 18:48

benesch reviewed Dec 5, 2023

View reviewed changes

benesch requested a review from sploiselle December 5, 2023 07:27

benesch reviewed Dec 12, 2023

View reviewed changes

philip-stoev requested review from philip-stoev and removed request for a team December 12, 2023 08:34

philip-stoev suggested changes Dec 12, 2023

View reviewed changes

philip-stoev previously requested changes Dec 12, 2023

View reviewed changes

benesch force-pushed the aws-connections branch from 4452da2 to e1f493a Compare December 12, 2023 15:52

benesch reviewed Dec 13, 2023

View reviewed changes

Mouli Mukherjee and others added 7 commits December 13, 2023 11:18

Apply suggestions from code review

be12d2a

Co-authored-by: Nikhil Benesch <[email protected]> Removing mz_principal

Passing aws_connection_role_arn to clusterd

1f2d059

revert Refactor aws connection Moving tests to secret-aws-secret-manager specifying column names Refactor parsing errors clippy fix Added TODOs to restructure errors revert not required changes fix test

Fixing slt tests

cc2ec89

adapter: simplify pack_aws_connection_update

afa360b

sql,storage-types: address review feedback on AWS connections

b5ebb76

storage: provide region correctly when assuming role

efc1fd5

guswynn force-pushed the aws-connections branch from f1605f7 to efc1fd5 Compare December 13, 2023 19:25

benesch approved these changes Dec 14, 2023

View reviewed changes

guswynn force-pushed the aws-connections branch from 748e730 to f21813e Compare December 14, 2023 16:48

guswynn added the release-blocker Critical issue that should block *any* release if not fixed label Dec 14, 2023

guswynn force-pushed the aws-connections branch from f21813e to 1467fae Compare December 14, 2023 17:12

guswynn enabled auto-merge (rebase) December 14, 2023 17:17

misc: add basic AwsConnection syntax check

587223b

guswynn force-pushed the aws-connections branch from 1467fae to 587223b Compare December 14, 2023 17:40

guswynn merged commit 1f8dbd7 into MaterializeInc:main Dec 14, 2023

moulimukherjee deleted the aws-connections branch December 27, 2023 16:44

moulimukherjee mentioned this pull request Jan 16, 2024

adapter: Copy to S3 adapter changes #24355

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Connection Implementation #23282

AWS Connection Implementation #23282

moulimukherjee commented Nov 17, 2023 •

edited

Loading

moulimukherjee Dec 1, 2023 •

edited

Loading

shepherdlybot bot commented Dec 1, 2023 •

edited

Loading

jkosh44 left a comment

benesch commented Dec 4, 2023

benesch left a comment •

edited

Loading

benesch Dec 12, 2023

benesch Dec 12, 2023

benesch Dec 12, 2023

benesch Dec 12, 2023

benesch Dec 12, 2023

philip-stoev left a comment

philip-stoev left a comment

guswynn commented Dec 12, 2023

benesch left a comment

benesch Dec 13, 2023

guswynn Dec 13, 2023

guswynn Dec 13, 2023

guswynn Dec 13, 2023

benesch Dec 13, 2023

guswynn Dec 13, 2023

benesch Dec 13, 2023

guswynn commented Dec 13, 2023

benesch commented Dec 14, 2023

moulimukherjee commented Dec 28, 2023

		#[clap(long, env = "AWS_EXTERNAL_CONNECTION_ROLE")]
		aws_external_connection_role: Option<String>,

		Self::ConflictingOptions => f.write_str("invalid CONNECTION: ASSUME ROLE ARN cannot be provided simultaneously with ACCESS KEY ID and SECRET ACCESS KEY"),
		Self::MissingRequiredOptions => f.write_str("invalid CONNECTION: must specify either ASSUME ROLE ARN or both ACCESS KEY ID and SECRET ACCESS KEY"),

AWS Connection Implementation #23282

AWS Connection Implementation #23282

Conversation

moulimukherjee commented Nov 17, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

moulimukherjee Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

shepherdlybot bot commented Dec 1, 2023 • edited Loading

Mitigations

jkosh44 left a comment

Choose a reason for hiding this comment

benesch commented Dec 4, 2023

benesch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philip-stoev left a comment

Choose a reason for hiding this comment

philip-stoev left a comment

Choose a reason for hiding this comment

guswynn commented Dec 12, 2023

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guswynn commented Dec 13, 2023

benesch commented Dec 14, 2023

moulimukherjee commented Dec 28, 2023

moulimukherjee commented Nov 17, 2023 •

edited

Loading

moulimukherjee Dec 1, 2023 •

edited

Loading

shepherdlybot bot commented Dec 1, 2023 •

edited

Loading

benesch left a comment •

edited

Loading