Handle forbiddenError for XPN clusters by raising event

controllers/gce/controller/cluster_manager.go

@@ -102,6 +102,9 @@ func (c *ClusterManager) shutdown() error {
 		return err
 	}
 	if err := c.firewallPool.Shutdown(); err != nil {
+		if _, ok := err.(*firewalls.FirewallSyncError); ok {
+			return nil
+		}
 		return err
 	}
 	// The backend pool will also delete instance groups.
@@ -159,11 +162,9 @@ func (c *ClusterManager) Checkpoint(lbs []*loadbalancers.L7RuntimeInfo, nodeName
 	for _, p := range fwNodePorts {
 		np = append(np, p.Port)
 	}
-	if err := c.firewallPool.Sync(np, nodeNames); err != nil {
-		return igs, err
-	}
 
-	return igs, nil
+	err = c.firewallPool.Sync(np, nodeNames)
+	return igs, err
 }
 
 func (c *ClusterManager) EnsureInstanceGroupsAndPorts(servicePorts []backends.ServicePort) ([]*compute.InstanceGroup, error) {

controllers/gce/controller/controller.go

@@ -36,6 +36,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
 
+	"k8s.io/ingress/controllers/gce/firewalls"
 	"k8s.io/ingress/controllers/gce/loadbalancers"
 )
 
@@ -318,18 +319,28 @@ func (lbc *LoadBalancerController) sync(key string) (err error) {
 		}
 		glog.V(3).Infof("Finished syncing %v", key)
 	}()
