pnoltes commented on a change in pull request #407:
URL: https://github.com/apache/celix/pull/407#discussion_r835944884
##########
File path:
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi.c
##########
@@ -257,8 +257,9 @@ celix_status_t
remoteServiceAdmin_create(celix_bundle_context_t *context, remote
unsigned int port_counter = 0;
do {
-
- const char *options[] = { "listening_ports", newPort,
"num_threads", "5", NULL};
+ char listeningPorts[50]={0};
+ const char *options[] = { "listening_ports", listeningPorts,
"num_threads", "5", NULL};
+
snprintf(listeningPorts,sizeof(listeningPorts),"%s:%s",(*admin)->ip, newPort);
Review comment:
note that this changes the behaviour. Currently the remote service admin
and the endpoint discovery server bind to 0.0.0.0, which means that they are
reach-able from all network interfaces.
The changes in this PR means that both the RSA and endpoint discovery server
is only reachable from the discovered/configured IP.
I also think - but I am not sure - this can be an issue when combined with
container environments (i.e. Kubernetes), because it those situation the public
reachable IP/host can be an unknown IP/host for the container.
Is it an idea to make binding to the RSA_IP an option?
something like:
```
bool bindToAllInterfaces = celix_bundleContext_getPropertyAsBool(context,
CELIX_RSA_BIND_ON_ALL_INTERFACES, CELIX_RSA_BIND_ON_ALL_INTERFACES_DEFAULT);
...
char* listeningPorts = NULL;
if (bindToAllInterfaces) {
asprintf(&listeningPorts, "0.0.0.0:%s", newPort);
} else {
asprintf(&listeningPorts, "%s:%s", (*admin)->ip, newPort);
}
...
free(listeningsPorts);
##########
File path:
bundles/remote_services/discovery_common/src/endpoint_discovery_server.c
##########
@@ -163,11 +163,13 @@ celix_status_t endpointDiscoveryServer_create(discovery_t
*discovery,
char newPort[10];
do {
+ char listeningPorts[50]={0};
const char *options[] = {
- "listening_ports", port,
+ "listening_ports", listeningPorts,
"num_threads", DEFAULT_SERVER_THREADS,
NULL
};
+ snprintf(listeningPorts,sizeof(listeningPorts),"%s:%s",(*server)->ip,
port);
Review comment:
I also prefer the usage of `asprintf`. This will use a malloc and
require a free, but is IMO better then using a arbitrarily max number of
bytes.
##########
File path: bundles/remote_services/discovery_etcd/src/etcd_watcher.c
##########
@@ -134,7 +134,7 @@ static celix_status_t
etcdWatcher_addAlreadyExistingWatchpoints(etcd_watcher_t *
static celix_status_t etcdWatcher_addOwnFramework(etcd_watcher_t *watcher)
{
char localNodePath[MAX_LOCALNODE_LENGTH];
- char *value;
+ char *value=NULL;
Review comment:
nitpick: missing spaces, I prefer:
`char* value = NULL;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
