-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GCLB] Handle case of not having firewall create/update/delete with XPN #1462
Conversation
4e14c68
to
90c6982
Compare
Coverage increased (+0.4%) to 43.938% when pulling 90c69820c5efb4c3d310aaef9a06ea3f16d464ba on nicksardo:graceful-xpn-firewall into 923f4d4 on kubernetes:master. |
Coverage increased (+0.4%) to 43.93% when pulling 90c69820c5efb4c3d310aaef9a06ea3f16d464ba on nicksardo:graceful-xpn-firewall into 923f4d4 on kubernetes:master. |
permissions on XPN cluster
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason why we should not delete ingress resource from apiserver till all cloud resources are deleted: kubernetes/kubernetes#32157
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).