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.

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

return IP address using kubeconfig --internal #1583

Closed
wants to merge 1 commit into from

Conversation

aojea
Copy link
Contributor

@aojea aojea commented May 13, 2020

Before 0.8.0 kind get kubeconfig --internal returned the container
IP address of the node with the API server.

Since we added docker custom bridges, the behavior changed
returning the hostname.

We recover the previous behavior returning an IP address, however,
it will return always an IPv4 address.

Fixes: #1558

Before 0.8.0 kind get kubeconfig --internal returned the container
IP address of the node with the API server.

Since we added docker custom bridges, the behavior changed
returning the hostname.

We recover the previous behavior returning an IP address, however,
it will return always an IPv4 address.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aojea
To complete the pull request process, please assign munnerz
You can assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot requested review from krzyzacy and munnerz May 13, 2020 11:13
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 13, 2020
@aojea
Copy link
Contributor Author

aojea commented May 13, 2020

/assign @BenTheElder

I'm not super happy but I couldn't find a "clean" way of getting the cluster IP family without adding a new method to the provider or a new variable indicating it.

Totally open to suggestions

@k8s-ci-robot
Copy link
Contributor

@aojea: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kind-conformance-parallel-ipv6 3839cf8 link /test pull-kind-conformance-parallel-ipv6
pull-kind-e2e-kubernetes-1-18 3839cf8 link /test pull-kind-e2e-kubernetes-1-18

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.

@BenTheElder
Copy link
Member

BenTheElder commented May 13, 2020 via email

@aojea
Copy link
Contributor Author

aojea commented May 13, 2020

I don't think most internal clients should use IP still.

IIRC that the original purpose of the flag was for obtaining the docker IP, it may be used for other containers that do not belong to the cluster.
Internal clients on the nodes can access directly the /etc/kubernetes/kubeadm.config

@aojea
Copy link
Contributor Author

aojea commented May 13, 2020

adding a new parameter is too gross?
--internal (ipv4|ipv6|dns) (default ipv4)

@BenTheElder
Copy link
Member

IIRC that the original purpose of the flag was for obtaining the docker IP, it may be used for other containers that do not belong to the cluster.

no, the purpose of the flag is to obtain a kubeconfig for use inside the cluster network as opposed to the one we export for the host.

wanting it to be an IP instead of whatever the internal default management is is ... different.
it should probably be it's own flag, technically it could apply to the host but probably have a different default (host default to IP to avoid issues where we don't control /etc/hosts, internal default to dns...)

@BenTheElder BenTheElder added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2020
@k8s-ci-robot
Copy link
Contributor

@aojea: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2020
@BenTheElder
Copy link
Member

btw when I tag an issue kind/design that's because we need to design something like this new flag, I wouldn't have tagged a trivial fix this way 🙃
I'm not sure what the clearest flags would be.

@aojea
Copy link
Contributor Author

aojea commented May 15, 2020

heh my fault, honestly, didn't see the tags and make my mind around it thinking that they only wanted the IPs, let's close and discuss then
/close

@k8s-ci-robot
Copy link
Contributor

@aojea: Closed this PR.

In response to this:

heh my fault, honestly, didn't see the tags and make my mind around it thinking that they only wanted the IPs, let's close and discuss then
/close

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.

@aojea aojea deleted the internalkconfig branch May 15, 2020 20:39
nmiculinic added a commit to kubermatic/kubecarrier that referenced this pull request May 25, 2020
kubermatic-bot pushed a commit to kubermatic/kubecarrier that referenced this pull request May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a way to get kubeconfig with node IP
3 participants