Add avoidBuggyIPs to the IPAddressPool CIDR - #1515
Merged
Merged
Conversation
fedepaol
requested review from
gclawes,
johananl,
oribon,
rata and
russellb
as code owners
July 18, 2022 09:50
fedepaol
force-pushed
the
restoreavoidbuggyips
branch
from
July 18, 2022 10:15
8831f2c to
2cbda9d
Compare
oribon
reviewed
Jul 18, 2022
| - Change the validating webhook configuration name to metallb-webhook-configuration instead of validating-webhook-configuration ([PR 1497](https://github.com/metallb/metallb/pull/1497)) | ||
| - L2 mode not working with ipv4 only nodes ([Issue 1507](https://github.com/metallb/metallb/issues/1507) , [PR 1506](https://github.com/metallb/metallb/pull/1506)) | ||
| - Add the AvoidBuggyIPs flag to the IPAddressPool CRD. Converting a CIDR to a range comes with limitation related | ||
| to setting the aggregation lenght and validating it. ([Issue 1495](https://github.com/metallb/metallb/issues/1495), |
fedepaol
force-pushed
the
restoreavoidbuggyips
branch
from
July 18, 2022 12:56
2cbda9d to
42d2479
Compare
This reverts commit e4d0d18. We restore the avoidbuggyIP files because setting ranges is not syntactically equivalent, if used in conjunction with aggregation length. A 10.0.1/24 range will become 10.0.1.1 - 10.0.1.254 which will fail a check against the /24 aggregation length (while it should not). Attempting to reverse the intention of the user is risky and prone to corner cases, so it's better to restore the original configurability option.
The original port from the operator inadvertently added the field to the legacy addresspool too. Here we are removing it to bring parity with the old api and to make the field exist only in the new and stable API. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Regenerate the manifests to be aligned with the change. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Adding it to the charts after restoring it to the apis. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Regenerating the apidocs after adding the avoidbuggyips flag Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Now that the avoidbuggyips flag is added to the ipaddresspool api, we can convert it straight away and not convert cidrs to ranges. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
Adding the item to the release notes. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
fedepaol
force-pushed
the
restoreavoidbuggyips
branch
from
July 18, 2022 15:04
42d2479 to
f267c40
Compare
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We originally thought that the avoidBuggyIPs flag was replaceable by picking the equivalent range of the cidr minus the extremes.
The issue is, the aggregation length validation won't work in that case because a 10.0.1.1-10.1.1.254 range doesn't correspond to a /24 cidr.
Because of that, we believe that bringing the flag back is safer than trying to understand if a range belongs to a cidr minus the buggy ips (and if the user's intention was to avoid them in first place).