pjfanning commented on code in PR #1971:
URL: https://github.com/apache/pekko-connectors/pull/1971#discussion_r4143532334
##########
s3/src/test/scala/org/apache/pekko/stream/connectors/s3/impl/auth/CanonicalRequestSpec.scala:
##########
@@ -17,11 +17,29 @@ import org.apache.pekko
import pekko.http.scaladsl.model.Uri.Query
import pekko.http.scaladsl.model._
import pekko.http.scaladsl.model.headers._
+import pekko.stream.connectors.s3.{ ApiVersion, BufferType, MemoryBufferType,
S3Settings }
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
+import software.amazon.awssdk.auth.credentials.{ AnonymousCredentialsProvider,
AwsCredentialsProvider }
+import software.amazon.awssdk.regions.Region
+import software.amazon.awssdk.regions.providers.AwsRegionProvider
class CanonicalRequestSpec extends AnyFlatSpec with Matchers {
+ def getSettings(
+ bufferType: BufferType = MemoryBufferType,
+ awsCredentials: AwsCredentialsProvider =
AnonymousCredentialsProvider.create(),
+ s3Region: Region = Region.US_EAST_1,
+ listBucketApiVersion: ApiVersion = ApiVersion.ListBucketVersion2) = {
+ val regionProvider = new AwsRegionProvider {
+ def getRegion = s3Region
+ }
+
+ S3Settings(bufferType, awsCredentials, regionProvider,
listBucketApiVersion, Map.empty)
+ }
+
+ implicit val settings: S3Settings = getSettings()
Review Comment:
The spec changes only add an implicit so the existing tests compile. Please
add a test where a custom `reservedCharacters` value changes the encoded path
(e.g. a character from the default set is left unencoded). An `S3SettingsSpec`
case showing `reserved-characters` is read from config would also be good.
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -365,9 +365,13 @@ final class S3Settings private (
val retrySettings: RetrySettings,
val multipartUploadSettings: MultipartUploadSettings,
val signAnonymousRequests: Boolean,
- val allowedHeaders: Map[String, Set[String]]
+ val allowedHeaders: Map[String, Set[String]],
+ val reservedCharacters: Option[String]
Review Comment:
Please add a public `withReservedCharacters(...)` like the other fields
have. `copy` is private, so at the moment this can only be set through config.
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -689,6 +697,11 @@ object S3Settings {
k -> (v ++ allowedHeadersBase.getOrElse(k, Set.empty[String]))
}
+ val reservedCharacters =
Review Comment:
Since `reference.conf` defines `reserved-characters`, `hasPath` is always
true here, so this is always `Some` when loaded from config. `None` only
happens via the programmatic `S3Settings.apply(...)` factories. Either make it
a plain `String` defaulting to `CanonicalRequest.reservedCharacters`, or leave
the key commented out in `reference.conf` so `None` really means "use the
default".
##########
s3/src/main/resources/reference.conf:
##########
@@ -342,4 +342,8 @@ pekko.connectors.s3 {
PutBucketVersioning = []
GetBucketVersioning = []
}
+
+ # Allow custom non-standard reserved characters
+ # https://tools.ietf.org/html/rfc3986#section-2.2
+ reserved-characters = ":?#[]@!$&'()*+,;="
Review Comment:
`reserved-characters` doesn't say what it controls. Something like
`signing.path-encoded-characters` would be clearer. Also note that `/` is
deliberately left out; if a user adds it, signing breaks. Worth documenting or
validating.
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -485,6 +491,7 @@ final class S3Settings private (
}.mkString(", ")
s"Map($entries)"
}" +
+ s"reservedCharacters=$reservedCharacters" +
Review Comment:
There's no separator before `reservedCharacters=`, so it runs straight on
from the `allowedHeaders` map. Several existing fields also lack a comma; it
would be good to fix those at the same time.
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/impl/auth/Signer.scala:
##########
@@ -16,8 +16,8 @@ package org.apache.pekko.stream.connectors.s3.impl.auth
import java.security.MessageDigest
import java.time.format.DateTimeFormatter
import java.time.ZonedDateTime
-
import org.apache.pekko
+import org.apache.pekko.stream.connectors.s3.S3Settings
Review Comment:
The import style is still off: this should be
`pekko.stream.connectors.s3.S3Settings`, grouped with the other `pekko.`
imports. Please also keep the blank line between the `java.*` imports and
`import org.apache.pekko` (it was removed here and in `SignerSpec.scala`).
##########
s3/src/main/resources/reference.conf:
##########
@@ -342,4 +342,8 @@ pekko.connectors.s3 {
PutBucketVersioning = []
GetBucketVersioning = []
}
+
+ # Allow custom non-standard reserved characters
Review Comment:
Please document what value Huawei OBS / Tencent COS actually need, here and
in `docs/src/main/paradox/s3.md`. Otherwise users hitting the 403 won't know
what to set.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
