Repository navigation
Support unix domain sockets - #784
christoph-hamm wants to merge 35 commits into
Conversation
d1adf6c to
b580024
Compare
|
The domains sockets have been added to the ank-server, ank-agent and ank. The devcontainer config, debian package and the install script have been updated as well. All three are working. Only for devcontainer the stests are not working at the moment. I have not checked the documentation yet, if everything is still working. I think we need to mention the new ankaios group, and that you have to be part of that group in order to use the ankaios CLI. We can also remove the warning in the documentation. |
d97f7da to
53f0cad
Compare
|
|
I'll start with the review here. |
krucod3
left a comment
There was a problem hiding this comment.
Initial review done. Configs and documentation are now reviewed. I'll continue with the rest now.
|
|
||
| fn validate_socket_configuration(agent_config: &AgentConfig) -> Result<(), String> { | ||
| validate_unix_socket_tls_settings( | ||
| "agent", |
There was a problem hiding this comment.
I somehow don't like this string here ...
There was a problem hiding this comment.
At the end it's mean to reduce code duplications, but the or below is still repeated and we have a freely configurable string as input. The logic in the common function is also not really complex. Maybe just providing common functions is_unix is is_tcp would make more sense.
There was a problem hiding this comment.
I have removed the the parameter. The call as now using map_err to adding a prefix.
| # The server url. | ||
| # server_url = 'https://127.0.0.1:25551' | ||
| # The server endpoint. | ||
| address = 'unix:///run/ankaios/server.sock' |
There was a problem hiding this comment.
Why renaming here? A domain socket is still specified by a url which includes also the protocol, in this case "unix://"
There was a problem hiding this comment.
Changed it back to URL.
| if address.starts_with("https://") || address.starts_with("http://") { | ||
| Ok(()) |
There was a problem hiding this comment.
So if only https:// it's OK?
There was a problem hiding this comment.
The code is now changed, as the URL is more early put in an enum to distinguish HTTP from UDS. But the logic is still the same, for UDS we check it is not empty, for URL we do not. For UDS the check is primarily there, to check for an absolute path, and returning an error saying it is not absolute for an empty path would be strange, hence the special handling for empty. For HTTP it is as before. If we start checking we could add more and more checks to see it is an valid HTTP URL, and I do not want to start this.
| )] | ||
| /// The server endpoint. | ||
| /// Supported values are https://host:port and unix:///path/to/socket. | ||
| pub address: Option<String>, |
There was a problem hiding this comment.
Again, I don't understand why we want to change the external interface of the agent. server url is already correct.
There was a problem hiding this comment.
It is back to server_url.
| let tls_config = if agent_config.address.starts_with("unix://") { | ||
| None | ||
| } else { | ||
| if let Err(err_message) = TLSConfig::is_config_conflicting( | ||
| agent_config.insecure, | ||
| &agent_config.ca_pem_content, | ||
| &agent_config.crt_pem_content, | ||
| &agent_config.key_pem_content, | ||
| ) { | ||
| log::warn!("{err_message}"); | ||
| } |
There was a problem hiding this comment.
This is a bit strange now. We already did some checks about TLS, insecure and connection type, and now we start to create the TLS config and ignore the uds connections ...
I have not looked in detail and there are some differences between server and agent/ank handling, but maybe there is a better way to organize the config handling.
There was a problem hiding this comment.
We checked if the config is valid before, and now we only create the tls config if it is needed. Maybe at the initial check we could create some enum structure which only allow valid configuration and which might remove some checks here, or something similar with traits. I think it is OK as it is and would not change it.
| # If set to 'true' and the certificates are not provided, then the server shall not use TLS. | ||
| insecure = false | ||
| # This option must not be used with 'unix://' addresses. | ||
| # insecure = true |
There was a problem hiding this comment.
I think we should leave the default to false also in the example.
There was a problem hiding this comment.
Set it to false again.
| If no `address` is configured and no configuration file is used, the Ankaios server defaults to | ||
| the TCP address `127.0.0.1:25551`. |
There was a problem hiding this comment.
So if nothing is configured, Ankaios does not start?
default url is http and insecure is per default false. No default tls config => fail to start. Maybe default should be uds.
There was a problem hiding this comment.
Below there is another paragraph stating that the default is the domain socket:
https://github.com/eclipse-ankaios/ankaios/pull/784/changes#diff-f37a7d24bc9ebf61ff0c5e3165666b5590ba702a6ad912760833cfd3024f855eR50-R52
Which one is correct?
There was a problem hiding this comment.
If you have no configuration files, the default is 127.0.0.1:5551 but as no certificates are provided, it will not start. I was also thinking about changing it to UDS as default, but this would be a breaking change. E.g. a user has a configuration files only configuring the certificate, the server would now start listening on the default https://127.0.01:5551. If we would now make UDS the default, it would not listen to localhost anymore or crash for wrong configuration (depending on whether we are changing this or not).
The thing below ist about the default installation methods. Here some configuration files are installed as well, which default to UDS.
| } | ||
|
|
||
| pub fn validate_server_address_format(address: &str) -> Result<(), String> { | ||
| if let Some(path) = address.strip_prefix("unix://") { |
There was a problem hiding this comment.
Please introduce constants for the protocol identifiers.
|
|
||
| ```toml | ||
| address = 'https://127.0.0.1:25551' | ||
| insecure = false |
There was a problem hiding this comment.
We don't need the false here as it is default.
krucod3
left a comment
There was a problem hiding this comment.
Review the rest without tools folder. Will review this one after all changes there are pushed.
| - itest | ||
|
|
||
| #### gRPC Server supports unix domain socket endpoints | ||
| `swdd~grpc-server-supports-unix-domain-socket-endpoints~1` |
There was a problem hiding this comment.
And the client does not need anything?
We should add another req for the client too.
There was a problem hiding this comment.
I have added an requirements for the client.
|
|
||
| Status: approved | ||
|
|
||
| The gRPC Server shall support listening for incoming gRPC connections on either a TCP socket endpoint or a Unix domain socket endpoint. |
There was a problem hiding this comment.
The name is about Unix domain sockets, but the description mentions either TCP or Unix sockets.
There was a problem hiding this comment.
Updated the description.
| The gRPC Server shall support listening for incoming gRPC connections on either a TCP socket endpoint or a Unix domain socket endpoint. | ||
|
|
||
| Rationale: | ||
| Unix domain sockets allow local communication without exposing a TCP port. |
There was a problem hiding this comment.
But requires a socket. That's not a rationale. Suggestion:
| Unix domain sockets allow local communication without exposing a TCP port. | |
| Unix domain sockets are faster and allow securing local communication using IAM improving the ease of use. |
There was a problem hiding this comment.
I took you rational but dropped the "faster" part, as this was not a reason we added them.
| fn parse_server_endpoint( | ||
| server_address: &String, |
There was a problem hiding this comment.
I still think calling is url instead of endpoint or address is more precise.
There was a problem hiding this comment.
It is URL again.
| - utest | ||
|
|
There was a problem hiding this comment.
We could also stest the group setting and the permissions.
There was a problem hiding this comment.
We could but I am not sure if it is worth it. We have to ensure the test user has supplementary group we can use for the test. We also have to ensure the umask does not produce the wanted file permissions.
I see it like this: we have tested in manually and it works, and I think it is unlikely it is removed or changed accidentally, as we also have utest for this.
| let server_config_content = r"# | ||
| version = 'v1' | ||
| address = 'unix:///tmp/ankaios-server.sock' | ||
| ca_pem = '/tmp/.certs/ca.pem' |
There was a problem hiding this comment.
ca_pem_content is not tested.
There was a problem hiding this comment.
Added unit test for ca_pem_content.
| ${AGENT_NAME}= agent_A | ||
| ${AGENT_2_NAME}= agent_B | ||
| ${ANKAIOS_TMP_FOLDER}= /tmp/ankaios | ||
| ${ANKAIOS_SERVER_URL}= unix:///tmp/ankaios.sock |
There was a problem hiding this comment.
I would go for a more prescriptive name containing UDS or UNIX, etc.
There was a problem hiding this comment.
Renamed it to ANKAIOS_SERVER_SOCKET_URL.
| Set Environment Variable name=ANKSERVER_SERVER_URL value=${ANKAIOS_SERVER_URL} | ||
| Set Environment Variable name=ANKAGENT_SERVER_URL value=${ANKAIOS_SERVER_URL} | ||
| Set Environment Variable name=ANK_SERVER_URL value=${ANKAIOS_SERVER_URL} |
There was a problem hiding this comment.
So now the stests could leave the environment changed in case the variables were not set.
I get the intention as they are normally ran by as in the devcontainer and there we have the variable as you are setting it now.
Let's go for this solution for now. If we get into trouble we could think about using configs in the dev environment and unsetting the vars here.
There was a problem hiding this comment.
I do not know what you mean here? Is your idea you start a shell, execute the stests and afterwards the environment variables of you shell are different? This will never happen as a child process can not change the environment variables of the parent process.
| And the workload "hello3" shall have the execution state "Pending(Initial)" on agent "agent_B" | ||
| # Actions | ||
| When user triggers "ank -k get agents" | ||
| When user triggers "ank get agents" |
There was a problem hiding this comment.
🎆 Yeah! Finally get rid of the -k 👍
krucod3
left a comment
There was a problem hiding this comment.
The changes in the tools are now reviewed too and look good 👍
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>



Issues: #780
Definition of Done
The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.
ToDo