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

Correcting use of httpsPort instead of secure.port #6538

Merged
merged 4 commits into from
Feb 11, 2022

Conversation

mikeV02
Copy link
Contributor

@mikeV02 mikeV02 commented Feb 4, 2022

I was trying to change the TrafficRouter SecurePort value to other than 443, and tried to use the parameter secure.port added to the server.xml on the CCR profile in Traffic Portal. But noticed, that TcOps/TcMon, do not add that parameter to the CrCronfig.json.

I digged into the code and fount that the use of secure.port was added to TrafficRouter on the following commit: 8ff9006 However, no counter part was added to TcOps to actually state the value secure.port from the TrafficPortal paramenters. The value that TcOps adds to the CrConfig.json is the httpsPort, from the server configuration itself on TrafficPortal, but that parameter is not used anywhere by Traffic Router (from what I saw).

I see that it would require just two lines edits on traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/util/LanguidState.java to make use of httpsPort and not the unexisting secure.port.

Basically, edit lines 86 and 87 of the file, and replace secure.port for httpsPort


Which Traffic Control components are affected by this PR?

  • Traffic Router

What is the best way to verify this PR?

Try adding the secure.port parameter to the CCR profile on Traffic Portal. It does not matter if you add it to server.xml or CrConfig,json, Traffic Ops never assigns secure.port to the traffic routers under that profile. Applying the change on the file on this PR, makes TR use the httpsPort configured under the server configuration un Traffic Portal.

If this is a bugfix, which Traffic Control versions contained the bug?

All

PR submission checklist

I was trying to change the TrafficRouter SecurePort value to other than 443, and tried to use the parameter secure.port added to the server.xml on the CCR profile in Traffic Portal. But noticed, that TcOps/TcMon, do not add that parameter to the CrCronfig.json.I digged into the code and fount that the use of secure.port was added to TrafficRouter on the following commit: apache@8ff9006 However, no counter part was added to TcOps to actually state the value secure.port from the TrafficPortal paramenters. The value that TcOps adds to the CrConfig.json is the httpsPort, from the server configuration itself on TrafficPortal, but that parameter is not used anyowhere by Traffic Router (from what I saw).I see that it would require just two lines edits on traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/util/LanguidState.java to make use of httpsPort and not the unexisting secure.port.Basically, edit lines 86 and 87 of the file, and replace secure.port for httpsPort
@mikeV02 mikeV02 marked this pull request as ready for review February 4, 2022 02:41
@rawlinp rawlinp self-assigned this Feb 4, 2022
@rawlinp rawlinp added bug something isn't working as intended Traffic Router related to Traffic Router labels Feb 4, 2022
@rawlinp
Copy link
Contributor

rawlinp commented Feb 7, 2022

Also, I just want to point out that if you use a non-standard httpsPort for your TRs, all the delivery service example URLs for https will be wrong, since they'll be missing TR's non-standard https port suffix. There's not much we can do to fix that, but it shouldn't really matter as long as the clients are given the correct URL with the non-standard https port. If we were to make secure.port a real per-CDN TR profile parameter, we could fix the example URLs for the delivery services.

@mikeV02 mikeV02 requested a review from rawlinp February 8, 2022 02:42
Copy link
Contributor

@rawlinp rawlinp left a comment

Choose a reason for hiding this comment

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

This looks good, but there are merge conflicts to resolve (most likely in CHANGELOG.md).

@mikeV02 mikeV02 requested a review from rawlinp February 11, 2022 21:06
@rawlinp rawlinp merged commit 418ebd5 into apache:master Feb 11, 2022
@mikeV02 mikeV02 deleted the patch-1 branch May 6, 2022 13:50
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 Traffic Router related to Traffic Router
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants