Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[client] Account different policies rules for routes firewall rules #2939

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mlsmaycon
Copy link
Collaborator

Describe your changes

This fixes the behavior where multiple policies with different access levels were applied to all distribution group peers.

Now, we ensure that route firewall rules generation will consider source group peers of access control policies and apply rules to peers from these groups. Peers from the distribution group that don't belong to any source group will have their traffic dropped.

This will change behavior for existing and valid policies benefiting from this flaw.

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

This change ensures that route firewall rules will consider source group peers in the rules generation for access control policies.

This fixes the behavior where multiple policies with different levels of access was being applied to all peers in a distribution group

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (2)

management/server/route.go:454

  • The check 'if pID == peerID' might be incorrect if the intention is to include the current peer in some cases. Verify if this is the intended behavior.
if pID == peerID {

management/server/route.go:422

  • The iteration over 'route.AccessControlGroups' twice in the 'getPeerRoutesFirewallRules' function might be redundant. Ensure that this is necessary.
for _, accessGroup := range route.AccessControlGroups {
Copy link

sonarcloud bot commented Nov 23, 2024

@@ -1770,7 +1886,7 @@ func TestAccount_getPeersRoutesFirewall(t *testing.T) {
IsDynamic: true,
},
}
assert.ElementsMatch(t, routesFirewallRules, expectedRoutesFirewallRules)
assert.ElementsMatch(t, orderRuleSourceRanges(routesFirewallRules), orderRuleSourceRanges(expectedRoutesFirewallRules))
Copy link
Contributor

@lixmal lixmal Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func ElementsMatch(t TestingT, listA interface{}, listB interface{}, msgAndArgs ...interface{}) (ok bool)

ElementsMatch asserts that the specified listA(array, slice...) is equal to specified listB(array, slice...) ignoring the order of the elements. If there are duplicate elements, the number of appearances of each of them in both lists should match.

This works, without the change in this line, doesn't it?

There's another a few lines up

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, I am just keeping the same style to avoid future issues and because this is a testing func.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants