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

Jump to bottom

Configurable http port #1596

Merged
merged 5 commits into from
Nov 22, 2022
Merged

Configurable http port #1596

merged 5 commits into from
Nov 22, 2022

Conversation

cmosd
Copy link
Contributor

@cmosd cmosd commented Nov 17, 2022

Proposed changes

Added configurable http port in tedge.toml where default is 8000.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#1479

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@cmosd cmosd force-pushed the improvement/1479/configurable-http-port branch from 6783046 to c10cf0e Compare November 17, 2022 09:27
didier-wenzek
didier-wenzek requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

There is mix between http and mqtt fields.

The documentation related to file transfer should also be updated to tell how to set a specific HTTP port.

crates/common/tedge_config/src/tedge_config.rs Outdated Show resolved Hide resolved
crates/common/tedge_config/src/tedge_config.rs Outdated Show resolved Hide resolved
crates/common/tedge_config/src/tedge_config.rs Outdated Show resolved Hide resolved
crates/core/tedge_agent/src/http_rest.rs Outdated Show resolved Hide resolved
albinsuresh
albinsuresh reviewed Nov 21, 2022
View reviewed changes
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

You need to update the c8y-config-plugin here to use this http port setting.

cmosd
cmosd commented Nov 21, 2022
View reviewed changes
crates/common/tedge_config/src/tedge_config_dto.rs Outdated
Comment on lines 107 to 111
pub(crate) struct HttpConfigDto {
pub(crate) port: Option<u16>,
pub(crate) bind_address: Option<IpAddress>,
pub(crate) external_port: Option<u16>,
pub(crate) external_bind_address: Option<IpAddress>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@didier-wenzek, maybe i should have just kept port here, but I had in mind the scenario where we might want a http external address and mqtt on an internal one, or vice-versa

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds fine Oops no.

  • It's really confusing to have these two addresses for MQTT. Let's avoid to have the same for HTTP.
  • => Keep only port and bind_address.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you add an http.bind_address here, you have also to add an implementation for ConfigSettingAccessor<HttptBindAddressSetting> and to use it in the mapper and the c8y_configuration plugin.

Note that the implementation of ConfigSettingAccessor<HttptBindAddressSetting>::query() is a convenient place to implement the logic defaulting to the MQTT setting when nothings specific is set for HTTP.

@cmosd
Copy link
Contributor Author

cmosd commented Nov 21, 2022

You need to update the c8y-config-plugin here to use this http port setting.

Thanks, updated!

didier-wenzek
didier-wenzek reviewed Nov 21, 2022
View reviewed changes
plugins/c8y_configuration_plugin/src/main.rs Outdated

//TODO: Port number to be read from HttpPortSetting
let local_http_host = format!("{}:8000", bind_address.to_string().as_str());
let local_http_host = format!("{}:{}", bind_address.to_string().as_str(), http_port);
Copy link
Contributor

Choose a reason for hiding this comment

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

The bind address is computed after MqttBindAddressSetting and MqttExternalBindAddressSetting. These should be used only if no HTTP bind address has been configured (since you added the latter in HttpConfigDto).

=> Add an implementation for ConfigSettingAccessor<HttptBindAddressSetting>::query() defaulting to the MQTT setting when nothings specific is set for HTTP.

crates/common/tedge_config/src/tedge_config_dto.rs Outdated
Comment on lines 107 to 111
pub(crate) struct HttpConfigDto {
pub(crate) port: Option<u16>,
pub(crate) bind_address: Option<IpAddress>,
pub(crate) external_port: Option<u16>,
pub(crate) external_bind_address: Option<IpAddress>,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you add an http.bind_address here, you have also to add an implementation for ConfigSettingAccessor<HttptBindAddressSetting> and to use it in the mapper and the c8y_configuration plugin.

Note that the implementation of ConfigSettingAccessor<HttptBindAddressSetting>::query() is a convenient place to implement the logic defaulting to the MQTT setting when nothings specific is set for HTTP.

@cmosd cmosd force-pushed the improvement/1479/configurable-http-port branch 2 times, most recently from 75e5bfd to a81d381 Compare November 22, 2022 09:47
didier-wenzek
didier-wenzek approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved

alexandru solomes added 4 commits November 22, 2022 12:54
Make http port configurable
0c37511 
Signed-off-by: alexandru solomes <solo@softwareag.com>
Use configurable http port in File Transfer Service
447b8b9 
Signed-off-by: alexandru solomes <solo@softwareag.com>
Use HTTP port setting in c8y-configuration-plugin
40f0fcf 
Signed-off-by: alexandru solomes <solo@softwareag.com>
Add HTTP bind address setting
3231d2f 
Added HttpBindAddressSetting to automatically choose between external
bind address or "internal" bind address. It currently queries the values
for mqtt.bind_address and mqtt.external.bind_address, choosing external
over internal when available

Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1479/configurable-http-port branch from a81d381 to d549140 Compare November 22, 2022 10:56
Use HTTP bind address in c8y-configuration-plugin
bd8a4b0 
Signed-off-by: alexandru solomes <solo@softwareag.com>
@cmosd cmosd force-pushed the improvement/1479/configurable-http-port branch from d549140 to bd8a4b0 Compare November 22, 2022 10:57
@cmosd cmosd merged commit bfab8ab into thin-edge:main Nov 22, 2022
@cmosd cmosd deleted the improvement/1479/configurable-http-port branch November 22, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cmosd @albinsuresh @didier-wenzek