Ignore empty or invalid IP in Endpoint Set*IP

[tpantelis]

No description provided.

[submariner-bot]

🤖 Created branch: z_pr3281/tpantelis/handle_empty_ip
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

[skitt]

Wouldn’t it be better to return an error?

[tpantelis]

Perhaps for an invalid IP but then every caller has to check the error even though it's likely not possible for it to pass an invalid IP.

Handling an empty IP without failing avoids more generic client code from having to first verify non-empty. Eg

for _, family := range submSpec.GetIPFamilies() {
    publicIP, resolver, err := getPublicIP(family, submSpec, k8sClient, backendConfig, airGappedDeployment)
    ...

    endpointSpec.SetPublicIP(publicIP)
}

getPublicIP would currently return empty for IPv6 b/c we haven't implemented it yet. If SetPublicIP returned an error on empty, we would want to ignore it here or check if non-empty first. Either way we wouldn't care about the return error but we'd still have to handle it to avoid a linter error.

[skitt]

OK, so if this is about handling empty strings, I think it’s best to be explicit about it:

func setIP(ips []string, ipv4Fallback, newIP string) ([]string, string) {
	if newIP == "" {
		return ips, ipv4Fallback
	}

	family := k8snet.IPFamilyOfString(newIP)
	…

Handling IPFamilyUnknown suggests that it’s about handling invalid addresses, not IPs (at least, to me).

[skitt]

Or add a comment mentioning that IPFamilyUnknown corresponds to empty addresses 😉.

[tpantelis]

Checking IPFamilyUnknown handles both empty and invalid as neither translates to V4 or V6. I'll add explicit check for empty so that IPFamilyUnknown means invalid and log an error message.

[submariner-bot]

🤖 Closed branches: [z_pr3281/tpantelis/handle_empty_ip]