Skip to content

Commit

Permalink
fix(forwarder): relax DataAccessPointArn requirement (#245)
Browse files Browse the repository at this point in the history
This commit relaxes DataAccessPointArn as a required parameter for the
Forwarder stack. The prior settings assumed the stack would always point
at Filedrop. In practice, there can be usecases where an access point
should not be assumed:
- if we point directly at an S3 bucket, for testing or aggregation
  purposes.
- if we point the forwarder towards an HTTP endpoint instead.
  • Loading branch information
jta authored May 9, 2024
1 parent e085724 commit 73713c0
Show file tree
Hide file tree
Showing 9 changed files with 146 additions and 38 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ build-Forwarder:
build-Subscriber:
APP=subscriber $(MAKE) build-App

# parameters: generate doc table for cloudformation parameters
## parameters: generate doc table for cloudformation parameters
parameters:
$(call check_var,APP)
@echo "| Parameter | Type | Description |"
Expand Down
69 changes: 48 additions & 21 deletions apps/forwarder/template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ Metadata:
- Label:
default: Filedrop Configuration
Parameters:
- DataAccessPointArn
- DestinationUri
- DataAccessPointArn
- NameOverride
- Label:
default: Data Sources
Expand All @@ -40,16 +40,18 @@ Globals:
MemorySize: 128

Parameters:
DataAccessPointArn:
DestinationUri:
Type: String
Description: >-
The access point ARN for your Filedrop.
AllowedPattern: "^arn:.*$"
DestinationUri:
The URI for your destination, e.g. `s3://bucket-alias/ds101/`. S3 URIs must
end in a forward slash.
AllowedPattern: "^(s3:\/\/.+\/|https:\/\/.+)$"
DataAccessPointArn:
Type: String
Description: >-
The S3 URI for your Filedrop, e.g. `s3://bucket-alias/ds101/`
AllowedPattern: "^(s3|https):\/\/.*"
The access point ARN for your Filedrop.
AllowedPattern: "^(arn:.*)?$"
Default: ''
NameOverride:
Type: String
Description: >-
Expand Down Expand Up @@ -100,7 +102,18 @@ Parameters:
Default: ''
AllowedPattern: "^(http(s)?:\/\/.*)?$"
Conditions:
IsMaxFileSizeEmpty: !Equals [ !Ref MaxFileSize, '' ]
NoDataAccessPointArn: !Equals
- !Ref DataAccessPointArn
- ''
HasS3Destination: !Equals
- !Select [ 0, !Split [ "://", !Ref DestinationUri ] ]
- 's3'
HasS3DestinationWithoutAccessPoint: !And
- !Condition HasS3Destination
- !Condition NoDataAccessPointArn
IsMaxFileSizeEmpty: !Equals
- !Ref MaxFileSize
- ''
DisableSourceS3: !Equals
- !Join
- ''
Expand Down Expand Up @@ -230,19 +243,33 @@ Resources:
- logs:CreateLogStream
- logs:PutLogEvents
Resource: !GetAtt LogGroup.Arn
- PolicyName: writer
PolicyDocument:
Version: 2012-10-17
Statement:
- Effect: Allow
Action:
- s3:PutObject
- s3:PutObjectTagging
Resource: "*"
Condition:
StringLike:
s3:DataAccessPointArn:
- !Ref DataAccessPointArn
- !If
- HasS3Destination
- PolicyName: writer
PolicyDocument:
Version: 2012-10-17
Statement:
- Effect: Allow
Action:
- s3:PutObject
- s3:PutObjectTagging
Resource: !If
- HasS3DestinationWithoutAccessPoint
# We enforce DestinationUri ends in a forward slash so as to ensure
# wildcard does not apply more broadly than intended.
- !Sub
- "arn:${AWS::Partition}:s3:::${Destination}*"
- Destination: !Select [ 1, !Split [ "s3://", !Ref DestinationUri ] ]
# If data access point is provided, we do not know the actual ARN.
# Restrict using a condition instead
- "*"
Condition: !If
- HasS3DestinationWithoutAccessPoint
- !Ref AWS::NoValue
- StringLike:
s3:DataAccessPointArn:
- !Ref DataAccessPointArn
- !Ref AWS::NoValue
- !If
- DisableSourceS3
- !Ref AWS::NoValue
Expand Down
18 changes: 10 additions & 8 deletions apps/stack/template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ Metadata:
AWS::CloudFormation::Interface:
ParameterGroups:
- Label:
default: Required parameters
default: Destination parameters
Parameters:
- DataAccessPointArn
- DestinationUri
- DataAccessPointArn
- Label:
default: AWS Config
Parameters:
Expand Down Expand Up @@ -48,16 +48,18 @@ Metadata:
- DebugEndpoint

Parameters:
DataAccessPointArn:
DestinationUri:
Type: String
Description: >-
The access point ARN for your Filedrop.
AllowedPattern: "^arn:.*$"
DestinationUri:
The URI for your destination, e.g. `s3://bucket-alias/ds101/`. S3 URIs must
end in a forward slash.
AllowedPattern: "^(s3:\/\/.+\/|https:\/\/.+)$"
DataAccessPointArn:
Type: String
Description: >-
The S3 URI for your Filedrop, e.g. `s3://bucket-alias/ds101/`
AllowedPattern: "^s3:\/\/.*$"
The access point ARN for your Filedrop.
AllowedPattern: "^(arn:.*)?$"
Default: ''
SourceBucketNames:
Type: CommaDelimitedList
Description: >-
Expand Down
5 changes: 3 additions & 2 deletions docs/forwarder.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ The forwarder stack can be configured with the following parameters:

| Parameter | Type | Description |
|-----------------|---------|-------------|
| **`DataAccessPointArn`** | String | The access point ARN for your Filedrop. |
| **`DestinationUri`** | String | The S3 URI for your Filedrop, e.g. `s3://bucket-alias/ds101/` |
| **`DestinationUri`** | String | The URI for your destination, e.g. `s3://bucket-alias/ds101/`. S3 URIs must end in a forward slash. |
| `DataAccessPointArn` | String | The access point ARN for your Filedrop. |
| `NameOverride` | String | Name of IAM role expected by Filedrop. This name will also be applied to the SQS Queue and Lambda Function processing events. In the absence of a value, the stack name will be used. |
| `SourceBucketNames` | CommaDelimitedList | A list of bucket names which the forwarder is allowed to read from. This list only affects permissions, and supports wildcards. In order to have files copied to Filedrop, you must also subscribe S3 Bucket Notifications to the forwarder. |
| `SourceTopicArns` | CommaDelimitedList | A list of SNS topics the forwarder is allowed to be subscribed to. |
| `MaxFileSize` | String | Max file size for objects to process (in bytes), default is 1GB |
| `ContentTypeOverrides` | CommaDelimitedList | A list of key value pairs. The key is a regular expression which is applied to the S3 source (<bucket>/<key>) of forwarded files. The value is the content type to set for matching files. For example, `\.json$=application/x-ndjson` would forward all files ending in `.json` as newline delimited JSON files. |
| `SourceKMSKeyArns` | CommaDelimitedList | A list of KMS Key ARNs the forwarder is allowed to use to decrypt objects in S3. |
| `DebugEndpoint` | String | Endpoint to send additional debug telemetry to. |

## Installation

Expand Down
6 changes: 3 additions & 3 deletions docs/stack.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ The Observe stack provisions the following components:

| Parameter | Type | Description |
|-----------------|---------|-------------|
| **`DataAccessPointArn`** | String | The access point ARN for your Filedrop. |
| **`DestinationUri`** | String | The S3 URI for your Filedrop, e.g. `s3://bucket-alias/ds101/` |
| `DataAccessPointArn` | String | The access point ARN for your Filedrop. |
| **`DestinationUri`** | String | The URI for your destination, e.g. `s3://bucket-alias/ds101/`. S3 URIs must end in a forward slash. |
| `SourceBucketNames` | CommaDelimitedList | A list of bucket names which the forwarder is allowed to read from. |
| `ContentTypeOverrides` | CommaDelimitedList | A list of key value pairs. The key is a regular expression which is applied to the S3 source (<bucket>/<key>) of forwarded files. The value is the content type to set for matching files. For example, `\.json$=application/x-ndjson` would forward all files ending in `.json` as newline delimited JSON files. |
| `NameOverride` | String | Name of IAM role expected by Filedrop. This role will be created as part of this stack, and must therefore be unique within the account. |
| `ConfigDeliveryBucketName` | String | If AWS Config is already enabled in this account and region, provide the S3 bucket snapshots are written to. |
| `IncludeResourceTypes` | CommaDelimitedList | Resources to collect using AWS Config. Use a wildcard to collect all supported resource types. Do not set this parameter if AWS Config is already installed for this region. |
| `IncludeResourceTypes` | CommaDelimitedList | If AWS Config is not enabled in this account and region, provide a list of resource types to collect. Use a wildcard to collect all supported resource types. |
| `ExcludeResourceTypes` | CommaDelimitedList | Exclude a subset of resource types from configuration collection. This parameter can only be set if IncludeResourceTypes is wildcarded. |
| `LogGroupNamePatterns` | CommaDelimitedList | Comma separated list of patterns. If not empty, the lambda function will only apply to log groups that have names that match one of the provided strings based on a case-sensitive substring search. |
| `LogGroupNamePrefixes` | CommaDelimitedList | Comma separated list of prefixes. If not empty, the lambda function will only apply to log groups that start with a provided string. |
Expand Down
2 changes: 1 addition & 1 deletion integration/tests/forwarder.tftest.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ run "install_forwarder" {
app = "forwarder"
parameters = {
DataAccessPointArn = run.target_bucket.access_point.arn
DestinationUri = "s3://${run.target_bucket.access_point.alias}"
DestinationUri = "s3://${run.target_bucket.access_point.alias}/"
SourceBucketNames = "${join(",", [for k, v in run.sources.buckets : v.id])}"
SourceTopicArns = "arn:aws:sns:${run.setup.region}:${run.setup.account_id}:*"
ContentTypeOverrides = "${var.override_match}=${var.override_content_type}"
Expand Down
78 changes: 78 additions & 0 deletions integration/tests/forwarder_s3.tftest.hcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# This test verifies our forwarder can write to an S3 bucket directly,
# without being fronted by a DataAccessPoint
run "setup" {
module {
source = "observeinc/collection/aws//modules/testing/setup"
version = "2.9.0"
}
}

run "target_bucket" {
module {
source = "observeinc/collection/aws//modules/testing/s3_bucket"
version = "2.9.0"
}

variables {
setup = run.setup
}
}

run "sources" {
module {
source = "./modules/setup_sources"
}

variables {
setup = run.setup
}
}

run "install_forwarder" {
variables {
setup = run.setup
app = "forwarder"
parameters = {
DestinationUri = "s3://${run.target_bucket.id}/"
SourceBucketNames = "${join(",", [for k, v in run.sources.buckets : v.id])}"
SourceTopicArns = "arn:aws:sns:${run.setup.region}:${run.setup.account_id}:*"
NameOverride = run.setup.id
}
capabilities = [
"CAPABILITY_NAMED_IAM",
"CAPABILITY_AUTO_EXPAND",
]
}
}

run "subscribe_sources" {
module {
source = "./modules/subscribe_sources"
}

variables {
sources = run.sources
queue_arn = run.install_forwarder.stack.outputs["Queue"]
}
}


run "check_sqs" {
module {
source = "observeinc/collection/aws//modules/testing/exec"
version = "2.9.0"
}

variables {
command = "./scripts/check_object_diff"
env_vars = {
SOURCE = run.sources.buckets["sqs"].id
DESTINATION = run.target_bucket.id
}
}

assert {
condition = output.exitcode == 0
error_message = "Failed to copy object using SQS"
}
}
2 changes: 1 addition & 1 deletion integration/tests/stack.tftest.hcl
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ run "install" {
app = "stack"
parameters = {
DataAccessPointArn = run.create_bucket.access_point.arn
DestinationUri = "s3://${run.create_bucket.access_point.alias}"
DestinationUri = "s3://${run.create_bucket.access_point.alias}/"
ConfigDeliveryBucketName = "example-bucket"
LogGroupNamePatterns = "*"
NameOverride = run.setup.id
Expand Down
2 changes: 1 addition & 1 deletion integration/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ variable "install_policy_json" {
type = string
default = null
validation {
condition = can(jsondecode(var.install_policy_json))
condition = var.install_policy_json == null || can(jsondecode(var.install_policy_json))
error_message = "must be valid JSON"
}
}
Expand Down

0 comments on commit 73713c0

Please sign in to comment.