OCPBUGS-57447,OCPBUGS-45056: Refrain from adding Egress IP to public LB backend pool #180
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@arghosh93: This pull request references Jira Issue OCPBUGS-57447, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@arghosh93: This pull request references Jira Issue OCPBUGS-57447, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 openshift-eng/jira-lifecycle-plugin repository. |
arghosh93
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@arghosh93: This pull request references Jira Issue OCPBUGS-57447, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-45056, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@arghosh93: This pull request references Jira Issue OCPBUGS-57447, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: This pull request references Jira Issue OCPBUGS-45056, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
arkadeepsen
left a comment
arkadeepsen
left a comment
There was a problem hiding this comment.
Do we have any tests which can be used to verify the changes made in this PR will correctly solve the issue?
pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go
Outdated
Show resolved
Hide resolved
We lack knowledge of API of different cloud providers to fake it. That is the main reason behind not having enough unit tests. |
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
c2b5065 to
6dca85c
Compare
|
@arghosh93 Does this PR introduce any limitations on pod egress traffic? From my understanding, if we skip adding the EgressIP to the load balancer backend pools, the egress traffic will be restricted to the infra subnet. Is that correct? |
Yes @pperiyasamy , that is correct. The plan is to document this limitation along with a suggestion of using NAT gateway instead of a general purpose public load balancer. I am also gonna notify support team members so that everyone is well aware. |
Thanks @arghosh93 , If this is agreed by everyone, i'm fine with it. one comment on the sync function. |
pkg/cloudprovider/azure.go
Outdated
| // backend pool regardless of the presence of an OutBoundRule. | ||
| // During upgrade this function removes any egress IP added to | ||
| // public load balancer backend pool previously. | ||
| func (a *Azure) SyncLBBackend(ip net.IP, node *corev1.Node) error { |
There was a problem hiding this comment.
have you tested this SyncLBBackend function with so many EIPs already in place ? because AFAIK Azure APIs are so slow and not sure how it works when you want to sync already existing IPs.
have you explored sync IPs belong to a node with single API call ? something similar to processing existing items (like this) before watching CloudPrivateIPConfig objects.
There was a problem hiding this comment.
I have tested with around 10 egress IPs. I have not seen much delay. We can ask QE to test with more egress IPs. There is a slack thread where ARO team did some testing with this PR.
https://redhat-internal.slack.com/archives/C09G14XDR9B/p1759942816358369?thread_ts=1759848001.402299&cid=C09G14XDR9B
Egress IPs are queued separately and may be difficult to obtain all at once. This is also a one time thing and expected to be run mostly during the upgrade.
I do not anticipate it taking much time and going beyond the upgrade completion time.
|
/retest-required |
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
6dca85c to
57b79e5
Compare
openshift-merge-robot
added
the
needs-rebase
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
57b79e5 to
02854f1
Compare
openshift-merge-robot
added
the
needs-rebase
This PR is to stop adding Egress IP to public load balancer backend pool regardless of presence of an OutBoundRule in any Azure cluster. This change comes with a consequence of no outbound connectivity except to the infrastructure subnet even if there is no OutBoundRule. However this is required to tackle following situation: - If an infra node is being used as an egressNode then health check for egress IP also succeeds when it is added to public load balancer and LB considers it as a legitimate ingress router backend. - Limits the number of egress IP which can be created on a cluster due to some Azure specific limitation. Signed-off-by: Arnab Ghosh <[email protected]>
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
3a2f01f to
cec2534
Compare
openshift-merge-robot
removed
the
needs-rebase
arghosh93
force-pushed
the
OCPBUGS-57447
branch
2 times, most recently
from
1600d22 to
bf35222
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go (1)
184-190: Treat missing nodes as a successful no-op.If the assigned node has already been deleted,
nodesLister.GetreturnsIsNotFound. We now bubble that up as an error, so this branch requeues forever for stale CloudPrivateIPConfigs during rollouts. Mirror the delete path: swallowIsNotFound(and a nil node) and skipSyncLBBackend, since there’s nothing left to clean. Only return real errors.- node, err := c.nodesLister.Get(cloudPrivateIPConfig.Spec.Node) - if err != nil { - return err - } + node, err := c.nodesLister.Get(cloudPrivateIPConfig.Spec.Node) + if apierrors.IsNotFound(err) || node == nil { + return nil + } + if err != nil { + return err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
pkg/cloudprovider/aws.go(1 hunks)pkg/cloudprovider/azure.go(10 hunks)pkg/cloudprovider/cloudprovider.go(2 hunks)pkg/cloudprovider/cloudprovider_fake.go(1 hunks)pkg/cloudprovider/gcp.go(1 hunks)pkg/cloudprovider/openstack.go(1 hunks)pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudprovider/gcp.go
- pkg/cloudprovider/aws.go
- pkg/cloudprovider/cloudprovider.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cloudprovider/cloudprovider_fake.gopkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.gopkg/cloudprovider/openstack.gopkg/cloudprovider/azure.go
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
bf35222 to
335bd7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/cloudprovider/openstack.go (1)
548-551: Fix typo in comment.The comment contains a typo: "dont" should be "don't".
Apply this diff:
- // We dont add Egress IP to OpenStack public LB backend; nothing to do + // We don't add Egress IP to OpenStack public LB backend; nothing to do
🧹 Nitpick comments (1)
pkg/cloudprovider/aws.go (1)
225-228: Fix typo in comment.The comment contains a minor typo: "dont" should be "don't".
Apply this diff:
- // We dont add Egress IP to AWS public LB backend; nothing to do + // We don't add Egress IP to AWS public LB backend; nothing to do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
pkg/cloudprovider/aws.go(1 hunks)pkg/cloudprovider/azure.go(10 hunks)pkg/cloudprovider/cloudprovider.go(2 hunks)pkg/cloudprovider/cloudprovider_fake.go(1 hunks)pkg/cloudprovider/gcp.go(1 hunks)pkg/cloudprovider/openstack.go(1 hunks)pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudprovider/cloudprovider.go
- pkg/cloudprovider/cloudprovider_fake.go
- pkg/cloudprovider/gcp.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cloudprovider/openstack.gopkg/cloudprovider/azure.gopkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.gopkg/cloudprovider/aws.go
pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go
Outdated
Show resolved
Hide resolved
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
335bd7a to
626819b
Compare
|
/retest-required |
|
/payload 4.21 ci blocking |
|
@pperiyasamy: trigger 5 job(s) of type blocking for the ci release of OCP 4.21
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/30b57f60-bee3-11f0-9164-d0c4d8ab18a3-0 trigger 13 job(s) of type blocking for the nightly release of OCP 4.21
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/30b57f60-bee3-11f0-9164-d0c4d8ab18a3-1 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arghosh93, arkadeepsen, pperiyasamy 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 |
|
/verified by pre-merge testing |
openshift-ci-robot
added
the
verified
|
@yingwang-0320: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
kyrtapz
left a comment
kyrtapz
left a comment
There was a problem hiding this comment.
I have some suggestions but I really want to make sure that we have a solid doc plan for this change, what is the version you plan to backport this to?
pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go
Outdated
Show resolved
Hide resolved
pkg/cloudprovider/cloudprovider.go
Outdated
|
|
||
| // SyncLBBackend removes any egress IP which is already added to backend pool of | ||
| // a public load balancer. This is mostly Azure specific and may be removed later. | ||
| SyncLBBackend(ip net.IP, node *corev1.Node) error |
There was a problem hiding this comment.
This is a very azure specific function...
And there is no point in running it for newly created EIPs that just got assigned right?
In my view this should be something that runs once, during startup, wdyt?
There was a problem hiding this comment.
To stop it running for newly created IPs, I have introduced lbBackendPoolSynced bool in Azure struct and setting it after running AssignPrivateIP. At the begining of SyncLBBackend function I am also checking if lbBackendPoolSynced is set or not. If set then I am returning instantly.
I agree that this is a Azure specific function but I thought it is unusual to run at main function.
There was a problem hiding this comment.
I wasn't talking about the main function. We already have a initCredentials interface method that is being ran from NewCloudProviderClient. We can add another method that would run once after the provided was configured and we could include that logic in there.
There was a problem hiding this comment.
I thought about invoking a method from initCredentials so that we run SyncLBBackend once and only for Azure. But, Azure struct does not have cloudPrivateIPConfigLister. So, do you suggest to include cloudPrivateIPConfigLister to Azure struct and invoke a method from initCredentials which should list all EIPs and do changes for LB backend pool only once?
As per current implementation I do agree that there is chance of invoking SyncLBBackend multiple times for a single EIP but in Azure struct there is a map lbBackendPoolSynced which tracks if we have already synced backend pool for any egress IP or not and if it is already done then we return immediately from SyncLBBackend. So, I assume low impact.
There was a problem hiding this comment.
It's fine to pass the lister if it is required. I am even ok to put it in the initCredentials but if you end up doing that please rename it in a separate commit.
| if err != nil { | ||
| return err | ||
| } | ||
| if err := c.cloudProviderClient.SyncLBBackend(ip, node); err != nil { |
There was a problem hiding this comment.
Why isn't this called from the node controller instead?
There was a problem hiding this comment.
We need cloudProviderClient to run this and that is available with CloudPrivateIPConfigController. Can you please explain if I misunderstood your point?
There was a problem hiding this comment.
We are already using cloudProviderClient in the node controller:
but let's not run the sync on every update as per my previous comment.
| ipConfigurations = append(ipConfigurations, newIPConfiguration) | ||
| networkInterface.Properties.IPConfigurations = ipConfigurations | ||
| // Send the request | ||
| klog.Warningf("Egress IP %s will have no outbound connectivity except for the infrastructure subnet: "+ |
There was a problem hiding this comment.
As we have discussed in the past this has to have a proper docs in OCP. Is there a card created for it? Are you going to ensure this will be backported to all releases that this change will affect?
This also means that we are a going to significantly change behavior in a Z stream release for the backports so a release note is certainly required too.
There was a problem hiding this comment.
This issue can be seen from Openshift 4.18 , where the egress IP gets added to the interface of the assigned node. So, the current plan is to backport till 4.18.
I have asked @jab-rh to assign a doc contact for this Jira. As discussed earlier, I have plan to send an email to the support team so that they are aware.
I have set 'Release note type' as 'Removed functionality' in the jira ticket.
There was a problem hiding this comment.
Thanks, I think that this might be enough.
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
626819b to
6730d47
Compare
openshift-ci-robot
removed
the
verified
| case nodeNameToAdd == "" && nodeNameToDel == "": | ||
| node, err := c.nodesLister.Get(cloudPrivateIPConfig.Spec.Node) | ||
| if err != nil && apierrors.IsNotFound(err) { | ||
| klog.Infof("Source node: %s no longer exists for CloudPrivateIPConfig: %q", cloudPrivateIPConfig.Spec.Node, key) |
There was a problem hiding this comment.
You still have to exit here? node is empty here
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
6730d47 to
ef0e716
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudprovider/cloudprovider.go (1)
141-175: Validate Azure informers are non-nil to avoid nil deref/panic.
cloudPrivateIPConfigInformer.Lister()/nodeInformer.Lister()will panic if callers pass nil (even accidentally) when PlatformType is Azure.func NewCloudProviderClient(cfg CloudProviderConfig, platformStatus *configv1.PlatformStatus, featureGates featuregates.FeatureGate, cloudPrivateIPConfigInformer cloudnetworkinformers.CloudPrivateIPConfigInformer, nodeInformer coreinformers.NodeInformer) (CloudProviderIntf, error) { var cloudProviderIntf CloudProviderIntf @@ switch cfg.PlatformType { case PlatformTypeAzure: + if cloudPrivateIPConfigInformer == nil || nodeInformer == nil { + return nil, fmt.Errorf("azure requires CloudPrivateIPConfigInformer and NodeInformer") + } var azurePlatformStatus *configv1.AzurePlatformStatus if platformStatus != nil && platformStatus.Type == PlatformTypeAzure { azurePlatformStatus = platformStatus.Azure } cloudProviderIntf = &Azure{ @@ lbBackendPoolSynced: make(map[string]bool), cloudPrivateIPConfigLister: cloudPrivateIPConfigInformer.Lister(), nodesLister: nodeInformer.Lister(), }
🧹 Nitpick comments (1)
pkg/cloudprovider/openstack.go (1)
70-76:OpenStack.init()wrapper looks correct (minor simplification possible).
This aligns with the newCloudProviderIntf.init()flow.Optional: the trailing
return nilis redundant.func (o *OpenStack) init() error { if err := o.initCredentials(); err != nil { return err } - - return nil + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
cmd/cloud-network-config-controller/main.go(1 hunks)pkg/cloudprovider/aws.go(1 hunks)pkg/cloudprovider/azure.go(10 hunks)pkg/cloudprovider/cloudprovider.go(5 hunks)pkg/cloudprovider/cloudprovider_fake.go(1 hunks)pkg/cloudprovider/gcp.go(1 hunks)pkg/cloudprovider/openstack.go(1 hunks)pkg/controller/node/node_controller.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/controller/node/node_controller.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cloudprovider/gcp.go
- pkg/cloudprovider/cloudprovider_fake.go
- pkg/cloudprovider/aws.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cloudprovider/openstack.gopkg/cloudprovider/cloudprovider.gocmd/cloud-network-config-controller/main.gopkg/cloudprovider/azure.go
🧬 Code graph analysis (2)
cmd/cloud-network-config-controller/main.go (1)
pkg/cloudprovider/cloudprovider.go (1)
NewCloudProviderClient(141-193)
pkg/cloudprovider/azure.go (1)
pkg/cloudprivateipconfig/cloudprivateipconfig.go (1)
NameToIP(28-39)
🔇 Additional comments (1)
pkg/cloudprovider/cloudprovider.go (1)
141-193: Call site has been properly updated. The onlyNewCloudProviderClient()call in the codebase (cmd/cloud-network-config-controller/main.go:145-149) correctly passes all five required parameters matching the updated signature. No compilation issues remain.
The consensus is to not add egress IP to public load balancer backend pool regardless of the presence of an OutBoundRule. During upgrade this PR let cobtroller removes any egress IP added to public load balancer backend pool previously. Signed-off-by: Arnab Ghosh <[email protected]>
arghosh93
force-pushed
the
OCPBUGS-57447
branch
from
ef0e716 to
dac9638
Compare
|
@arghosh93: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
7 participants
This PR is to stop adding Egress IP to public load balancer
backend pool regardless of presence of an OutBoundRule in any
Azure cluster.
This change comes with a consequence of no outbound connectivity
except to the infrastructure subnet even if there is no OutBoundRule.
However this is required to tackle following situation:
this PR also let cobtroller remove any egress IP
added to public load balancer backend pool previously.