Fix invalid handling of ip addresses in comparisons

[SgtCoDFish]

Pull Request Motivation

Prompted by #6265 which included an example allowing for reproduction of the bug ❤️

This fix was discovered in two parts:

1. I discovered we weren't using a more correct IP comparison function we already had in our util package
2. I discovered that the function we already had was inefficient and poorly tested

The underlying issue is also twofold here:

1. IPv6 addresses often have multiple printed representations which are different strings but which represent the same IP address, meaning that comparing strings is incorrect for IP addresses. Consider 2a0e:2345:6789:1:1:2::6 and 2a0e:2345:6789:0001:0001:0002:0000:0006 which are the same. The same is true for IPv4 addresses but much less commonly seen, e.g. 001.001.001.001 and 1.1.1.1.
2. IPv4 addresses have multiple representations in Go. They can be either 4 bytes or embedded into a 16-byte slice (which is the difference between 1.1.1.1 and ::ffff:1.1.1.1). Again, comparing strings would be incorrect here.

I think the original EqualIPsUnsorted function might have been correct but it was extremely inefficient and risky to convert to strings in any case, so I've changed it to use byte slices (which is what IP addresses are in Go anyway).

Further Work

There's a lot more cleanup that could be done in https://github.com/cert-manager/cert-manager/blob/16b77849dd80df3083e6948e8db6313930e8f20a/pkg/util/util.go. Notably it has an init() which affects global state, and there are other improvements and cleanups we could amke.

More importantly,

cert-manager/pkg/util/util_test.go

Line 77 in 16b7784

func TestEqualURLsUnsorted(t *testing.T) {

is a mess and the test doesn't actually test much that's useful. I didn't want to expand the scope of this PR too much though.

Kind

/kind bug

Release Note

Fixes an issue where cert-manager would incorrectly reject two IP addresses as being unequal when they should have compared equal. This would be most noticeable when using an IPv6 address which doesn't match how Go's `net.IP.String()` function would have printed that address.

[inteon]

Thanks, this is a very important bugfix.
I have one small remaining remark (see below).

[inteon]

/cherrypick release-1.12

[jetstack-bot]

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.12

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.

[inteon]

/approve
/lgtm
Making cert-manager even better.

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, SgtCoDFish

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 [SgtCoDFish,inteon]

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

[jetstack-bot]

@SgtCoDFish: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-license	bbbc758	link	false	/test pull-cert-manager-master-license

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[jetstack-bot]

@inteon: new pull request created: #6295

In response to this:

/cherrypick release-1.12

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.