fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready #4996

mikejoh · 2025-01-08T14:54:56Z

Description

Due to changes in logic in the controller that operates on VirtualServer objects the VirtualServer external-dns Source needs to determine if the VirtualServer object is ready or not. This PR adds a check to determine if the VirtualServer is ready and if not skips endpoint creation for that particular object.

Checklist

Unit tests updated
End user documentation updated (not needed AFAICT)

Signed-off-by: Mikael Johansson <[email protected]>

k8s-ci-robot · 2025-01-08T14:55:08Z

Hi @mikejoh. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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-sigs/prow repository.

mloiseleur · 2025-01-10T12:52:02Z

/ok-to-test

mloiseleur · 2025-01-10T14:06:04Z

source/f5_virtualserver.go

@@ -147,6 +148,12 @@ func (vs *f5VirtualServerSource) endpointsFromVirtualServers(virtualServers []*f
 	var endpoints []*endpoint.Endpoint

 	for _, virtualServer := range virtualServers {
+		if !isVirtualServerReady(virtualServer) {
+			log.Infof("F5 VirtualServer %s/%s is not ready or is missing an IP address, skipping endpoint creation.",


Suggested change

log.Infof("F5 VirtualServer %s/%s is not ready or is missing an IP address, skipping endpoint creation.",

log.Warnf("F5 VirtualServer %s/%s is not ready or is missing an IP address, skipping endpoint creation.",

Shouldn't it be a warning ?

It could definitely be a warning instead, i'll change it!

Fixed here: d5c7f23

mloiseleur · 2025-01-10T14:08:35Z

This PR looks good, thanks. I have not a strong opinion about the log level, it's just a suggestion.
/lgtm

Signed-off-by: Mikael Johansson <[email protected]>

mloiseleur · 2025-01-10T14:21:10Z

/retitle fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready

ivankatliarchuk · 2025-01-10T14:23:01Z

source/f5_virtualserver.go

@@ -211,3 +219,10 @@ func (vs *f5VirtualServerSource) filterByAnnotations(virtualServers []*f5.Virtua

 	return filteredList, nil
 }
+
+func isVirtualServerReady(vs *f5.VirtualServer) bool {
+	normalizedStatus := strings.ToLower(vs.Status.Status)


potential null pointer as vs is a reference

The null check will help to avoid external-dns crashlooping.

Ideally the null check should be added in method

func (vs *f5VirtualServerSource) endpointsFromVirtualServers ... for _, virtualServer := range virtualServers { if virtualServer == nil { continue }

Good catch but AFAICT the vs pointer here are explicitly set earlier and appended to the slice of vs pointer (before passed to the endpointsFromVirtualServers method), this would effectively mitigate any risk of encountering a nil pointer.

Hi

Looks like missed that piece ;-) If server is empty and created like that, then you right.

virtualServer := &f5.VirtualServer{}

If the virtual server created like the one above, is there is is a chance to throw a null pointer panic in helper function isVirtualServerReady, as vs.Status is not set?

vs.Status.Status vs.Status.VSAddress

Am I correct or missing something?

Thanks for all the review comments, super helpful! 🙏🏻

The status field of the VirtualServer object is defined as a non-pointer: https://github.com/F5Networks/k8s-bigip-ctlr/blob/508f686bf2886a4f95af0f3a822e6ed25bce2ec7/config/apis/cis/v1/types.go#L19. AFAICT this means that in this context, the fields of the VirtualServerStatus struct, will be set to their respective zero values if not explicitly initialized!

ivankatliarchuk · 2025-01-10T14:30:21Z

source/f5_virtualserver.go

+	normalizedStatus := strings.ToLower(vs.Status.Status)
+	normalizedAddress := strings.ToLower(vs.Status.VSAddress)
+
+	return normalizedStatus == "ok" && (normalizedAddress != "none" && normalizedAddress != "")


Just a suggestion to use early return, example

if strings.ToLower(vs.Status.Status) != "ok" { return false } normalizedAddress := strings.ToLower(vs.Status.VSAddress) return normalizedAddress != "none" && normalizedAddress != ""

Good point, changed here: a70d5d0

ivankatliarchuk · 2025-01-10T15:05:34Z

source/f5_virtualserver.go

@@ -147,6 +148,12 @@ func (vs *f5VirtualServerSource) endpointsFromVirtualServers(virtualServers []*f
 	var endpoints []*endpoint.Endpoint

 	for _, virtualServer := range virtualServers {
+		if !isVirtualServerReady(virtualServer) {


just a suggestion

if !virtualServer.isReady()

^ This will crash if virtualServer is null))

Totally understand this but we would need to embed the f5.VirtualServer struct in our own data type and then add a method to that, in this context it feels a bit overkill? A helper func (as used now) that operates on a *f5.VirtualServer feels like a good middle ground.

it was simply a style suggestion, nice one.

k8s-ci-robot · 2025-01-12T13:09:50Z

@ivankatliarchuk: 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-sigs/prow repository.

mikejoh · 2025-01-12T13:19:37Z

@ivankatliarchuk Btw, do you want me to rebase and squash the commits? Not sure what the usual workflow is!

ivankatliarchuk · 2025-01-12T13:35:20Z

You could try asking a bot to squash commits https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#squashing. Not sure if it's mandatory.

As this branch has no conflicts with master rebase is most likely optional.

👋 @mloiseleur am I correct?

mloiseleur · 2025-01-13T15:56:15Z

It can be configured with labels, see tide/merge-method-squash.

thanks for this review @ivankatliarchuk 👍

/label tide/merge-method-squash
/lgtm
/approve

k8s-ci-robot · 2025-01-13T15:56:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk, mloiseleur

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:

~~OWNERS~~ [mloiseleur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…r when TSServer is not ready

… when TSServer is not ready

Skip endpoint creation if VirtualServer is not considered ready

6ad8e5f

Signed-off-by: Mikael Johansson <[email protected]>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 8, 2025

k8s-ci-robot requested review from mloiseleur and szuecs January 8, 2025 14:55

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 8, 2025

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 8, 2025

mikejoh changed the title ~~Skip endpoint creation if VirtualServer is not considered ready~~ fix(source/f5_virtualserver): skip endpoint creation if VirtualServer is not considered ready Jan 8, 2025

mikejoh marked this pull request as ready for review January 10, 2025 12:36

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2025

mikejoh mentioned this pull request Jan 10, 2025

feat: Add F5 TransportServer source #4944

Merged

2 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2025

mloiseleur reviewed Jan 10, 2025

View reviewed changes

k8s-ci-robot assigned mloiseleur Jan 10, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2025

Change to warning instead

d5c7f23

Signed-off-by: Mikael Johansson <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2025

k8s-ci-robot changed the title ~~fix(source/f5_virtualserver): skip endpoint creation if VirtualServer is not considered ready~~ fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready Jan 10, 2025

ivankatliarchuk reviewed Jan 10, 2025

View reviewed changes

Earlier return

a70d5d0

ivankatliarchuk approved these changes Jan 12, 2025

View reviewed changes

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 13, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2025

k8s-ci-robot merged commit 9368a24 into kubernetes-sigs:master Jan 13, 2025
14 checks passed

visokoo added a commit to visokoo/external-dns that referenced this pull request Jan 21, 2025

fix: add @mikejoh's commit changes (a70d5d0, kubernetes-sigs#4996) fo…

c1bc2bb

…r when TSServer is not ready

visokoo added a commit to visokoo/external-dns that referenced this pull request Jan 21, 2025

fix: add mikejoh's commit changes (a70d5d0, kubernetes-sigs#4996) for…

5c14cd2

… when TSServer is not ready

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready #4996

fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready #4996

mikejoh commented Jan 8, 2025 •

edited

Loading

k8s-ci-robot commented Jan 8, 2025

mloiseleur commented Jan 10, 2025

mloiseleur Jan 10, 2025

mikejoh Jan 10, 2025

mikejoh Jan 10, 2025

mloiseleur commented Jan 10, 2025

mloiseleur commented Jan 10, 2025

ivankatliarchuk Jan 10, 2025

mikejoh Jan 11, 2025 •

edited

Loading

ivankatliarchuk Jan 11, 2025 •

edited

Loading

mikejoh Jan 12, 2025

ivankatliarchuk Jan 10, 2025 •

edited

Loading

mikejoh Jan 11, 2025

ivankatliarchuk Jan 10, 2025

mikejoh Jan 11, 2025 •

edited

Loading

ivankatliarchuk Jan 11, 2025

k8s-ci-robot commented Jan 12, 2025

mikejoh commented Jan 12, 2025

ivankatliarchuk commented Jan 12, 2025

mloiseleur commented Jan 13, 2025

k8s-ci-robot commented Jan 13, 2025

	log.Infof("F5 VirtualServer %s/%s is not ready or is missing an IP address, skipping endpoint creation.",
	log.Warnf("F5 VirtualServer %s/%s is not ready or is missing an IP address, skipping endpoint creation.",

fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready #4996

fix(f5-virtualserver): skip endpoint creation when VirtualServer is not ready #4996

Conversation

mikejoh commented Jan 8, 2025 • edited Loading

k8s-ci-robot commented Jan 8, 2025

mloiseleur commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloiseleur commented Jan 10, 2025

mloiseleur commented Jan 10, 2025

Choose a reason for hiding this comment

mikejoh Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

ivankatliarchuk Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankatliarchuk Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejoh Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 12, 2025

mikejoh commented Jan 12, 2025

ivankatliarchuk commented Jan 12, 2025

mloiseleur commented Jan 13, 2025

k8s-ci-robot commented Jan 13, 2025

mikejoh commented Jan 8, 2025 •

edited

Loading

mikejoh Jan 11, 2025 •

edited

Loading

ivankatliarchuk Jan 11, 2025 •

edited

Loading

ivankatliarchuk Jan 10, 2025 •

edited

Loading

mikejoh Jan 11, 2025 •

edited

Loading