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.

Sign up for GitHub

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

Fix nginx template for custom s3 endpoints #3278

Open
wants to merge 5 commits into
base: preview
Choose a base branch
from
Open

Fix nginx template for custom s3 endpoints #3278

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a57838e
fix custom minio usage
dbtek Dec 29, 2023
78e8c41
revert custom path
dbtek Dec 29, 2023
ae5c637
fix trailing slash
dbtek Dec 29, 2023
9702b80
Merge remote-tracking branch 'upstream/preview' into preview
dbtek Jun 7, 2024
47db737
Update nginx.conf.template
dbtek Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nginx/nginx.conf.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ http {
}

location /${BUCKET_NAME}/ {
proxy_pass http://plane-minio:9000/uploads/;
proxy_pass ${AWS_S3_ENDPOINT_URL}/${AWS_S3_BUCKET_NAME};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please append "/" at the end as

proxy_pass ${AWS_S3_ENDPOINT_URL}/${AWS_S3_BUCKET_NAME}/;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad. Fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

I believe this is not the complete fix for custom S3 end point. As today if the files were uploaded to S3 (and not minio), the uploaded file url gets into the issue content. That means every time you see that file, it doesn’t go through proxy but directly from client to S3

if this has to go through proxy, application level code need to be modified as well.

Let me reconfirm with team and get back to you. @sriramveeraghanta @pablohashescobar

I will keep this PR on hold till we get clarity from team

Copy link
Author

@dbtek dbtek Dec 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm this works with external minio and proxied asset urls. Application level modification would be required to use direct s3 urls other than current with proxy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek, this change requires a significant change in the application we will keep this PR open till then once we have made the application we will merge this. Thank you for your contributions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablohashescobar is there any way to make external minio url work with docker deployment currently?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this proposed change I perfectly use external minio without any issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek Got it, but pr seems to be open for long time and this is blocker for anyone trying to use external minio

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poudelprakash yeah I know. I built my own proxy image and advise anyone to the same as it seems that pr won't be merged anytime soon.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek Thanks to your pr and comments, I got it working now :)

}
}
}