[GCLB] Handle case of not having firewall create/update/delete with XPN #1462
Conversation
|
This change is |
k8s-ci-robot
added
size/L
nicksardo
force-pushed
the
graceful-xpn-firewall
branch
from
4e14c68
to
90c6982
Compare
permissions on XPN cluster
nicksardo
force-pushed
the
graceful-xpn-firewall
branch
from
90c6982
to
2025675
Compare
| syncError = fmt.Errorf("%v, update ingress error: %v", syncError, err) | ||
| } | ||
| return syncError | ||
| } | ||
|
|
||
| // updateIngressStatus updates the IP and annotations of a loadbalancer. | ||
| // The annotations are parsed by kubectl describe. | ||
| func (lbc *LoadBalancerController) updateIngressStatus(l7 *loadbalancers.L7, ing extensions.Ingress) error { | ||
| func (lbc *LoadBalancerController) updateIngressStatus(l7 *loadbalancers.L7, ing *extensions.Ingress) error { | ||
| ingClient := lbc.client.Extensions().Ingresses(ing.Namespace) |
There was a problem hiding this comment.
With this change ing can be nil now. Do you want to handle that case?
| NetworkURL() string | ||
| OnXPN() bool |
There was a problem hiding this comment.
Can you add a comment here.
Is this expected to return true if XPN is on in the project?
| @@ -159,7 +162,7 @@ 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 { | |||
| if err := c.firewallPool.Sync(np, nodeNames, currentIngress); err != nil { | |||
There was a problem hiding this comment.
I am not sure if it will work but want to throw an idea: How about letting firewallPool.Sync return an error and cluster manager can then decide to generate an event for it.
That way wont need to pass currentIngress and eventrecorder down to FirewallPool. In future, the same logic can be extended by other libraries like healthcheck and forwarding rules to generate events as well? Better than passing extra objects everywhere
| cluster.firewallPool = firewalls.NewFirewallPool(cloud, cluster.ClusterNamer) | ||
|
|
||
| eventRaiser := func(ing *extensions.Ingress, reason, msg string) { | ||
| recorder.Event(ing, apiv1.EventTypeNormal, reason, msg) |
There was a problem hiding this comment.
What happens when ing is nil? No event?
| @@ -53,7 +53,8 @@ func defaultBackendName(clusterName string) string { | |||
| // newLoadBalancerController create a loadbalancer controller. | |||
| func newLoadBalancerController(t *testing.T, cm *fakeClusterManager) *LoadBalancerController { | |||
| kubeClient := fake.NewSimpleClientset() | |||
| ctx := NewControllerContext(kubeClient, api_v1.NamespaceAll, 1*time.Second) | |||
| eventRecorder := utils.NewEventRecorder(kubeClient) | |||
| ctx := NewControllerContext(kubeClient, api_v1.NamespaceAll, 1*time.Second, eventRecorder) | |||
There was a problem hiding this comment.
For my knowledge, why the change to pass eventRecorder in context rather than in NewController? Why is kubeclient passed in both?
| Namespace: api.NamespaceNone, | ||
| UID: id, |
There was a problem hiding this comment.
Is this change really required? Why do we need to set the UID now?
| err := fr.cloud.DeleteFirewall(name) | ||
| if utils.IsForbiddenError(err) && fr.cloud.OnXPN() { | ||
| gcloudCmd := gce.FirewallToGCloudDeleteCmd(name, fr.cloud.NetworkProjectID()) | ||
| glog.V(3).Infof("Could not delete L7 firewall on XPN cluster. %q needs to be ran.", gcloudCmd) |
There was a problem hiding this comment.
Make it consistent with the log message for create and update
| if utils.IsForbiddenError(err) && fr.cloud.OnXPN() { | ||
| gcloudCmd := gce.FirewallToGCloudDeleteCmd(name, fr.cloud.NetworkProjectID()) | ||
| glog.V(3).Infof("Could not delete L7 firewall on XPN cluster. %q needs to be ran.", gcloudCmd) | ||
| // An event cannot be raised because there's no ingress for reference. |
There was a problem hiding this comment.
Another reason why we should not delete ingress resource from apiserver till all cloud resources are deleted: kubernetes/kubernetes#32157
| // Modify nodePorts to cause an event | ||
| nodePorts = append(nodePorts, 3001) | ||
|
|
||
| // Run sync again with same state - expect no event |
There was a problem hiding this comment.
expect no event -> expect event
|
Looks good with mostly questions about the code and few minor suggestions. |
If the controller does not have the ability of managing the firewall rule on XPN, the controller will instead raise an event. The event message prints out the resulting gcloud command.
Since this bug (#950) is still relevant, events raised here will apply to all ingresses that have synced. For example, suppose you have two ingress objects. Ingress-A references nodeport-A and ingress-B references nodeport-B. Assuming a firewall rule exists with only nodeport-A, an event will be raised on both ingresses (after syncing for each ingress).