remove GCE cloud provider dependency to pkg/master/ports #73611
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
kind/cleanup
| // NOTE: Please keep the following port in sync with ProxyHealthzPort in pkg/master/ports/ports.go | ||
| // ports.ProxyHealthzPort was not used here to avoid dependencies to k8s.io/kubernetes in the | ||
| // GCE cloud provider which is required as part of the out-of-tree cloud provider efforts. | ||
| lbNodesHealthCheckPort = 10256 |
There was a problem hiding this comment.
Not ideal to duplicate the port value there but it will unblock us from being able to move the GCE provider out-of-tree. I think it's worth the trade-off.
There was a problem hiding this comment.
Shouldn't the ports move to some common api-like repository? E.g. well-known constants. Maybe we can drive that, rather than break these depedencies...
There was a problem hiding this comment.
Ideally yes, but this seems like something that would go through API change reviews. There are also differing opinions on where those port values should live and I would prefer not to be blocked on this for too long. Can we add a TODO here and I will try to follow-up on putting the ports in a common api-like repo (maybe k8s.io/api/v1/well_known_constants.go`)?
There was a problem hiding this comment.
Also we should not be using the hard-coded port as it's override-able at runtime. So this constant won't qualify for that repo either
There was a problem hiding this comment.
+1 for adding a TODO
I believe we should have a default but can be over-riden.
However switching it to a default seems a separate effort from Andrew's dependency break.
k8s-ci-robot
added
sig/cloud-provider
|
/lgtm ( for @cheftako ) |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
@dims what would be the right home for this constant? |
|
@bowei ProxyHealthzPort itself is the default right? and the actual port can be specified in the command line, so we need to be looking up the real port set by the user instead of using |
|
@andrewsykim can you please add a TODO? and then we can get this in? |
…ng the proxy port value
andrewsykim
force-pushed
the
gce-lb-ports
branch
from
62b0c76
to
fc9b63d
Compare
k8s-ci-robot
removed
the
lgtm
|
TODO added |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, bowei 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
removed
the
do-not-merge/hold
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Duplicates the proxy port value to remove dependencies to k8s.io/kubernetes in the GCE provider.
Which issue(s) this PR fixes:
Part of #69585
Special notes for your reviewer:
Not ideal to duplicate the port value there but it will unblock us from being able to move the GCE provider out-of-tree. I think it's worth the trade-off.
Does this PR introduce a user-facing change?:
/assign @cheftako @bowei