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
common/pick_address.cc: Copy public_netw to cluster_netw if cluster empty #12929
common/pick_address.cc: Copy public_netw to cluster_netw if cluster empty #12929
Conversation
Looks good to me, but can't this be done in config_opts.h already? Otherwise this change looks good to me. |
Hi @wido , Not really... Furthermore it needs to be done at the latest point possible because the std values from config_opts.h can be changed at several point during its lifetime. And here is where the values are actually used. And this is where it is actually used to determine the public/cluster IPs. |
lderr(cct) << "Public network was set, but cluster network was not set " << dendl; | ||
lderr(cct) << " Using public network also for cluster network" << dendl; | ||
cct->_conf->set_val("cluster_network", cct->_conf->public_network); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we do this without actually changing the cluster_network option. Just set up a local value and use that below instead...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liewegas
I think that could be done even simpler, and just pass public_network instead of cluster_network in that case.
I'll rewrite the patch.
c6bdb85
to
5027328
Compare
Can you add a short note to PendingReleaseNotes (in ceph.git root) noting the change in behavior so that it will get folded into the release notes? |
@liewegas |
test this please |
@liewegas |
@@ -7,3 +7,12 @@ | |||
in old version would operate on different priority ranges | |||
than new ones. Once upgraded, cluster will operate on | |||
consistent values. | |||
|
|||
* When assigning a network to then public network and not to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/then/the/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise lgtm!
fc8e897
to
5478b70
Compare
@liewegas |
@liewegas |
It changed from another PR that merged.. just rebase |
- When public network is set, but cluster network is not, then the cluster-bindings would be on 0.0.0.0 which could be unexpeted. In this commit we copy the public network into the cluster network to make sure that the cluster backend is not bound on 0.0.0.0 Which could be consideren an insecure, or unexpected, action. Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
…public_netw Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
5478b70
to
2c4010b
Compare
test this please |
@wjwithagen this is redy for merge, sign off needed pls @liewegas FYI |
the cluster-bindings would be on 0.0.0.0 which could be unexpeted.
In this commit we copy the public network into the cluster network
to make sure that the cluster backend is not bound on 0.0.0.0
Which could be consideren an insecure, or unexpected, action.
Signed-off-by: Willem Jan Withagen wjw@digiware.nl