fix: return url without port if default port protocol is used

[flobz]

When using Hawkbit behind a proxy wrong port is returned for downloading artifacts if default port protcol is used(eg: 443 for https).

If no port is specified hawkbit return hawkbit port not protocol default port:
https://anotherhost.com:8080/DEFAULT/controller/v1/xxxxxx/softwaremodules/X/artifacts/*
Instead of
https://anotherhost.com/DEFAULT/controller/v1/xxxxxx/softwaremodules/X/artifacts/*

[hawkbit-bot]

Can one of the admins verify this patch?

[avgustinmm]

Not sure about that. Now, if I have configured 8080 for http port and no request port you'll return no port - i.e. 80.
this wouldn't work if you have no proxy? This forced you to change AbstractDDiApiIntegrationTest#HTTP_LOCALHOST.
If you are behind proxy, and the request port is not available, I suppose you shall configure the protocol port (https) in that case to be 443.
would addub something like:

hawkbit.artifact.url.protocols.download-cdn-http.protocol=https
hawkbit.artifact.url.protocols.download-cdn-http.port=443

or

hawkbit.artifact.url.protocols.download-http.protocol=https
hawkbit.artifact.url.protocols.download-http.port=443

help for proper resolution?

[flobz]

It seems to work because all test in PropertyBasedArtifactUrlHandlerTest.java passed ?

Also this isn't a configuration issue hawkbit should be able to generate correct url without changing the configuration.
I change indeed the "property" hawkbit.artifact.url.protocols.download-http.port to 443 as a temporary fix but in my opinion this is not the correct behavior.

For exemple I could have different proxy with different port at the same time.

I change AbstractDDiApiIntegrationTest#HTTP_LOCALHOST because MockMvcRequestBuilders create a fake server with default http port so it should not return url with 8080 port.

[avgustinmm]

By default, hawkbit server is plain http on 8080. That's why it shall, by default, add this port into the download URL.
That's why in the AbstractDDiApiIntegrationTest it expect 8080.
If hawkbit is behind a proxy and uses standard http/https ports your PR works.
I agree that in production hawkbit shall always run behind proxy (or SSL shall be enabled) and this plain http/8080 mode won't be applied.
However, it shall also work without proxy and so for testing purposes. That's why I question if this is the right approach.
Anyway, in production, you would have to make additional configurations. Then, if a configuration fix that problem - why not?

[flobz]

Thz class AbstractDDiApiIntegrationTest is about hawkbit test without proxy.

So without proxy, port in artifact url should be equal to hawkbit port. Then if hawkbit run on port 80, artifact url shoukd use port 80. In my understanding the mock MockMvcRequestBuilders emulate a server on port 80.

[strailov]

The current DEFAULT port in scope of hawkBit is 8080. Each application could define such default port. Of course the local / development purposes configuration often differ from one for particular environment.
So if you do a deployment you should always configure the application for your specific needs.
So I would stick here with 8080. If your case is different you can always configure the application for your case.

[flobz]

@strailov This PR isn't about changing default Hawkbit port it's about generating correct url when hawkbit is behind an https proxy on port 443.

@avgustinmm They were two solutions to fix integration tests:

1. Use default port set in MockHttpServletRequest which is 80 for testing url
2. Use Hawkbit default port 8080 that's what I did in this new commit beaed0a

Solution 1 seems simpler to me because it was just about changing one line.

[avgustinmm]

@flobz,
the point is not about making tests to use 8080 but more about having a correctly running default hawkBit application. Imagine that someone starts monolith hawkBit via the docker image or the jar or just via the start class in the IDE.
He will get hawkBit running on 8080 port with REST API (mgmt + did) on that port. However, with your PR, when a controller gets the download URL it will get http://localhost/... which points to port 80 and won't work.
So, with this PR you may get an application running correctly behind a proxy but not without.
That, in my opinion, is not ok. If you have run hawkBit behind a proxy you probably run it in production so you should configure it anyway - e.g you shall configure user/pass to be not admin/admin. So, I think that the correct configuration of the download port shall be part of that configuration.
My proposal would be to:

