Skip to content

Commit

Permalink
policy: Do not populate reserved policy maps anymore
Browse files Browse the repository at this point in the history 
The datapath no longer uses them, remove all code to keep them up to date

Signed-off-by: Thomas Graf <thomas@cilium.io>
  • Loading branch information
@tgraf
tgraf committed Apr 2, 2018
1 parent 6103362 commit f1d4144
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 111 deletions.
5 changes: 2 additions & 3 deletions pkg/endpoint/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,14 @@ const (

// allowIngressIdentity must be called with global endpoint.Mutex held
func (e *Endpoint) allowIngressIdentity(id identityPkg.NumericIdentity) bool {
return e.Consumable.AllowIngressIdentityLocked(policy.GetConsumableCache(), id)
return e.Consumable.AllowIngressIdentityLocked(id)
}

// allowEgressIdentity allows security identity id to be communicated to by
// this endpoint by updating the endpoint's Consumable.
// Must be called with global endpoint.Mutex held.
func (e *Endpoint) allowEgressIdentity(id identityPkg.NumericIdentity) bool {
cache := policy.GetConsumableCache()
return e.Consumable.AllowEgressIdentityLocked(cache, id)
return e.Consumable.AllowEgressIdentityLocked(id)
}

// ProxyID returns a unique string to identify a proxy mapping.
Expand Down
24 changes: 7 additions & 17 deletions pkg/policy/cache.go
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2016-2017 Authors of Cilium
// Copyright 2016-2018 Authors of Cilium
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -15,14 +15,10 @@
package policy

import (
"fmt"

"github.com/cilium/cilium/pkg/bpf"
"github.com/cilium/cilium/pkg/identity"
"github.com/cilium/cilium/pkg/labels"
"github.com/cilium/cilium/pkg/lock"
"github.com/cilium/cilium/pkg/logging/logfields"
"github.com/cilium/cilium/pkg/maps/policymap"
)

var (
Expand Down Expand Up @@ -113,23 +109,17 @@ func ResolveIdentityLabels(id identity.NumericIdentity) labels.LabelArray {
// calculation, which is done for a specific endpoint when it is regenerated.
func Init() {
for key, val := range identity.ReservedIdentities {
log.WithField(logfields.Identity, key).Debug("creating policy for identity")

policyMapPath := bpf.MapPath(fmt.Sprintf("%sreserved_%d", policymap.MapName, int(val)))

policyMap, _, err := policymap.OpenMap(policyMapPath)
if err != nil {
log.WithError(err).Fatalf("Could not create policy BPF map for reserved identity '%s'", policyMapPath)
}
log.WithField(logfields.Identity, key).Debug("Registering reserved identity")

identity := identity.NewIdentity(val, labels.Labels{
key: labels.NewLabel(val.String(), "", labels.LabelSourceReserved),
})
c := GetConsumableCache().GetOrCreate(val, identity)
if c == nil {

cache := GetConsumableCache()
if c := cache.GetOrCreate(val, identity); c != nil {
GetConsumableCache().addReserved(c)
} else {
log.WithField(logfields.Identity, identity).Fatal("Unable to initialize consumable")
}
GetConsumableCache().addReserved(c)
c.AddMap(policyMap)
}
}
87 changes: 2 additions & 85 deletions pkg/policy/consumer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,36 +214,14 @@ func (c *Consumable) removeFromMaps(id identity.NumericIdentity, trafficDirectio
// IngressIdentities map. Must be called with Consumable mutex Locked.
// Returns true if the identity was not present in this Consumable's
// IngressIdentities map, and thus had to be added, false if it is already added.
func (c *Consumable) AllowIngressIdentityLocked(cache *ConsumableCache, id identity.NumericIdentity) bool {
func (c *Consumable) AllowIngressIdentityLocked(id identity.NumericIdentity) bool {
_, exists := c.IngressIdentities[id]
if !exists {
log.WithFields(logrus.Fields{
logfields.Identity: id,
"consumable": logfields.Repr(c),
}).Debug("Allowing security identity on ingress for consumable")
c.addToPolicyMaps(id, policymap.Ingress)

// If id corresponds to a reserved identity, Consumable corresponding to
// that security identity needs to be updated explicitly, as reserved
// identities do not have a corresponding endpoint for which policy
// recalculation (when Consumables are updated) is done.
if id.IsReservedIdentity() {
reservedConsumable := cache.Lookup(id)
if reservedConsumable != nil {
// If we are accessing the same Consumable (allowing traffic
// to itself), we don't need to take its mutex because it was
// already taken before calling this function.
if id != c.ID {
reservedConsumable.Mutex.Lock()
reservedConsumable.AllowIngressIdentityLocked(cache, c.ID)
reservedConsumable.Mutex.Unlock()
} else {
reservedConsumable.AllowIngressIdentityLocked(cache, c.ID)
}
} else {
log.WithField(logfields.Identity, id).Warningf("unable to allow ingress from identity %d", c.ID)
}
}
}

c.IngressIdentities[id] = true
Expand All @@ -255,35 +233,14 @@ func (c *Consumable) AllowIngressIdentityLocked(cache *ConsumableCache, id ident
// EgressIdentities map. Must be called with Consumable mutex Locked.
// Returns true if the identity was not present in this Consumable's
// EgressIdentities map, and thus had to be added, false if it is already added.
func (c *Consumable) AllowEgressIdentityLocked(cache *ConsumableCache, id identity.NumericIdentity) bool {
func (c *Consumable) AllowEgressIdentityLocked(id identity.NumericIdentity) bool {
_, exists := c.EgressIdentities[id]
if !exists {
log.WithFields(logrus.Fields{
logfields.Identity: id,
"consumable": logfields.Repr(c),
}).Debug("New egress security identity for consumable")

c.addToPolicyMaps(id, policymap.Egress)

// If id corresponds to a reserved identity, Consumable corresponding to
// that security identity needs to be updated explicitly, as reserved
// identities do not have a corresponding endpoint for which policy
// recalculation (when Consumables are updated) is done.
if id.IsReservedIdentity() {
reservedConsumable := cache.Lookup(id)
if reservedConsumable != nil {
// Avoid deadlock ; is this necessary? Being cautious.
if id != c.ID {
reservedConsumable.Mutex.Lock()
reservedConsumable.AllowEgressIdentityLocked(cache, c.ID)
reservedConsumable.Mutex.Unlock()
} else {
reservedConsumable.AllowEgressIdentityLocked(cache, c.ID)
}
} else {
log.WithField(logfields.Identity, id).Warningf("unable to allow egress to identity %d", c.ID)
}
}
}
c.EgressIdentities[id] = true
return !exists // not changed.
Expand All @@ -296,27 +253,6 @@ func (c *Consumable) RemoveIngressIdentityLocked(id identity.NumericIdentity) {
if _, ok := c.IngressIdentities[id]; ok {
log.WithField(logfields.Identity, id).Debug("Removing identity from ingress map")
delete(c.IngressIdentities, id)

// Consumables corresponding to reserved identities need to be updated
// explicitly because they are not updated or regenerated.
if id.IsReservedIdentity() {
reservedConsumable := c.cache.Lookup(id)
if reservedConsumable != nil {
// If we are accessing the same Consumable (allowing traffic
// to itself), we don't need to take its mutex because it was
// already taken before calling this function.
if id != c.ID {
reservedConsumable.Mutex.Lock()
reservedConsumable.RemoveIngressIdentityLocked(c.ID)
reservedConsumable.Mutex.Unlock()
} else {
reservedConsumable.RemoveIngressIdentityLocked(c.ID)
}
} else {
log.WithField(logfields.Identity, id).Warningf("unable to disallow ingress from identity %d", c.ID)
}

}
c.removeFromMaps(id, policymap.Ingress)
}
}
Expand All @@ -328,25 +264,6 @@ func (c *Consumable) RemoveEgressIdentityLocked(id identity.NumericIdentity) {
if _, ok := c.EgressIdentities[id]; ok {
log.WithField(logfields.Identity, id).Debug("Removing egress identity")
delete(c.EgressIdentities, id)

// Consumables corresponding to reserved identities need to be updated
// explicitly because they are not updated or regenerated.
if id.IsReservedIdentity() {
reservedConsumable := c.cache.Lookup(id)
if reservedConsumable != nil {
// Avoid deadlock!
if id != c.ID {
reservedConsumable.Mutex.Lock()
reservedConsumable.RemoveEgressIdentityLocked(c.ID)
reservedConsumable.Mutex.Unlock()
} else {
reservedConsumable.RemoveEgressIdentityLocked(c.ID)
}
} else {
log.WithField(logfields.Identity, id).Warningf("unable to disallow egress to identity %d", c.ID)
}
}

c.removeFromMaps(id, policymap.Egress)
}
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/policy/consumer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,17 @@ func (s *PolicyTestSuite) TestIngressIdentityAllowed(c *C) {
c.Assert(c1.AllowsIngress(ID2), Equals, false)
c.Assert(c1.AllowsIngress(ID3), Equals, false)

c1.AllowIngressIdentityLocked(cache, ID2)
c1.AllowIngressIdentityLocked(ID2)
c.Assert(c1.AllowsIngress(ID2), Equals, true)
id2Allowed, _ := c1.IngressIdentities[ID2]
c.Assert(id2Allowed, Equals, true)

c1.AllowIngressIdentityLocked(cache, ID2)
c1.AllowIngressIdentityLocked(ID2)
c.Assert(c1.AllowsIngress(ID2), Equals, true)
id2Allowed, _ = c1.IngressIdentities[ID2]
c.Assert(id2Allowed, Equals, true)

c1.AllowIngressIdentityLocked(cache, ID3)
c1.AllowIngressIdentityLocked(ID3)
c.Assert(c1.AllowsIngress(ID3), Equals, true)
id3Allowed, _ := c1.IngressIdentities[ID3]
c.Assert(id3Allowed, Equals, true)
Expand All @@ -78,17 +78,17 @@ func (s *PolicyTestSuite) TestEgressIdentityAllowed(c *C) {
c.Assert(c1.AllowsEgress(ID2), Equals, false)
c.Assert(c1.AllowsEgress(ID3), Equals, false)

c1.AllowEgressIdentityLocked(cache, ID2)
c1.AllowEgressIdentityLocked(ID2)
c.Assert(c1.AllowsEgress(ID2), Equals, true)
id2Allowed, _ := c1.EgressIdentities[ID2]
c.Assert(id2Allowed, Equals, true)

c1.AllowEgressIdentityLocked(cache, ID2)
c1.AllowEgressIdentityLocked(ID2)
c.Assert(c1.AllowsEgress(ID2), Equals, true)
id2Allowed, _ = c1.EgressIdentities[ID2]
c.Assert(id2Allowed, Equals, true)

c1.AllowEgressIdentityLocked(cache, ID3)
c1.AllowEgressIdentityLocked(ID3)
c.Assert(c1.AllowsEgress(ID3), Equals, true)
id3Allowed, _ := c1.EgressIdentities[ID3]
c.Assert(id3Allowed, Equals, true)
Expand Down

0 comments on commit f1d4144

Please sign in to comment.