From a061b2ffb4ebff67c239ff13e7f355bb139ec51f Mon Sep 17 00:00:00 2001 From: Nick Sardo Date: Thu, 5 Oct 2017 15:24:28 -0700 Subject: [PATCH] Handle forbiddenError for XPN clusters by raising event --- controllers/gce/controller/cluster_manager.go | 9 +-- controllers/gce/controller/controller.go | 28 +++++--- controllers/gce/controller/fakes.go | 2 +- controllers/gce/firewalls/fakes.go | 55 +++++++++++++--- controllers/gce/firewalls/firewalls.go | 66 ++++++++++++++++--- controllers/gce/firewalls/firewalls_test.go | 63 +++++++++++++++++- controllers/gce/firewalls/interfaces.go | 4 ++ controllers/gce/utils/utils.go | 12 +++- 8 files changed, 207 insertions(+), 32 deletions(-) diff --git a/controllers/gce/controller/cluster_manager.go b/controllers/gce/controller/cluster_manager.go index e987711b9..acd4710b7 100644 --- a/controllers/gce/controller/cluster_manager.go +++ b/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) { diff --git a/controllers/gce/controller/controller.go b/controllers/gce/controller/controller.go index e0a8de1fa..546bceb63 100644 --- a/controllers/gce/controller/controller.go +++ b/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. - eventMsg := "GCE" - if ingExists { - lbc.recorder.Eventf(obj.(*extensions.Ingress), apiv1.EventTypeWarning, eventMsg, err.Error()) + if fwErr, ok := err.(*firewalls.FirewallSyncError); ok { + if ingExists { + lbc.recorder.Eventf(obj.(*extensions.Ingress), apiv1.EventTypeNormal, eventMsg, fwErr.Message) + } 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 != nil { + if err = setInstanceGroupsAnnotation(ing.Annotations, igs); 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 diff --git a/controllers/gce/controller/fakes.go b/controllers/gce/controller/fakes.go index 113460e12..495a6d1f1 100644 --- a/controllers/gce/controller/fakes.go +++ b/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, diff --git a/controllers/gce/firewalls/fakes.go b/controllers/gce/firewalls/fakes.go index 8ee43ad3b..969e74aa9 100644 --- a/controllers/gce/firewalls/fakes.go +++ b/controllers/gce/firewalls/fakes.go @@ -25,14 +25,21 @@ import ( ) type fakeFirewallsProvider struct { - fw map[string]*compute.Firewall - networkUrl string + fw map[string]*compute.Firewall + 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) { diff --git a/controllers/gce/firewalls/firewalls.go b/controllers/gce/firewalls/firewalls.go index a8aceca39..0522e8400 100644 --- a/controllers/gce/firewalls/firewalls.go +++ b/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) - if err != nil && utils.IsHTTPErrorCode(err, 404) { - glog.Infof("Firewall with name %v didn't exist at Shutdown", name) - return nil - } - return err + return fr.deleteFirewall(name) } // 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 +} diff --git a/controllers/gce/firewalls/firewalls_test.go b/controllers/gce/firewalls/firewalls_test.go index 484ad90be..8e2b239d2 100644 --- a/controllers/gce/firewalls/firewalls_test.go +++ b/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 { diff --git a/controllers/gce/firewalls/interfaces.go b/controllers/gce/firewalls/interfaces.go index 97fc6a74d..21f3e2dea 100644 --- a/controllers/gce/firewalls/interfaces.go +++ b/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 } diff --git a/controllers/gce/utils/utils.go b/controllers/gce/utils/utils.go index c0486f557..caccbfa60 100644 --- a/controllers/gce/utils/utils.go +++ b/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