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
Firewall Pinhole Fix for ILB and NetLB #109510
Conversation
Welcome @sugangli! |
Hi @sugangli. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
Thank you for reviewing this PR! I added some background in the description. Please let me know if I need to provide further detail. |
/approve This is a bug fix. |
/approve Who's doing the code-review on this? I can approve but it would be best to get someone who knows this code well to do a thorough review. It all looks OK to me but I didn't go super deep on it. |
@sugangli -- can we sync for 15 minutes to understand why certain sections of logic was moved around in the function, it will help me review this quickly. |
/lgtm |
@kl52752: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@@ -27,7 +27,7 @@ import ( | |||
"strings" | |||
|
|||
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" | |||
"k8s.io/api/core/v1" | |||
v1 "k8s.io/api/core/v1" |
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.
this is redundant..
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 think this was added by vs code automatically, which I am not 100% sure why. But I see a earlier PR has this too:https://github.com/kubernetes/kubernetes/blame/master/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go#L34
@@ -123,12 +123,6 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, | |||
// keeping the static IP around even though this is more complicated because | |||
// it makes it less likely that we'll run into quota issues. Only 7 static | |||
// IP addresses are allowed per region by default. | |||
// |
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.
Why was this comment removed?
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.
Because we moved the firewall configuration to the last.
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.
Discussed offline and we decided to move this back so we don't accidentally break implicit logic around this.
@@ -286,6 +256,30 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, | |||
status := &v1.LoadBalancerStatus{} |
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 think we should keep this right before the return
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.
done.
@@ -921,7 +915,7 @@ func (g *Cloud) ensureHTTPHealthCheckFirewall(svc *v1.Service, serviceName, ipAd | |||
return fmt.Errorf("error getting firewall for health checks: %v", err) | |||
} | |||
klog.Infof("Creating firewall %v for health checks.", fwName) | |||
if err := g.createFirewall(svc, fwName, desc, sourceRanges, ports, hosts); err != nil { | |||
if err := g.createFirewall(svc, fwName, desc, "", sourceRanges, ports, hosts); 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.
g.createFirewall(..., /*destinationIP*/ "", ...
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 HC, we don't have to set destination IP as the sourceRanges are known CIDRs. I set it anyway as it doesn't hurt.
@@ -934,7 +928,7 @@ func (g *Cloud) ensureHTTPHealthCheckFirewall(svc *v1.Service, serviceName, ipAd | |||
!equalStringSets(fw.Allowed[0].Ports, []string{strconv.Itoa(int(ports[0].Port))}) || | |||
!equalStringSets(fw.SourceRanges, sourceRanges.StringSlice()) { | |||
klog.Warningf("Firewall %v exists but parameters have drifted - updating...", fwName) | |||
if err := g.updateFirewall(svc, fwName, desc, sourceRanges, ports, hosts); err != nil { | |||
if err := g.updateFirewall(svc, fwName, desc, "", sourceRanges, ports, hosts); 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.
/*destinationIP*/ ...
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.
See above.
@@ -1007,7 +1001,7 @@ func (g *Cloud) updateFirewall(svc *v1.Service, name, desc string, sourceRanges | |||
return nil | |||
} | |||
|
|||
func (g *Cloud) firewallObject(name, desc string, sourceRanges utilnet.IPNetSet, ports []v1.ServicePort, hosts []*gceInstance) (*compute.Firewall, error) { | |||
func (g *Cloud) firewallObject(name, desc, destinationIP string, sourceRanges utilnet.IPNetSet, ports []v1.ServicePort, hosts []*gceInstance) (*compute.Firewall, error) { |
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.
We should document that destinationIP can be empty string "" and this means that it is not set.
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.
Added.
@@ -416,7 +421,7 @@ func (g *Cloud) teardownInternalHealthCheckAndFirewall(svc *v1.Service, hcName s | |||
return nil | |||
} | |||
|
|||
func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc string, sourceRanges []string, portRanges []string, protocol v1.Protocol, nodes []*v1.Node, legacyFwName string) error { | |||
func (g *Cloud) ensureInternalFirewall(svc *v1.Service, fwName, fwDesc, DestinationIP string, sourceRanges []string, portRanges []string, protocol v1.Protocol, nodes []*v1.Node, legacyFwName string) error { |
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.
don't capitalize DestinationIP
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.
Done.
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.
Walked through the changes with Sugang offline on this and overall LGTM.
@@ -123,12 +123,6 @@ func (g *Cloud) ensureExternalLoadBalancer(clusterName string, clusterID string, | |||
// keeping the static IP around even though this is more complicated because | |||
// it makes it less likely that we'll run into quota issues. Only 7 static | |||
// IP addresses are allowed per region by default. | |||
// |
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.
Discussed offline and we decided to move this back so we don't accidentally break implicit logic around this.
@@ -225,6 +220,16 @@ func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v | |||
} | |||
} | |||
|
|||
fwd, err := g.GetRegionForwardingRule(loadBalancerName, g.region) |
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.
Pasting comment from offline - merge the second GetRegionForwardingRule
down below into this so we don't generate additional get call.
@sugangli: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
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.
Thanks! Could you update the release note since this changes the firewall behavior for new/existing L4 LBs?
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrHohn, nckturner, sugangli, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind bug
Adding new gce firewall field DestinationRanges to ILB and NetLB. This change does not change the existing behaviors while creating these LBs. It simply restricts the access from Node & All IPs to Node & DestinationRanges.
Two major changes:
Before the introduction of GCE pinhole firewall feature, the destination-ranges of the firewall rule were default to all, i.e., any source allowed in the source-ranges was allowed to access any IP in the backend. This introduced a security risk, for example, any netlb service opens port for 22 would expose ssh access to the node in addition to other security issues. The fix here is aimed at restricting the access to the backend IPs via allowing traffic via ILB or NetLB IP only.