+
+	// TODO: Implement proper backoff for the queue.
+	eventMsg := "GCE"
+
 	// Record any errors during sync and throw a single error at the end. This
 	// allows us to free up associated cloud resources ASAP.
 	igs, err := lbc.CloudClusterManager.Checkpoint(lbs, nodeNames, gceNodePorts, allNodePorts)
 	if err != nil {
-		// TODO: Implement proper backoff for the queue.
+		if fwErr, ok := err.(*firewalls.FirewallSyncError); ok {
-		eventMsg := "GCE"
+			if ingExists {
-		if ingExists {
+				lbc.recorder.Eventf(obj.(*extensions.Ingress), apiv1.EventTypeNormal, eventMsg, fwErr.Message)
-			lbc.recorder.Eventf(obj.(*extensions.Ingress), apiv1.EventTypeWarning, eventMsg, err.Error())
+			} else {
+				glog.Warningf("Received firewallSyncError but don't have an ingress for raising an event: %v", fwErr.Message)
+			}
 		} else {
-			err = fmt.Errorf("%v, error: %v", eventMsg, err)
+			if ingExists {
+				lbc.recorder.Eventf(obj.(*extensions.Ingress), apiv1.EventTypeWarning, eventMsg, err.Error())
+			} else {
+				err = fmt.Errorf("%v, error: %v", eventMsg, err)
+			}
+			syncError = err
 		}
-		syncError = err
 	}
 
 	if !ingExists {
@@ -341,11 +352,10 @@ func (lbc *LoadBalancerController) sync(key string) (err error) {
 		if ing.Annotations == nil {
 			ing.Annotations = map[string]string{}
 		}
-		err = setInstanceGroupsAnnotation(ing.Annotations, igs)
+		if err = setInstanceGroupsAnnotation(ing.Annotations, igs); err != nil {
-		if err != nil {
 			return err
 		}
-		if err := lbc.updateAnnotations(ing.Name, ing.Namespace, ing.Annotations); err != nil {
+		if err = lbc.updateAnnotations(ing.Name, ing.Namespace, ing.Annotations); err != nil {
 			return err
 		}
 		return nil

controllers/gce/controller/fakes.go

@@ -65,7 +65,7 @@ func NewFakeClusterManager(clusterName, firewallName string) *fakeClusterManager
 		testDefaultBeNodePort,
 		namer,
 	)
-	frPool := firewalls.NewFirewallPool(firewalls.NewFakeFirewallsProvider(), namer)
+	frPool := firewalls.NewFirewallPool(firewalls.NewFakeFirewallsProvider(false, false), namer)
 	cm := &ClusterManager{
 		ClusterNamer: namer,
 		instancePool: nodePool,

controllers/gce/firewalls/fakes.go

@@ -25,14 +25,21 @@ import (
 )
 
 type fakeFirewallsProvider struct {
-	fw         map[string]*compute.Firewall
+	fw               map[string]*compute.Firewall
-	networkUrl string
+	networkProjectID string
+	networkURL       string
+	onXPN            bool
+	fwReadOnly       bool
 }
 
 // NewFakeFirewallsProvider creates a fake for firewall rules.
-func NewFakeFirewallsProvider() *fakeFirewallsProvider {
+func NewFakeFirewallsProvider(onXPN bool, fwReadOnly bool) *fakeFirewallsProvider {
 	return &fakeFirewallsProvider{
-		fw: make(map[string]*compute.Firewall),
+		fw:               make(map[string]*compute.Firewall),
+		networkProjectID: "test-network-project",
+		networkURL:       "/path/to/my-network",
+		onXPN:            onXPN,
+		fwReadOnly:       fwReadOnly,
 	}
 }
 
@@ -44,7 +51,7 @@ func (ff *fakeFirewallsProvider) GetFirewall(name string) (*compute.Firewall, er
 	return nil, utils.FakeGoogleAPINotFoundErr()
 }
 
-func (ff *fakeFirewallsProvider) CreateFirewall(f *compute.Firewall) error {
+func (ff *fakeFirewallsProvider) doCreateFirewall(f *compute.Firewall) error {
 	if _, exists := ff.fw[f.Name]; exists {
 		return fmt.Errorf("firewall rule %v already exists", f.Name)
 	}
@@ -52,7 +59,15 @@ func (ff *fakeFirewallsProvider) CreateFirewall(f *compute.Firewall) error {
 	return nil
 }
 
-func (ff *fakeFirewallsProvider) DeleteFirewall(name string) error {
+func (ff *fakeFirewallsProvider) CreateFirewall(f *compute.Firewall) error {
+	if ff.fwReadOnly {
+		return utils.FakeGoogleAPIForbiddenErr()
+	}
+
+	return ff.doCreateFirewall(f)
+}
+
+func (ff *fakeFirewallsProvider) doDeleteFirewall(name string) error {
 	// We need the full name for the same reason as CreateFirewall.
 	_, exists := ff.fw[name]
 	if !exists {
@@ -63,7 +78,15 @@ func (ff *fakeFirewallsProvider) DeleteFirewall(name string) error {
 	return nil
 }
 
-func (ff *fakeFirewallsProvider) UpdateFirewall(f *compute.Firewall) error {
+func (ff *fakeFirewallsProvider) DeleteFirewall(name string) error {
+	if ff.fwReadOnly {
+		return utils.FakeGoogleAPIForbiddenErr()
+	}
+
+	return ff.doDeleteFirewall(name)
+}
+
+func (ff *fakeFirewallsProvider) doUpdateFirewall(f *compute.Firewall) error {
 	// We need the full name for the same reason as CreateFirewall.
 	_, exists := ff.fw[f.Name]
 	if !exists {
@@ -74,8 +97,24 @@ func (ff *fakeFirewallsProvider) UpdateFirewall(f *compute.Firewall) error {
 	return nil
 }
 
+func (ff *fakeFirewallsProvider) UpdateFirewall(f *compute.Firewall) error {
+	if ff.fwReadOnly {
+		return utils.FakeGoogleAPIForbiddenErr()
+	}
+
+	return ff.doUpdateFirewall(f)
+}
+
+func (ff *fakeFirewallsProvider) NetworkProjectID() string {
+	return ff.networkProjectID
+}
+
 func (ff *fakeFirewallsProvider) NetworkURL() string {
-	return ff.networkUrl
+	return ff.networkURL
+}
+
+func (ff *fakeFirewallsProvider) OnXPN() bool {
+	return ff.onXPN
 }
 
 func (ff *fakeFirewallsProvider) GetNodeTags(nodeNames []string) ([]string, error) {

controllers/gce/firewalls/firewalls.go

@@ -17,12 +17,15 @@ limitations under the License.
 package firewalls
 
 import (
+	"fmt"
+	"sort"
 	"strconv"
 
 	"github.com/golang/glog"
 
 	compute "google.golang.org/api/compute/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
 	netset "k8s.io/kubernetes/pkg/util/net/sets"
 
 	"k8s.io/ingress/controllers/gce/utils"
@@ -68,7 +71,7 @@ func (fr *FirewallRules) Sync(nodePorts []int64, nodeNames []string) error {
 
 	if rule == nil {
 		glog.Infof("Creating global l7 firewall rule %v", name)
-		return fr.cloud.CreateFirewall(firewall)
+		return fr.createFirewall(firewall)
 	}
 
 	requiredPorts := sets.NewString()
@@ -92,19 +95,14 @@ func (fr *FirewallRules) Sync(nodePorts []int64, nodeNames []string) error {
 		return nil
 	}
 	glog.V(3).Infof("Firewall %v already exists, updating nodeports %v", name, nodePorts)
-	return fr.cloud.UpdateFirewall(firewall)
+	return fr.updateFirewall(firewall)
 }
 
 // Shutdown shuts down this firewall rules manager.
 func (fr *FirewallRules) Shutdown() error {
 	name := fr.namer.FrName(fr.namer.FrSuffix())
 	glog.Infof("Deleting firewall %v", name)
-	err := fr.cloud.DeleteFirewall(name)
+	return fr.deleteFirewall(name)
-	if err != nil && utils.IsHTTPErrorCode(err, 404) {
-		glog.Infof("Firewall with name %v didn't exist at Shutdown", name)
-		return nil
-	}
-	return err
 }
 
 // GetFirewall just returns the firewall object corresponding to the given name.
@@ -119,6 +117,8 @@ func (fr *FirewallRules) createFirewallObject(firewallName, description string,
 	for ix := range nodePorts {
 		ports[ix] = strconv.Itoa(int(nodePorts[ix]))
 	}
+	// Sorting the ports will prevent duplicate events being created despite having identical params.
+	sort.Strings(ports)
 
 	// If the node tags to be used for this cluster have been predefined in the
 	// provider config, just use them. Otherwise, invoke computeHostTags method to get the tags.
@@ -126,6 +126,7 @@ func (fr *FirewallRules) createFirewallObject(firewallName, description string,
 	if err != nil {
 		return nil, err
 	}
+	sort.Strings(targetTags)
 
 	return &compute.Firewall{
 		Name:         firewallName,
@@ -141,3 +142,52 @@ func (fr *FirewallRules) createFirewallObject(firewallName, description string,
 		TargetTags: targetTags,
 	}, nil
 }
+
+func (fr *FirewallRules) createFirewall(f *compute.Firewall) error {
+	err := fr.cloud.CreateFirewall(f)
+	if utils.IsForbiddenError(err) && fr.cloud.OnXPN() {
+		gcloudCmd := gce.FirewallToGCloudCreateCmd(f, fr.cloud.NetworkProjectID())
+		glog.V(3).Infof("Could not create L7 firewall on XPN cluster. Raising event for cmd: %q", gcloudCmd)
+		return newFirewallXPNError(err, gcloudCmd)
+	}
+	return err
+}
+
+func (fr *FirewallRules) updateFirewall(f *compute.Firewall) error {
+	err := fr.cloud.UpdateFirewall(f)
+	if utils.IsForbiddenError(err) && fr.cloud.OnXPN() {
+		gcloudCmd := gce.FirewallToGCloudUpdateCmd(f, fr.cloud.NetworkProjectID())
+		glog.V(3).Infof("Could not update L7 firewall on XPN cluster. Raising event for cmd: %q", gcloudCmd)
+		return newFirewallXPNError(err, gcloudCmd)
+	}
+	return err
+}
+
+func (fr *FirewallRules) deleteFirewall(name string) error {
+	err := fr.cloud.DeleteFirewall(name)
+	if utils.IsNotFoundError(err) {
+		glog.Infof("Firewall with name %v didn't exist when attempting delete.", name)
+		return nil
+	} else if utils.IsForbiddenError(err) && fr.cloud.OnXPN() {
+		gcloudCmd := gce.FirewallToGCloudDeleteCmd(name, fr.cloud.NetworkProjectID())
+		glog.V(3).Infof("Could not attempt delete of L7 firewall on XPN cluster. %q needs to be ran.", gcloudCmd)
+		return newFirewallXPNError(err, gcloudCmd)
+	}
+	return err
+}
+
+func newFirewallXPNError(internal error, cmd string) *FirewallSyncError {
+	return &FirewallSyncError{
+		Internal: internal,
+		Message:  fmt.Sprintf("Firewall change required by network admin: `%v`", cmd),
+	}
+}
+
+type FirewallSyncError struct {
+	Internal error
+	Message  string
+}
+
+func (f *FirewallSyncError) Error() string {
+	return f.Message
+}

controllers/gce/firewalls/firewalls_test.go

@@ -18,6 +18,7 @@ package firewalls
 
 import (
 	"strconv"
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -26,7 +27,7 @@ import (
 
 func TestSyncFirewallPool(t *testing.T) {
 	namer := utils.NewNamer("ABC", "XYZ")
-	fwp := NewFakeFirewallsProvider()
+	fwp := NewFakeFirewallsProvider(false, false)
 	fp := NewFirewallPool(fwp, namer)
 	ruleName := namer.FrName(namer.FrSuffix())
 
@@ -87,6 +88,66 @@ func TestSyncFirewallPool(t *testing.T) {
 	}
 }
 
+// TestSyncOnXPNWithPermission tests that firwall sync continues to work when OnXPN=true
+func TestSyncOnXPNWithPermission(t *testing.T) {
+	namer := utils.NewNamer("ABC", "XYZ")
+	fwp := NewFakeFirewallsProvider(true, false)
+	fp := NewFirewallPool(fwp, namer)
+	ruleName := namer.FrName(namer.FrSuffix())
+
+	// Test creating a firewall rule via Sync
+	nodePorts := []int64{80, 443, 3000}
+	nodes := []string{"node-a", "node-b", "node-c"}
+	err := fp.Sync(nodePorts, nodes)
+	if err != nil {
+		t.Errorf("unexpected err when syncing firewall, err: %v", err)
+	}
+	verifyFirewallRule(fwp, ruleName, nodePorts, nodes, l7SrcRanges, t)
+}
+
+// TestSyncOnXPNReadOnly tests that controller behavior is accurate when the controller
+// does not have permission to create/update/delete firewall rules.
+// Specific errors should be returned.
+func TestSyncOnXPNReadOnly(t *testing.T) {
+	namer := utils.NewNamer("ABC", "XYZ")
+	fwp := NewFakeFirewallsProvider(true, true)
+	fp := NewFirewallPool(fwp, namer)
+	ruleName := namer.FrName(namer.FrSuffix())
+
+	// Test creating a firewall rule via Sync
+	nodePorts := []int64{80, 443, 3000}
+	nodes := []string{"node-a", "node-b", "node-c"}
+	err := fp.Sync(nodePorts, nodes)
+	if fwErr, ok := err.(*FirewallSyncError); !ok || !strings.Contains(fwErr.Message, "create") {
+		t.Errorf("Expected firewall sync error with a user message. Received err: %v", err)
+	}
+
+	// Manually create the firewall
+	firewall, err := fp.(*FirewallRules).createFirewallObject(ruleName, "", nodePorts, nodes)
+	if err != nil {
+		t.Errorf("unexpected err when creating firewall object, err: %v", err)
+	}
+	err = fwp.doCreateFirewall(firewall)
+	if err != nil {
+		t.Errorf("unexpected err when creating firewall, err: %v", err)
+	}
+
+	// Run sync again with same state - expect no event
+	err = fp.Sync(nodePorts, nodes)
+	if err != nil {
+		t.Errorf("unexpected err when syncing firewall, err: %v", err)
+	}
+
+	// Modify nodePorts to cause an event
+	nodePorts = append(nodePorts, 3001)
+
+	// Run sync again with same state - expect no event
+	err = fp.Sync(nodePorts, nodes)
+	if fwErr, ok := err.(*FirewallSyncError); !ok || !strings.Contains(fwErr.Message, "update") {
+		t.Errorf("Expected firewall sync error with a user message. Received err: %v", err)
+	}
+}
+
 func verifyFirewallRule(fwp *fakeFirewallsProvider, ruleName string, expectedPorts []int64, expectedNodes, expectedCIDRs []string, t *testing.T) {
 	var strPorts []string
 	for _, v := range expectedPorts {

controllers/gce/firewalls/interfaces.go

@@ -36,5 +36,9 @@ type Firewall interface {
 	DeleteFirewall(name string) error
 	UpdateFirewall(f *compute.Firewall) error
 	GetNodeTags(nodeNames []string) ([]string, error)
+	NetworkProjectID() string
 	NetworkURL() string
+
+	// OnXPN returns true if the GCE NetworkProjectID != ProjectID.
+	OnXPN() bool
 }

controllers/gce/utils/utils.go

@@ -309,9 +309,14 @@ func (g GCEURLMap) PutDefaultBackend(d *compute.BackendService) {
 	}
 }
 
+// FakeGoogleAPIForbiddenErr creates a Forbidden error with type googleapi.Error
+func FakeGoogleAPIForbiddenErr() *googleapi.Error {
+	return &googleapi.Error{Code: http.StatusForbidden}
+}
+
 // FakeGoogleAPINotFoundErr creates a NotFound error with type googleapi.Error
 func FakeGoogleAPINotFoundErr() *googleapi.Error {
-	return &googleapi.Error{Code: 404}
+	return &googleapi.Error{Code: http.StatusNotFound}
 }
 
 // IsHTTPErrorCode checks if the given error matches the given HTTP Error code.
@@ -344,6 +349,11 @@ func IsNotFoundError(err error) bool {
 	return IsHTTPErrorCode(err, http.StatusNotFound)
 }
 
+// IsForbiddenError returns true if the operation was forbidden
+func IsForbiddenError(err error) bool {
+	return IsHTTPErrorCode(err, http.StatusForbidden)
+}
+
 // CompareLinks returns true if the 2 self links are equal.
 func CompareLinks(l1, l2 string) bool {
 	// TODO: These can be partial links