move an apiserver helper function out of pkg/proxy #118680
Conversation
k8s-ci-robot
added
release-note-none
|
/assign @apelisse |
k8s-ci-robot
added
triage/accepted
| @@ -258,27 +214,29 @@ func MapCIDRsByIPFamily(cidrStrings []string) map[v1.IPFamily][]string { | |||
| return ipFamilyMap | |||
| } | |||
|
|
|||
| func getIPFamilyFromIP(ipStr string) (v1.IPFamily, error) { | |||
| // Returns the IP family of ipStr, or "" if ipStr can't be parsed as an IP | |||
| func getIPFamilyFromIP(ipStr string) v1.IPFamily { | |||
There was a problem hiding this comment.
Not a big fan of this change, but I guess it doesn't really matter in practice.
There was a problem hiding this comment.
I considered having it return (v1.IPFamily, bool) but it seemed pointless since the second value is completely redundant with the first, and it's not an exported API...
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 9d96d4a4e9a01a659c98534e6f34d9284bb5c422
|
|
/lgtm |
There was a problem hiding this comment.
Blech on the resting place for this.
Do we really need this as a util function? If so, does it really need to be a public API (apimachinery is more public, while k/k is not considered public - we don't support importing it)?
Is this a place where "a little copy is better than a little dep" applies? Could we just copy this to the 2 users? Simplified a bit, it's pretty trivial.
func IsProxyableIP(ip string) bool {
netIP := netutils.ParseIPSloppy(ip)
if netIP == nil {
return false
}
return ip.IsGlobalUnicast()
}
If we need a util func, why not k/k/pkg/util/net (or similar) ?
pkg/registry/core/node/strategy.go
Outdated
| @@ -241,7 +240,7 @@ func ResourceLocation(getter ResourceGetter, connection client.ConnectionInfoGet | |||
| return nil, nil, err | |||
| } | |||
|
|
|||
| if err := proxyutil.IsProxyableHostname(ctx, &net.Resolver{}, info.Hostname); err != nil { | |||
| if err := utilnet.IsProxyableHostname(ctx, &net.Resolver{}, info.Hostname); err != nil { | |||
There was a problem hiding this comment.
@thockin is right, if this is only used here we can make it local right?
|
I don't think k/util is right for it; this isn't a generic utility. I can't explain why it checks exactly the things that it checks and doesn't check anything else. It's basically tailored to its two existing users, and may not be appropriate for anyone else. I put it in apimachinery because it belongs to the apiserver, but yeah, you're right, that's too public too. yeah, maybe just inline it... |
|
Note we have k/utils which is ostensibly supported and has stong API promises, and we have k/k/pkg/util which is a random mishmash of internal BS with no real guarantees, and we have apimachinery/.../util which has some API guarantees but is less generic than k/utils. I could live with inlining or k/k/pkg/utils |
danwinship
force-pushed
the
not-that-kind-of-proxying
branch
from
1836182
to
94a569b
Compare
k8s-ci-robot
removed
the
lgtm
|
Oh, oops, I missed that you had said The big problem with adding I went with inlining. This means we lose the explicit unit tests for those two functions, but once you inline |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: e5ee618fb46fccfa21b183ff78201c50bb5513cf
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, apelisse, danwinship, thockin 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
4 similar comments
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
These don't belong in pkg/proxy/util; they involve a completely unrelated definition of proxying. Since each is only used from one place, just inline them at the callers.
danwinship
force-pushed
the
not-that-kind-of-proxying
branch
from
94a569b
to
bb0c3a0
Compare
k8s-ci-robot
removed
the
lgtm
|
ugh, sorry, gofmt |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 450b670a1c28358d88c312ffec7a5f09809e55af
|
|
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
pkg/proxy/utilcontains the functionsIsProxyableIPandIsProxyableHostname, but they don't belong there becausepkg/proxy/utilis "utility functions for kube-proxy" not "utility functions that involve the concept of 'proxying' in any sense".I moved them to
k8s.io/apimachinery/pkg/util/netbut maybe there's a better place. (The only callers of the functions are inpkg/registry/core/node/strategy.goandpkg/registry/core/pod/strategy.go.)(Related cleanup: #118120)
Does this PR introduce a user-facing change?