Skip to content
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.

Sign up for GitHub

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

Ignore empty or invalid IP in Endpoint Set*IP #3281

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Ignore empty or invalid IP in Endpoint Set*IP #3281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/apis/submariner.io/v1/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,15 @@ import (
"strconv"

"github.com/pkg/errors"
"github.com/submariner-io/admiral/pkg/log"
"github.com/submariner-io/admiral/pkg/resource"
"k8s.io/apimachinery/pkg/api/equality"
k8snet "k8s.io/utils/net"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

var logger = log.Logger{Logger: logf.Log.WithName("EndpointAPI")}

func (ep *EndpointSpec) GetBackendPort(configName string, defaultValue int32) (int32, error) {
if portStr := ep.BackendConfig[configName]; portStr != "" {
port, err := parsePort(portStr)
Expand Down Expand Up @@ -119,7 +123,15 @@ func getIPFrom(family k8snet.IPFamily, ips []string, ipv4Fallback string) string
}

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

family := k8snet.IPFamilyOfString(newIP)
if family == k8snet.IPFamilyUnknown {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn’t it be better to return an error?

Copy link
Contributor Author

@tpantelis tpantelis Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

logger.Errorf(nil, "Unable to determine IP family for %q - ignoring", newIP)
return ips, ipv4Fallback
}

if family == k8snet.IPv4 {
ipv4Fallback = newIP
Expand Down
30 changes: 30 additions & 0 deletions pkg/apis/submariner.io/v1/endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,16 @@ func testGetIP(ipsSetter func(*v1.EndpointSpec, []string, string), ipsGetter fun
})
})
})

When("the specified family is IPFamilyUnknown", func() {
BeforeEach(func() {
ips = []string{ipV4Addr, ipV6Addr}
})

It("should return empty string", func() {
Expect(ipsGetter(spec, k8snet.IPFamilyUnknown)).To(BeEmpty())
})
})
}

func testSetIP(initIPs func(*v1.EndpointSpec, []string), ipsSetter func(*v1.EndpointSpec, string),
Expand Down Expand Up @@ -309,6 +319,26 @@ func testSetIP(initIPs func(*v1.EndpointSpec, []string), ipsSetter func(*v1.Endp
})
})
})

When("the specified IP is empty", func() {
BeforeEach(func() {
initialIPs = []string{ipV6Addr}
})

It("should not add it", func() {
verifyIPs([]string{ipV6Addr}, "")
})
})

When("the specified IP is invalid", func() {
BeforeEach(func() {
ipToSet = "invalid"
})

It("should not add it", func() {
verifyIPs([]string{}, "")
})
})
}

func testGetHealthCheckIP() {
Expand Down
Loading