Fix inverted ipBlocksUpdated() causing dropped Group IPBlocks updates - #8209
Open
Archong-Liu wants to merge 1 commit into
Open
Fix inverted ipBlocksUpdated() causing dropped Group IPBlocks updates#8209Archong-Liu wants to merge 1 commit into
Archong-Liu wants to merge 1 commit into
Conversation
Contributor
Author
|
@antoninbas @luolanzone @jianjuns its a one line logical fix & test, would love some quick feedback, Thanks. |
jianjuns
previously approved these changes
Jul 27, 2026
| // TestUpdateGroupIPBlocksOnlyChange guards against a regression where ipBlocksUpdated() returned | ||
| // whether the old and new IPBlocks were equal instead of whether they differed, causing updateGroup | ||
| // to mistake an IPBlocks-only change for no change and skip updating the internal Group store. | ||
| func TestUpdateGroupIPBlocksOnlyChange(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Cannot we add this case to TestUpdateGroup?
Contributor
Author
There was a problem hiding this comment.
Yeah sure. I originally thought that a Group couldn't have both a NamespaceSelector and IPBlocks set at the same time so was feeling unsure to consolidate them.
But I think the trade off is fair to make the test code cleaner.
updateGroup's ipBlocksUpdated() closure returned oldIPBs.Equal(newIPBs) instead of the negation, unlike the identical helper in clustergroup.go, which correctly returns !oldIPBs.Equal(newIPBs). Because updateGroup skips internalGroupStore.Update() and enqueueInternalGroup() when "!ipBlocksUpdated() && !svcRefUpdated() && !selectorUpdated() && !childGroupsUpdated()" is true, the inverted boolean made an IPBlocks-only change on a Group CR look like "no change" whenever nothing else in the spec changed: the internal Group was left stale and never re-enqueued, so the updated IPBlocks were silently dropped instead of being propagated to policy enforcement. Add a "g-update-ip-block-only-change" case to TestUpdateGroup, which updates only IPBlocks and asserts the internal Group reflects the change. It needs its own starting Group already in IPBlocks mode on both sides, since processGroup returns early on IPBlocks and never sets Selector, so reusing TestUpdateGroup's existing selector-based baseline would also flip selectorUpdated() and mask the regression. Verified it fails against the pre-fix code and passes after the fix. Full networkpolicy package tests pass. Signed-off-by: ArchongLiu <aaarrchong@gmail.com>
Archong-Liu
force-pushed
the
fix/group-ipblocks-updated-inverted
branch
from
August 1, 2026 04:52
cad7969 to
a78d0a8
Compare
jianjuns
approved these changes
Aug 3, 2026
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
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.
updateGroup'sipBlocksUpdated()closure returnedoldIPBs.Equal(newIPBs)instead of the negation, unlike the identical helper inclustergroup.go, which correctly returns!oldIPBs.Equal(newIPBs).Because
updateGroupskipsinternalGroupStore.Update()andenqueueInternalGroup()when!ipBlocksUpdated() && !svcRefUpdated() && !selectorUpdated() && !childGroupsUpdated()is true, the inverted boolean made an IPBlocks-only change on a Group CR look like "no change" whenever nothing else in the spec changed: the internal Group was left stale and never re-enqueued, so the updated IPBlocks were silently dropped instead of being propagated to policy enforcement.Added a
g-update-ip-block-only-changecase to the existingTestUpdateGrouptable instead of a separate top-level test function. It needs its own starting Group already in IPBlocks mode on both sides, rather than reusingTestUpdateGroup's selector-based baseline:processGroupreturns early on IPBlocks and never setsSelector, so transitioning from a selector-based Group to an IPBlocks one would also flipselectorUpdated()and mask whetheripBlocksUpdated()itself is correct. Verified the new case fails against the pre-fix code and passes after the fix, while the other six existing cases in the table stay green either way (none of them actually isolate this regression).Test plan
go test ./pkg/controller/networkpolicy/...passesgo vet/gofmt/golangci-lint(linux + windows) clean