Fix firewalls for L4 ILBs. #2163
Conversation
|
Hi @mmamczur. 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 |
There was an omission in the code for multinet and some firewall rules were created in the incorrect VPC.
80fa0cb to
06386ad
Compare
| @@ -266,6 +266,7 @@ func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress st | |||
| } | |||
|
|
|||
| // Add firewall rule for L4 External LoadBalancer traffic to nodes | |||
| defaultNetwork := network.DefaultNetwork(l4netlb.cloud) | |||
There was a problem hiding this comment.
I think it should be not default, but also as done in L4, we should store network in l4netlb struct, and pass it here
Otherwise, I guess it will fail, if nodes are not in default network
(same for l4netlbipv6)
|
I'll store this in l4netlb later when I do multinet for netlb.
Default here means cluster network.
This change will mimic the behavior we had before
…On Fri, Jun 9, 2023, 6:53 PM Viacheslav Panasovets ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In pkg/loadbalancers/l4netlb.go
<#2163 (comment)>
:
> @@ -266,6 +266,7 @@ func (l4netlb *L4NetLB) ensureIPv4NodesFirewall(nodeNames []string, ipAddress st
}
// Add firewall rule for L4 External LoadBalancer traffic to nodes
+ defaultNetwork := network.DefaultNetwork(l4netlb.cloud)
I think it should be not default, but also as done in L4, we should store
network in l4netlb struct, and pass it here
Otherwise, I guess it will fail, if nodes are not in default network
(same for l4netlbipv6)
—
Reply to this email directly, view it on GitHub
<#2163 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AVNNTWHAONYXTCIZLPIUW53XKNIHLANCNFSM6AAAAAAY5VEOVE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
oh okay, thought default is "default" network in google cloud thanks for the fix /lgtm but let's track, if that will be safe |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmamczur, panslava 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 |
There was an omission in the code for multinet and some firewall rules were created in the incorrect VPC.