Since this was reported in Kubernetes, I'd prefer the whole ecosystem to be consistent about registries URIs, rather than fix it in Kubernetes only.
cc :@dims @mikebrow @thaJeztah

Also wondering if we can use net.SplitHostPort() for these 🤔 https://pkg.go.dev/net#SplitHostPort

Also wondering if we can use net.SplitHostPort() for these https://pkg.go.dev/net#SplitHostPort

I used the regex based on current state and previous PRs, but I rather prefer to validate with the golang stdlib functions if possible, it seems less error prone (unless stdlib changes of course, but this only happened in 1.17 for ipv4 addresses)

Yes, perhaps there's other reasons for the regexes that I didn't think of, but I saw them, and know that it's easy to make a mistake in regexes (besides them known to be potentially more power-hungry), so if possible, using stdlib, that would be my preference.

Of course that's orthogonal to the " the whole ecosystem to be consistent about registries URIs, rather than fix it in Kubernetes only." question

For that, given that currently, IPv4[:<port>] is accepted, I (personally) see no reason why the same shouldn't be supported for IPv6. Assuming, of course it doesn't conflict with existing expectations, but (first glance) this PR is only adding more test-cases, and all existing tests continue to pass, so it looks solid.

can you please rebase @aojea

can you please rebase @aojea

@milosgajdos rebased :)

it needs other approval, right?

yeah, pinging @thaJeztah

CC: @wy65701436 @deleteriousEffect

Codecov Report

Merging #3489 (d897a19) into main (3e4f8a0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3489   +/-   ##
=======================================
  Coverage   56.58%   56.58%           
=======================================
  Files         103      103           
  Lines        7520     7520           
=======================================
  Hits         4255     4255           
  Misses       2596     2596           
  Partials      669      669           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e4f8a0...d897a19. Read the comment docs.

Let me post my comment form #3667 here as well

my concern in that one specifically (but partially this one as well), is the huge dependency on more-and-more complex regexes; I know there's been efforts to simplify them (as they are notorious for being a resource-hog).

From that perspective (but I'd have to have a closer look at how they're all used), I'd rather have a "good enough" regex to split the elements, followed by a "regular" check to make further determination, e.g.;

1. split first element if it contains a ., a [ or a : (in which case we "assume" it's a hostname or IP-address)
2. call net.SplitHostPort (and/or other functions) to perform more specific validation

With that, we avoid running a (likely) heavy regex for each use and will be able to return a more specific error message ("port out of range"), instead of "invalid reference".

/hold

let me work in the stdlib approach, I do prefer that too, this IPv6 regex is not something easier to think about

ok, it seems I had a misconception, we don't need full validation, as @thaJeztah says we just need a simpler regex

I was comparing with golang url.Parse and it doesn't perform validation too
https://go.dev/play/p/D0m9NZ1AMJL

I think I'm close now, just missing the referenceRegex

I think I'm close now, just missing the referenceRegex

Ok, this is ready

Ipv4 has dots and digits, that are valid repository names, ipv6 has invalid character for repository names.
However both ipv6 and ipv4.will be valid domains

/hold cancel

@thaJeztah @mikebrow @milosgajdos

the current approach is consistent with the golang url.Parse implementation, it just parses without validation,

@milosgajdos I'm not familiar with the workflow here, what is missing?

@aojea sorry for the radio silence on my end. LGTM.

@aojea sorry for the radio silence on my end. LGTM.

no worries,

thanks so much

by the way, how often do you use to release, so I can revendor it in kubernetes to fix kubernetes/kubernetes#104491

We don't have a regular cadence. main is currently set up to become v3 but for that we'll have to have a proper release plan which is not quite the case at the moment. I shall try and put something together, though I wouldn't expect that to become the reality earlier than the EOY 😞

Fair enough