Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Special" character issues when using the S3 storage backend #1795

Closed
bschmalhofer opened this issue May 27, 2022 · 6 comments
Closed

"Special" character issues when using the S3 storage backend #1795

bschmalhofer opened this issue May 27, 2022 · 6 comments
Assignees
Labels
bug Something isn't working as intended
Milestone

Comments

@bschmalhofer
Copy link
Contributor

When running the test suite with MinIO syncing. #1777, there are some failures that seem to be related to the characters used in the file name. An example is the test _scripts/test/Console/Command/Admin/Package/Upgrade.t _ . There we get the message:

Could not write the key Kernel/Config/Files/ZZZZUnitTestAdmin::Package::Upgradetest3947006996400002.pm to S3 at /opt/otobo/Kernel/System/UnitTest/Helper.pm line 583.
That something fishy is going on can be show in the MinIO Gui. Let's upload a dummy file called ZZZZUnitTestAdmin::Package::Upgradetest3947006996400002.pm and then retrieve it from MinIO. storing it in ../tmp:


bes:~/devel/OTOBO/otobo-docker (issue-#99-minio)$ cat ZZZZUnitTestAdmin::Package::Upgradetest3947006996400002.pm 
xxxx
bes:~/devel/OTOBO/otobo-docker (issue-#99-minio)$ cat ../tmp/ZZZZUnitTestAdmin\ Package\ Upgradetest3947006996400002.pm 
xxxx
bes:~/devel/OTOBO/otobo-docker (issue-#99-minio)$ 

The content xxxx is fine. But in the file name :: seems to be replaced ** ** (single space).

Needs to be investigated.

@bschmalhofer bschmalhofer added the bug Something isn't working as intended label May 27, 2022
@bschmalhofer bschmalhofer added this to the OTOBO 10.1.5 milestone May 27, 2022
@bschmalhofer bschmalhofer self-assigned this May 27, 2022
@bschmalhofer
Copy link
Contributor Author

The AWS documentation regarding object keys is at https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html.

@bschmalhofer
Copy link
Contributor Author

Not really understanding the AWS documentation. It's obvious that characters like ';' and '&' must be URL-encoded when used in URL. But why shouldn't they be used in object keys? As a first approach, let's be empiric. Wenn requesting PUT on 'http://minio:9000/otobo-bucket-20220524a/OTOBO/Kernel/Config/Files/ZZZZUnitTestAdmin::Package::Upgradetest1181006654200002.pm' then MinIO returns a 403. This needs to be worked around.

But first investigate how that works when there is no S3 syncing. Apparently the '::' do not end up in the file written to Kernel/Config/Files .

@bschmalhofer
Copy link
Contributor Author

bschmalhofer commented May 28, 2022

A lot of things are wrong in scripts/test/Console/Command/Admin/Package/Upgrade.t .

@bschmalhofer
Copy link
Contributor Author

bschmalhofer commented May 28, 2022

Found that there is not really a need to support fancy characters in the object keys. Adapted the test scripts/test/Storage/S3.t accordingly. But in that test script we should check whether we can store file names that were sanitized by FilenameCleanup. This is important for the article storage.

Here is the clean up from ArticleStorageFS.pm:


    # Perform FilenameCleanUp here already to check for
    #   conflicting existing attachment files correctly
    my $MainObject   = $Kernel::OM->Get('Kernel::System::Main');
    my $OrigFilename = $MainObject->FilenameCleanUp(
        Filename  => $Param{Filename},
        Type      => 'Local',
        NoReplace => 0,
    );


bschmalhofer added a commit that referenced this issue May 28, 2022
bschmalhofer added a commit that referenced this issue May 28, 2022
by recreating an instance of Kernel::Config
bschmalhofer added a commit that referenced this issue May 28, 2022
bschmalhofer added a commit that referenced this issue May 28, 2022
avoid the caching of the package verification
bschmalhofer added a commit that referenced this issue May 28, 2022
Using '::' makes no sense in any case as it created confusing in package names,
as usually a '::' indicates another directory level in the file system.
And trying to use a colon in a MinIO key results in an error, HTTP status code 403.
bschmalhofer added a commit that referenced this issue May 28, 2022
as custom code identifiers.
bschmalhofer added a commit that referenced this issue May 28, 2022
also fix regex for mtime in the last test
bschmalhofer added a commit that referenced this issue May 28, 2022
File names were generated by Kernel::System::Main::FilenameCleanUp().
Skipping the test with a '+' in the object key.
bschmalhofer added a commit that referenced this issue May 28, 2022
bschmalhofer added a commit that referenced this issue May 28, 2022
Do not reuse the variable @tests.
Use Test2::V0 more consistently.
bschmalhofer added a commit that referenced this issue May 28, 2022
@bschmalhofer
Copy link
Contributor Author

bschmalhofer commented May 28, 2022

Two findings:

  • ArticleStorageS3.pm should pass NoReplace => 0 to FilenameCleanUp() , just like ArticleStorageFS.pm
  • In the S3 object key '+' is problematic. Replace it too with an underscore.
  • same with the character '#'

bschmalhofer added a commit that referenced this issue May 30, 2022
Also replace + and # for the S3 type.
Use the new type for the S3 article storage.
Add test cases.
bschmalhofer added a commit that referenced this issue May 30, 2022
@bschmalhofer
Copy link
Contributor Author

The code has been adapted. Currently there are no known problems with unsupported characters. Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as intended
Projects
None yet
Development

No branches or pull requests

1 participant