Validate that SDN API object CIDRs are in canonical form #13508
Conversation
|
[test][testextended][extended:networking] |
danwinship
force-pushed
the
cidr-validation
branch
2 times, most recently
from
1e077b4
to
ead561e
Compare
|
@openshift/networking PTAL |
We can glog.Warningf() and then not return an error, but that will just result in a warning in the master logs that they probably won't see. There's no way to return a warning directly to the person trying to create the bad policy. |
|
@openshift/api-review can you give a thumbs up/thumbs down on the breakage of previously-allowed-but-possibly-misinterpreted EgressNetworkPolicy objects as described in the initial comment? |
|
So if this is added, a previously working master could fail to start during an upgrade? This is generally a good example of something we would not tighten validation on (impact to a cluster). If there was a prevalidation check in Ansible it would be less impactful (or at least, can prevent a serious break). Since there are nominally security implications, and this is unlikely to surprise someone who understands networks, I think we could probably get away with it if we ensure we don't break prior to an upgrade (and document this as a breaking change). |
danwinship
force-pushed
the
cidr-validation
branch
from
ead561e
to
d51561a
Compare
Actually, it turns out, no; we were already silently fixing bad clusterNetworkCIDR and serviceNetworkCIDR values before trying to create a ClusterNetwork object from them. The new commit in the latest push changes this to a warning rather than a silent error, but still allows it. So, the PR will not cause any upgrade failures; it only causes problems if you try to modify "bad" objects post-upgrade. |
|
Ok, I'm fine with that. Let's make sure we doc it and double check any
upgrade problems (older nodes?)
On Apr 3, 2017, at 5:14 PM, Dan Winship <notifications@github.com> wrote:
So if this is added, a previously working master could fail to start during
an upgrade?
Actually, it turns out, no; we were already silently fixing bad
clusterNetworkCIDR and serviceNetworkCIDR values before trying to create a
ClusterNetwork object from them.
The new commit in the latest push changes this to a warning rather than a
silent error, but still allows it. So, the PR will not cause any upgrade
failures; it only causes problems if you try to modify "bad" objects
post-upgrade.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#13508 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p7t5RMZndCm16uWwmCTcFc6gQSKAks5rsQzCgaJpZM4MlkdP>
.
|
danwinship
force-pushed
the
cidr-validation
branch
from
d51561a
to
fa31789
Compare
|
Updates LGTM |
|
[merge] |
|
Make sure this is in the release notes and API changes section of the next
release.
On Apr 5, 2017, at 10:08 PM, Ravi Sankar Penta <notifications@github.com> wrote:
*@pravisankar* approved this pull request.
LGTM
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#13508 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p5XAI5ezFzib8CMoeSUPk9BJgs8Oks5rs_ShgaJpZM4MlkdP>
.
|
danwinship
force-pushed
the
cidr-validation
branch
from
fa31789
to
1fef683
Compare
Previously you could say things like clusterNetworkCIDR: "10.128.32.99/14" and the "extra" bits in the address would just be ignored and that would count as "10.128.0.0/14". Make it warn about this now (but still accept it).
Eg, if you want ClusterNetwork to be "10.128.0.0/14", you have to say "10.128.0.0/14", not "10.128.0.1/14" or "10.128.32.99/14". All OpenShift-generated objects already did this correctly, but this might cause previously-considered-valid EgressNetworkPolicy objects to start failing to validate.
danwinship
force-pushed
the
cidr-validation
branch
from
1fef683
to
a0822dd
Compare
|
Evaluated for origin test up to 3a10959 |
|
Evaluated for origin testextended up to 3a10959 |
|
Unit tests failed to compile: |
|
Evaluated for origin merge up to 3a10959 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/275/) (Base Commit: 42c3bfa) (Image: devenv-rhel7_6121) |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/627/) (Base Commit: 42c3bfa) |
yeah, I kept missing that in the output and thinking it was just some sort of infrastructure flake or something. fixed in the latest push so it should go through this time |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/128/) (Base Commit: 42c3bfa) (Extended Tests: networking) |
|
[merge] |
Eg, if you want ClusterNetwork to be "10.128.0.0/14", you have to say "10.128.0.0/14", not "10.128.0.1/14" or "10.128.32.99/14". (net.ParseCIDR() accepts the latter two, because they are valid ways of
referring to hosts within that network, but they aren't valid ways of referring to the network itself, which is what we want here).
Tagging needs-api-review because, in particular, this could cause previously-considered-valid EgressNetworkPolicy objects to start being rejected (existing objects when they're updated, or new objects if the admin is creating them automatically from a buggy script/template or something). I'm claiming that this is a bug fix rather than an API break though, on the grounds that we are currently probably doing the wrong thing with policies like "allow traffic to 192.168.1.15/24" right now; currently we pass it to OVS as-is, and OVS interprets it as meaning "allow traffic to 192.168.1.0/24", but the user probably actually meant "allow traffic to 192.168.1.15/32". (Meaning, they wanted to allow traffic to a single host, and we're allowing traffic to the entire subnet instead.) (It's not clear that anyone is actually making this mistake in production, but if they did, this is what would happen.)
(The same concern theoretically also applies to ClusterNetwork and HostSubnet objects, which are also affected by this patch, but ClusterNetwork would only be checked when restarting master, and it should be obvious what needs to be fixed then; and the HostSubnet.Subnet field is always populated by OpenShift itself, with a correct value, so the extra validation shouldn't hurt anything there.)