1. keep download poet of default (without proxy) app 8080
2. configure an app behind a proxy to return a proper URL.
   The configuration, I proposed in my first comment, may be do the latter. If not, or you want to not have https://host:443/.. but https://host/.. I think you could achieve that by modifying hawkbit.artifact.url.ref.
   If the latter is not possible with the current hawkBit then maybe a PR shall made to enable it. Otherwise, maybe the target here shall be to properly describe the configuration of an app behind a proxy in the documentation.
   What do you think?

[flobz]

the point is not about making tests to use 8080 but more about having a correctly running default hawkBit application. Imagine that someone starts monolith hawkBit via the docker image or the jar or just via the start class in the IDE.
He will get hawkBit running on 8080 port with REST API (mgmt + did) on that port. However, with your PR, when a controller gets the download URL it will get http://localhost/... which points to port 80 and won't work.

I totally agree, we should not change default behavior and I think that my change don't do that . This test https://github.com/eclipse/hawkbit/blob/28f0446d9d835979c7bc7759ae0a75e82c5514e5/hawkbit-core/src/test/java/org/eclipse/hawkbit/api/PropertyBasedArtifactUrlHandlerTest.java#L71 seems to verify the scenario you describe, and it still pass with my changes.

If you have run hawkBit behind a proxy you probably run it in production so you should configure it anyway - e.g you shall configure user/pass to be not admin/admin. So, I think that the correct configuration of the download port shall be part of that configuration.

The class PropertyBasedArtifactUrlHandler has a parameter called "portRequest":
https://github.com/eclipse/hawkbit/blob/312498fc511f05a7aa18e1c5493f099386f2cd8a/hawkbit-core/src/main/java/org/eclipse/hawkbit/api/PropertyBasedArtifactUrlHandler.java#L56
it allow to dynamically return the good port depending of the situation it's works for all port except 80 and 443 so in my opinion this is an incorrect behaviour.
Hardcoding the proxy port in the configuration is a solution but what is the point of having portRequest option

Here is a table to show what this PR change really:

request	proxy	generated artifacts url before	generated artifacts url before after	comment
http://localhost:8080	no	http://localhost:8080	http://localhost:8080	no change
http://exemple.com:4242	yes	http://exemple.com:4242	http://exemple.com:4242	no change
https://exemple.com:4242	yes	https://exemple.com:4242	https://exemple.com:4242	no change
http://exemple.com	yes	http://exemple.com:8080	http://exemple.com	fix
https://exemple.com	yes	https://exemple.com:8080	https://exemple.com	fix

So to sum up this PR :

• don't modify anything if hawkbit is used without proxy
• don't modify anything if hawkbit is called on port 8080
• don't modify anything if hawkbit is used with proxy on any port except 80 and 443
• fix if hawkbit is used with proxy on port 80 and 443

[avgustinmm]

aha, we will check. If

request	proxy	generated artifacts url before	generated artifacts url before after	comment
http://localhost:8080	no	http://localhost:8080	http://localhost:8080	no change

then I suppose the PR is ok

[avgustinmm]

@flobz, thanks for the contribution.
It seems OK and we could merge it. I just see that the copyright of the RequestOnHawkbitDefaultPortPostProcessor is 2015 and Bosch. Could you fix it?
You could use https://github.com/eclipse/hawkbit/blob/master/licenses/LICENSE_HEADER_TEMPLATE.txt . I just fixed it for 2024.

[flobz]

I fixed the copyright issue. 😊. You want me to update the copyright to 2024?

[avgustinmm]

I'd propose to make the first line "Copyright (c) 2024 Contributors to the Eclipse Foundation"

[flobz]

@avgustinmm it's done.

[avgustinmm]

@flobz, thanks for the contribution