Skip to content

Logging improvement: PROXY-DOMAIN may be a subdomain + domain #5415

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

Closed
wants to merge 1 commit into from

Conversation

ltomes
Copy link

@ltomes ltomes commented Aug 4, 2022

This logging is confusing when a user passes a full <subdomain>.<domain> as proxy-domain.
As an example if PROXY-DOMAIN = code.test.com
Before the logging output was:

[2022-08-04T16:15:11.829Z] info    - Proxying the following domain:
[2022-08-04T16:15:11.830Z] info      - *.code.test.com

After:

[2022-08-04T16:15:11.829Z] info    - Proxying the following domain:
[2022-08-04T16:15:11.830Z] info      - code.test.com

This was confusing when attempting to setup code-server on a reverse proxy with a service like cloudflare tunnels.

This is likely not the best fix. If preferred, I can do some additional work here to determine if the string is just a domain, or a subdomain+domain, and switch logging behavior based on that condition, and update the PR accordingly.

This logging was confusing when a user passes a full <subdomain>.<domain> as proxy-domain, for instance when running code-server with cloudflare tunnels.
This is likely not the best fix. If preferred, I can do some work here to determine if the string is just a domain, or a subdomain+domain, and switch logging behavior based on that condition.
@ltomes ltomes requested a review from a team August 4, 2022 16:37
@jsjoeio
Copy link
Contributor

jsjoeio commented Aug 4, 2022

@code-asher I think you're the best person to review this so assigning to you

@jsjoeio jsjoeio added this to the August 2022 milestone Aug 4, 2022
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #5415 (3dbff76) into main (9087e0c) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5415   +/-   ##
=======================================
  Coverage   72.42%   72.42%           
=======================================
  Files          30       30           
  Lines        1672     1672           
  Branches      366      366           
=======================================
  Hits         1211     1211           
  Misses        398      398           
  Partials       63       63           
Impacted Files Coverage Δ
src/node/main.ts 50.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9087e0c...3dbff76. Read the comment docs.

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

I like the existing *. because code-server will proxy for example:

8080.code.test.com
8081.code.test.com

But it will not proxy code.test.com (it will just show code-server).

Also there will need to be wildcard DNS records and certificates available at *.code.test.com.

I might be overthinking this though. Could you elaborate on the specific confusion you ran into?

@ltomes
Copy link
Author

ltomes commented Aug 4, 2022

But it will not proxy code.test.com (it will just show code-server).

That was helpful/I had a misconception, I think we should close this PR.

I just tested running my container without PROXY-DOMAIN set, and it worked properly.
I had been under the impression PROXY-DOMAIN was working with my reverse proxy but clearly it's not in use when a subdomain+domain is used as you stated. If I only set the domain, that interferes with my reverse proxy (as expected).

@ltomes ltomes closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants