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

Support ARTIFACT_ROOT_URL #30883

Closed
wants to merge 2 commits into from

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented May 7, 2024

Fix https://gitea.com/gitea/act_runner/issues/541

Users may use a non-ROOT_URL address to register a runner, since the ROOT_URL may be a public address listened by a reverse proxy.

Most communication between runners and Gitea (like calling API and cloning repo) uses the URL provided during registration, except for uploading or downloading artifacts. These addresses are generated by Gitea and provided to the runners. Gitea always uses the ROOT_URL to generate these addresses, causing the runner to upload or download artifacts from the public network.

This PR adds an new configuration ARTIFACT_ROOT_URL to indicate the address for uploading or downloading artifacts. If not specified, the ROOT_URL is still used.

@wolfogre wolfogre added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/gitea-actions related to the actions of Gitea labels May 7, 2024
@wolfogre wolfogre added this to the 1.23.0 milestone May 7, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 7, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 7, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/docs labels May 7, 2024
@wxiaoguang
Copy link
Contributor

I think if there is such a use case, we should make the "http ctx" support "X-Forwarded-Xxx" headers, then no need to add any new config option.

Ref:

@wolfogre
Copy link
Member Author

wolfogre commented May 7, 2024

I think if there is such a use case, we should make the "http ctx" support "X-Forwarded-Xxx" headers, then no need to add any new config option.

Ref:

I have thought about this, but I'm not sure if it could cause more problems.

In my mind, it's something like:

  1. If there is X-Forwarded-Xxx, it's behind a proxy so it's OK to use X-Forwarded-Xxx.
  2. If there's no X-Forwarded-Xxx, always assume there's no proxy, use Host even if it's different from ROOT_URL.

And you have got the point: "X-Forwarded-Xxx is not standard." We cannot assume there's no proxy if there's no such header.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 7, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 7, 2024

2.

And you have got the point: "X-Forwarded-Xxx is not standard." We cannot assume there's no proxy if there's no such header.

It is de-facto standard. I am sure we should introduce it sooner or later.

Let me describe what I understand now (correct me if I am wrong):

  • Runner accesses https://non-app-url/xxx and expects to get a full URL https://non-app-url/yyy, right?

If it is so, I will propose a general solution, we only need to tell end users: please configure your reverse proxy properly , and actually, it has been well documented for long time: https://docs.gitea.com/administration/reverse-proxies#nginx

@wolfogre
Copy link
Member Author

wolfogre commented May 7, 2024

And you have got the point: "X-Forwarded-Xxx is not standard." We cannot assume there's no proxy if there's no such header.

It is de-facto standard. I am sure we should introduce it sooner or later.

Let me describe what I understand now (correct me if I am wrong):

  • Runner accesses https://non-app-url/xxx and expects to get a full URL https://non-app-url/yyy, right?

If it is so, I will propose a general solution, we only need to tell end users: please configure your reverse proxy properly , and actually, it has been well documented for long time: docs.gitea.com/administration/reverse-proxies#nginx

Your understanding is completely right.

If we required any proxy for Gitea provides X-Forwarded-Xxx and Gitea can assume there's no proxy if there's not such header, you are right, no need to add any new config option, and I believe things will be much easier.

@wxiaoguang
Copy link
Contributor

If there's no X-Forwarded-Xxx, always assume there's no proxy, use Host even if it's different from ROOT_URL.

For this case, I think it could also be right in most cases. I will try to propose a solution.

ps: do we need it in 1.22?

@wolfogre
Copy link
Member Author

wolfogre commented May 7, 2024

ps: do we need it in 1.22?

You mean the issue of artifacts? I think it's not a hurry since there are some workarounds.

But I agree to do it in 1.22 since it's just RC1, and it doesn't break anything as long as users configure the proxy correctly.

@wxiaoguang
Copy link
Contributor

Does #30885 look good to you? Will write some tests.

@wolfogre
Copy link
Member Author

wolfogre commented May 7, 2024

Does #30885 look good to you? Will write some tests.

Let's follow it, this PR can be closed now.

@wolfogre wolfogre closed this May 7, 2024
@GiteaBot GiteaBot removed this from the 1.23.0 milestone May 7, 2024
wxiaoguang added a commit that referenced this pull request May 7, 2024
Fix #30883
Fix #29591

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request May 7, 2024
Fix go-gitea#30883
Fix go-gitea#29591

---------

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
wxiaoguang added a commit that referenced this pull request May 8, 2024
Backport #30885
Fix #30883
Fix #29591

Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/docs modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/gitea-actions related to the actions of Gitea type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants