Skip to content

Commit

Permalink
k8s: Remove unnecessary AllocatePrefixes from RuleTranslator
Browse files Browse the repository at this point in the history 
This variable is no longer necessary because it doesn't actually prevent
ipcache interaction as of commit 4b87ccc ("pkg/k8s/watcher: fix
deadlock with service event handler & CES watcher."). Remove it as
provides no functional impact.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
  • Loading branch information
@christarazi
christarazi committed Jun 7, 2023
1 parent c0cce57 commit 647f901
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 49 deletions.
45 changes: 16 additions & 29 deletions pkg/k8s/rule_translate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ type RuleTranslator struct {
OldEndpoint, NewEndpoint Endpoints
ServiceLabels map[string]string
Revert bool
AllocatePrefixes bool
}

// Translate calls TranslateEgress on all r.Egress rules
Expand Down Expand Up @@ -64,7 +63,7 @@ func (k RuleTranslator) TranslateEgress(r *api.EgressRule, result *policy.Transl
func (k RuleTranslator) populateEgress(r *api.EgressRule, result *policy.TranslationResult) error {
for _, service := range r.ToServices {
if k.serviceMatches(service) {
if backendPrefixes, err := k.generateToCidrFromEndpoint(r, k.NewEndpoint, k.AllocatePrefixes); err != nil {
if backendPrefixes, err := k.generateToCidrFromEndpoint(r, k.NewEndpoint); err != nil {
return err
} else {
result.PrefixesToAdd = append(result.PrefixesToAdd, backendPrefixes...)
Expand All @@ -81,7 +80,7 @@ func (k RuleTranslator) depopulateEgress(r *api.EgressRule, result *policy.Trans
// counting rules twice
result.NumToServicesRules++
if k.serviceMatches(service) {
if prefixesToRelease, err := k.deleteToCidrFromEndpoint(r, k.OldEndpoint, k.AllocatePrefixes); err != nil {
if prefixesToRelease, err := k.deleteToCidrFromEndpoint(r, k.OldEndpoint); err != nil {
return err
} else {
result.PrefixesToRelease = append(result.PrefixesToRelease, prefixesToRelease...)
Expand Down Expand Up @@ -114,17 +113,8 @@ func (k RuleTranslator) serviceMatches(service api.Service) bool {
func (k RuleTranslator) generateToCidrFromEndpoint(
egress *api.EgressRule,
endpoints Endpoints,
allocatePrefixes bool) ([]netip.Prefix, error) {

var prefixes []netip.Prefix

// allocatePrefixes if true here implies that this translation is
// occurring after policy import. This means that the CIDRs were not
// known at that time, so the IPCache hasn't been informed about them.
// In this case, it's the job of this Translator to notify the IPCache.
if allocatePrefixes {
prefixes = endpoints.Prefixes()
}
) ([]netip.Prefix, error) {
prefixes := endpoints.Prefixes()

// This will generate one-address CIDRs consisting of endpoint backend ip
for addrCluster := range endpoints.Backends {
Expand Down Expand Up @@ -165,7 +155,7 @@ func (k RuleTranslator) generateToCidrFromEndpoint(
func (k RuleTranslator) deleteToCidrFromEndpoint(
egress *api.EgressRule,
endpoints Endpoints,
releasePrefixes bool) ([]netip.Prefix, error) {
) ([]netip.Prefix, error) {

var toReleasePrefixes []netip.Prefix
delCIDRRules := make(map[int]*api.CIDRRule, len(egress.ToCIDRSet))
Expand Down Expand Up @@ -204,13 +194,11 @@ func (k RuleTranslator) deleteToCidrFromEndpoint(
return toReleasePrefixes, nil
}

if releasePrefixes {
delSlice := make([]api.CIDRRule, 0, len(egress.ToCIDRSet))
for _, delCIDRRule := range delCIDRRules {
delSlice = append(delSlice, *delCIDRRule)
}
toReleasePrefixes = policy.GetPrefixesFromCIDRSet(delSlice)
delSlice := make([]api.CIDRRule, 0, len(egress.ToCIDRSet))
for _, delCIDRRule := range delCIDRRules {
delSlice = append(delSlice, *delCIDRRule)
}
toReleasePrefixes = policy.GetPrefixesFromCIDRSet(delSlice)

// if endpoint is not in CIDR or it's not generated it's ok to retain it
newCIDRRules := make([]api.CIDRRule, 0, len(egress.ToCIDRSet)-len(delCIDRRules))
Expand Down Expand Up @@ -243,7 +231,7 @@ func PreprocessRules(r api.Rules, cache *ServiceCache) error {
if ok && svc.IsExternal() {
eps := ep.GetEndpoints()
if eps != nil {
t := NewK8sTranslator(ns, Endpoints{}, *eps, false, svc.Labels, false)
t := NewK8sTranslator(ns, Endpoints{}, *eps, false, svc.Labels)
// We don't need to check the translation result here because the k8s
// RuleTranslator above sets allocatePrefixes to be false.
err := t.Translate(rule, &policy.TranslationResult{})
Expand All @@ -265,13 +253,12 @@ func NewK8sTranslator(
oldEPs, newEPs Endpoints,
revert bool,
labels map[string]string,
allocatePrefixes bool) RuleTranslator {
) RuleTranslator {
return RuleTranslator{
Service: serviceInfo,
OldEndpoint: oldEPs,
NewEndpoint: newEPs,
ServiceLabels: labels,
Revert: revert,
AllocatePrefixes: allocatePrefixes,
Service: serviceInfo,
OldEndpoint: oldEPs,
NewEndpoint: newEPs,
ServiceLabels: labels,
Revert: revert,
}
}
34 changes: 16 additions & 18 deletions pkg/k8s/rule_translate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ func TestTranslate(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
lator := tt.translator()
args := tt.args()
tr := NewK8sTranslator(lator.sid, lator.oldEPs, lator.newEPs, lator.revert, lator.labels, true)
tr := NewK8sTranslator(lator.sid, lator.oldEPs, lator.newEPs, lator.revert, lator.labels)
err := tr.Translate(args.rule, args.result)
assert.Equal(t, tt.expected, err)
assert.Equal(t, tt.expectedResult, args.result)
Expand Down Expand Up @@ -262,7 +262,7 @@ func (s *K8sSuite) TestTranslatorDirect(c *C) {
Labels: tag1,
}

translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{}, false)
translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{})

_, _, err := repo.Add(rule1, []policy.Endpoint{})
c.Assert(err, IsNil)
Expand All @@ -276,7 +276,7 @@ func (s *K8sSuite) TestTranslatorDirect(c *C) {
c.Assert(len(rule.ToCIDRSet), Equals, 1)
c.Assert(string(rule.ToCIDRSet[0].Cidr), Equals, epAddrCluster.Addr().String()+"/32")

translator = NewK8sTranslator(serviceInfo, endpointInfo, endpointInfo, true, map[string]string{}, false)
translator = NewK8sTranslator(serviceInfo, endpointInfo, endpointInfo, true, map[string]string{})
result, err = repo.TranslateRules(translator)
c.Assert(result.NumToServicesRules, Equals, 1)

Expand Down Expand Up @@ -318,7 +318,7 @@ func (s *K8sSuite) TestServiceMatches(c *C) {
},
}

translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, svcLabels, false)
translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, svcLabels)
c.Assert(translator.serviceMatches(service), Equals, true)
}

Expand Down Expand Up @@ -368,7 +368,7 @@ func (s *K8sSuite) TestTranslatorLabels(c *C) {
Labels: tag1,
}

translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, svcLabels, false)
translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, svcLabels)

_, _, err := repo.Add(rule1, []policy.Endpoint{})
c.Assert(err, IsNil)
Expand All @@ -382,7 +382,7 @@ func (s *K8sSuite) TestTranslatorLabels(c *C) {
c.Assert(len(rule.ToCIDRSet), Equals, 1)
c.Assert(string(rule.ToCIDRSet[0].Cidr), Equals, epAddrCluster.Addr().String()+"/32")

translator = NewK8sTranslator(serviceInfo, endpointInfo, endpointInfo, true, svcLabels, false)
translator = NewK8sTranslator(serviceInfo, endpointInfo, endpointInfo, true, svcLabels)
result, err = repo.TranslateRules(translator)

rule = repo.SearchRLocked(tag1)[0].Egress[0]
Expand Down Expand Up @@ -423,10 +423,9 @@ func (s *K8sSuite) TestGenerateToCIDRFromEndpoint(c *C) {
Namespace: "default",
}

translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{}, false)
prefixesToAllocate, err := translator.generateToCidrFromEndpoint(rule, endpointInfo, false)
translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{})
_, err := translator.generateToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)
c.Assert(len(prefixesToAllocate), Equals, 0, Commentf("if allocatePrefixes is false, then it should return nothing"))
cidrs := rule.ToCIDRSet.StringSlice()
sort.Strings(cidrs)
c.Assert(len(cidrs), Equals, 2)
Expand All @@ -436,9 +435,8 @@ func (s *K8sSuite) TestGenerateToCIDRFromEndpoint(c *C) {
})

// second run, to make sure there are no duplicates added
prefixesToAllocate, err = translator.generateToCidrFromEndpoint(rule, endpointInfo, true)
prefixesToAllocate, err := translator.generateToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)
c.Assert(len(prefixesToAllocate), Equals, 2, Commentf("if allocatePrefixes is true, then it should list of prefixes to allocate"))
_, epIP1Prefix, err := net.ParseCIDR(epAddrCluster1.Addr().String() + "/32")
c.Assert(err, IsNil)
_, epIP2Prefix, err := net.ParseCIDR(epAddrCluster2.Addr().String() + "/32")
Expand All @@ -460,12 +458,12 @@ func (s *K8sSuite) TestGenerateToCIDRFromEndpoint(c *C) {
epAddrCluster2.Addr().String() + "/32",
})

_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo, false)
_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)
c.Assert(len(rule.ToCIDRSet), Equals, 0)

// third run, to make sure there are no duplicates added
_, err = translator.generateToCidrFromEndpoint(rule, endpointInfo, false)
_, err = translator.generateToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)

cidrs = rule.ToCIDRSet.StringSlice()
Expand All @@ -477,7 +475,7 @@ func (s *K8sSuite) TestGenerateToCIDRFromEndpoint(c *C) {
})

// and one final delete
_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo, false)
_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)
c.Assert(len(rule.ToCIDRSet), Equals, 0)
}
Expand Down Expand Up @@ -577,21 +575,21 @@ func (s *K8sSuite) TestDontDeleteUserRules(c *C) {
Namespace: "default",
}

translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{}, false)
_, err := translator.generateToCidrFromEndpoint(rule, endpointInfo, false)
translator := NewK8sTranslator(serviceInfo, Endpoints{}, endpointInfo, false, map[string]string{})
_, err := translator.generateToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)

c.Assert(len(rule.ToCIDRSet), Equals, 2)
c.Assert(string(rule.ToCIDRSet[1].Cidr), Equals, epAddrCluster.Addr().String()+"/32")

// second run, to make sure there are no duplicates added
_, err = translator.generateToCidrFromEndpoint(rule, endpointInfo, false)
_, err = translator.generateToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)

c.Assert(len(rule.ToCIDRSet), Equals, 2)
c.Assert(string(rule.ToCIDRSet[1].Cidr), Equals, epAddrCluster.Addr().String()+"/32")

_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo, false)
_, err = translator.deleteToCidrFromEndpoint(rule, endpointInfo)
c.Assert(err, IsNil)
c.Assert(len(rule.ToCIDRSet), Equals, 1)
c.Assert(string(rule.ToCIDRSet[0].Cidr), Equals, string(userCIDR))
Expand Down
4 changes: 2 additions & 2 deletions pkg/k8s/watchers/watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ func (k *K8sWatcher) k8sServiceHandler() {
if event.OldEndpoints != nil {
oldEP = *event.OldEndpoints
}
translator := k8s.NewK8sTranslator(event.ID, oldEP, *event.Endpoints, false, svc.Labels, true)
translator := k8s.NewK8sTranslator(event.ID, oldEP, *event.Endpoints, false, svc.Labels)
result, err := k.policyRepository.TranslateRules(translator)
if err != nil {
log.WithError(err).Error("Unable to repopulate egress policies from ToService rules")
Expand Down Expand Up @@ -645,7 +645,7 @@ func (k *K8sWatcher) k8sServiceHandler() {
if event.OldEndpoints != nil {
oldEP = *event.OldEndpoints
}
translator := k8s.NewK8sTranslator(event.ID, oldEP, *event.Endpoints, true, svc.Labels, true)
translator := k8s.NewK8sTranslator(event.ID, oldEP, *event.Endpoints, true, svc.Labels)
result, err := k.policyRepository.TranslateRules(translator)
if err != nil {
log.WithError(err).Error("Unable to depopulate egress policies from ToService rules")
Expand Down

0 comments on commit 647f901

Please sign in to comment.