Add parameter to disable default vlan #875
Merged
Conversation
This file contains 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
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
2 times, most recently
from
7760bc0 to
1203127
Compare
maiqueb
reviewed
Apr 3, 2023
maiqueb
reviewed
Apr 3, 2023
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
2 times, most recently
from
18dc732 to
5428eeb
Compare
phoracek
reviewed
Apr 3, 2023
| HairpinMode bool `json:"hairpinMode"` | ||
| PromiscMode bool `json:"promiscMode"` | ||
| Vlan int `json:"vlan"` | ||
| PreserveDefaultVlan bool `json:"preserveDefaultVlan"` |
There was a problem hiding this comment.
There was a problem hiding this comment.
Should this be a *bool to preserve the existing behavior?
There was a problem hiding this comment.
That's another option.
Right now, the default value of this parameter is set to true in line 98 (before unmarshaling the config). So, users have to explicitly set it to false to disable it. If it is not present, the value is set to true.
I didn't want this param to be different than the others, that's why I didn't want to do it with a pointer. Also, because it will need an additional check.
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
3 times, most recently
from
6532d58 to
58e441c
Compare
maiqueb
reviewed
Apr 3, 2023
maiqueb
reviewed
Apr 3, 2023
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
2 times, most recently
from
38a8b1b to
7e50dd4
Compare
|
Thank you @mlguerrero12. The only think holding back the lgtm (from my perspective) is documenting the new attribute in the README. |
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
from
7e50dd4 to
1f5c07a
Compare
s1061123
reviewed
Apr 4, 2023
maiqueb
approved these changes
Apr 5, 2023
There was a problem hiding this comment.
@mlguerrero12 I had forgotten the README is defined in a separate project.
This looks good to me. Please address the other reviewers comments.
This new parameter allows users to remove the default vlan Fixes: containernetworking#667 Signed-off-by: Marcelo Guerrero Viveros <[email protected]>
mlguerrero12
force-pushed
the
adddefaultvlanparam
branch
from
1f5c07a to
821982d
Compare
mlguerrero12
added a commit
to mlguerrero12/cni.dev
that referenced
this pull request
Apr 14, 2023
Linked to containernetworking/plugins#875 Signed-off-by: Marcelo Guerrero Viveros <[email protected]>
|
@phoracek, @maiqueb, please have a look at containernetworking/cni.dev#119 |
squeed
approved these changes
Apr 17, 2023
phoracek
added a commit
to phoracek/cluster-network-addons-operator
that referenced
this pull request
May 23, 2023
This bump brings in containernetworking/plugins#875 This change is required to improve the VLAN isolation done by bridge CNI. This bump is unorthodox since it does not only cherry-pick the bug fix, but also a few other patches. This is due to the lack of a stable branch in CNI plugins. We reviewed the parasiting patches that are introduced by this bump and evaluated that they are not disruptive to our typical use of the bridge CNI. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2209318 Signed-off-by: Petr Horacek <[email protected]>
oshoval
pushed a commit
to oshoval/cluster-network-addons-operator
that referenced
this pull request
Jun 19, 2023
This bump brings in containernetworking/plugins#875 This change is required to improve the VLAN isolation done by bridge CNI. This bump is unorthodox since it does not only cherry-pick the bug fix, but also a few other patches. This is due to the lack of a stable branch in CNI plugins. We reviewed the parasiting patches that are introduced by this bump and evaluated that they are not disruptive to our typical use of the bridge CNI. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2209318 Signed-off-by: Petr Horacek <[email protected]>
kubevirt-bot
pushed a commit
to kubevirt/cluster-network-addons-operator
that referenced
this pull request
Jun 26, 2023
This bump brings in containernetworking/plugins#875 This change is required to improve the VLAN isolation done by bridge CNI. This bump is unorthodox since it does not only cherry-pick the bug fix, but also a few other patches. This is due to the lack of a stable branch in CNI plugins. We reviewed the parasiting patches that are introduced by this bump and evaluated that they are not disruptive to our typical use of the bridge CNI. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2209318 Signed-off-by: Petr Horacek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This new parameter allows users to remove the default vlan
Fixes: #667