TL;DR
I’ve been looking at #10926 (the fix for #39159, now merged into develop). After testing it on a develop environment, I found two behaviors around input validation that I’d like to discuss:
- An invalid mask slips through validation and gets saved — but only when the network address field contains a suffix (e.g.
/24). I believe this is a bug. - Whether the suffix is applied depends on the state of the mask (valid / invalid / blank), so the behavior is inconsistent. Conflicting input is also saved without warning.
On top of that, I’d like to settle how the suffix in the network address field should be handled in general (reject / strip / a middle ground). My preference is to reject it. Once we agree on a direction, I’ll open a follow-up PR with regression tests. Details below. @lstejska
Background
I commented on the validation direction earlier on the PR itself, but the above surfaced when I checked the actual behavior after the merge, so I’m sharing it here. For context, I tested on develop and confirmed that app/models/subnet.rb contains strip_network_cidr (called from normalize_addresses as a before_validation hook). The relevant line is:
self.cidr = parts[1].to_i if mask.blank? || cidr.nil?
This means the suffix from the address is only adopted when the mask is blank or when the mask can’t be parsed (i.e. invalid). That condition seems to lead to the two behaviors below. Here are reproductions with hammer (organization/location options omitted for brevity).
1. An invalid mask slips through validation
Passing the same invalid mask (not-a-mask) gives different results depending on whether the address carries a suffix:
# No suffix -> rejected as expected
hammer subnet create --name example-no-suffix --network 192.168.90.0 --mask not-a-mask
Could not create the subnet:
Cidr can't be blank
Network mask is invalid
# With suffix (same invalid mask) -> created
hammer subnet create --name example-with-suffix --network 192.168.91.0/24 --mask not-a-mask
Subnet created.
# info: network 192.168.91.0 / cidr 24 / mask 255.255.255.0
In the suffixed case, the invalid mask is silently replaced with 255.255.255.0 and saved. In other words, mask validation stops taking effect the moment a suffix is added to the address. A typo in the mask can be quietly swallowed via the suffix, which I don’t think is the intended behavior.
2. Whether the suffix is adopted depends on the state of the mask
Even with the same “address plus suffix”, whether the suffix wins flips depending on the mask:
| Mask state | Example input | Result | Saved cidr |
|---|---|---|---|
| Valid (255.255.0.0 = /16) | network 192.168.60.0/24 + mask 255.255.0.0 |
suffix ignored, /16 wins | 16 |
| Blank (unspecified) | network 192.168.61.0/24 (no mask/prefix) |
suffix adopted (normal completion) | 24 |
| Invalid (not-a-mask) | network 192.168.91.0/24 + mask not-a-mask |
suffix adopted, invalid mask swallowed | 24 |
When a valid mask is passed, the suffix is ignored (even with /24, mask 255.255.0.0 takes priority and it’s saved as /16); only when the mask is invalid does the suffix become the source of truth. In addition, input where the suffix and the prefix/mask disagree (e.g. address /24 with prefix 16) is saved as-is without any warning. I confirmed the same in the WebUI — conflicting input is saved without an error.
Options and my take
I believe #1 above is a clear bug that should be fixed — does that seem right? Beyond that, I’d like to decide how the suffix in the network address field should be handled in the first place. I see three options:
- (a) If a suffix (e.g.
/24) is entered in the network address field, treat it as invalid input and reject it with a validation error (prefix length continues to be specified in the Network Prefix field as before). - (b) Keep the current behavior: accept the suffix in the network address field, strip it, and don’t store it as a value (the strip approach, #10926).
- (c) Strip only the redundant case where the suffix is well-formed and matches the other fields (prefix/mask); reject an invalid suffix or one that conflicts with the other fields (middle ground).
I lean toward (a), for reasons on both the development and the operator side.
From a development standpoint:
- Prefix length is managed in the Network Prefix field, so a suffix in the address field is duplicate information.
- The Network Prefix field rejects invalid values, whereas the address field silently normalizes — that’s inconsistent behavior across fields.
- The implementation is a simple check: reject if a suffix is present.
From an operator standpoint (the people actually creating subnets):
- With the current approach, a typo in the mask isn’t rejected; you can end up in a “saved, but with a value different from what I intended” state. Tracking down that drift later makes it harder to work with, not easier.
- With the reject approach, bad input errors out on the spot, which prevents a wrong configuration from being saved unnoticed.
- Currently, when the suffix and prefix/mask disagree, which one wins depends on the mask state (the result flips between a valid and an invalid mask). A consistent rule — “don’t put a suffix in the address; put the prefix in its dedicated field” — makes the behavior more predictable for operators too.
For reference, here’s a rough implementation sketch for (a). The idea is to drop the merged strip_network_cidr and instead reject a network that contains a suffix:
validate :validate_ranges
validate :check_if_type_changed, :on => :update
+ validate :network_must_not_contain_prefix
+ def network_must_not_contain_prefix
+ return unless network.present? && network.to_s.include?('/')
+
+ errors.add(:network, _("is invalid"))
+ end
(This assumes strip_network_cidr and its call are removed as well. I’ll provide the actual implementation in the follow-up PR.)
As for (c):
- It would have to handle a combination of conditions — whether a suffix is present, whether the suffix itself is well-formed, whether it matches the other fields, and whether mask or prefix is specified — so the number of cases grows and the implementation gets more complex.
- I understand the convenience angle from #10926 (being able to paste CIDR notation directly), so (c) is a possible compromise, but the added complexity is a tradeoff worth weighing.
Once we agree on a direction, I’ll open a follow-up PR with regression tests. Feedback would be much appreciated.