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

Modify kubeconfig by using client-go, not regex #520

Closed

Conversation

marckhouzam
Copy link

This commit uses k8s.io/client-go to modify the kubeconfig
file to use a localhost server.

Signed-off-by: Marc Khouzam [email protected]

This commit uses k8s.io/client-go to modify the kubeconfig
file to use a localhost server.

Signed-off-by: Marc Khouzam <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 12, 2019
@k8s-ci-robot k8s-ci-robot requested review from amwat and krzyzacy May 12, 2019 02:40
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marckhouzam
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: bentheelder

If they are not already assigned, you can assign the PR to them by writing /assign @bentheelder in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 12, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @marckhouzam. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@marckhouzam
Copy link
Author

@BenTheElder I made the assumption that Kind was targeting a single kubernetes version, but now I'm thinking that is not correct.

If Kind supports multiple versions of Kubernetes, then I'm not sure it is possible to use client-go, as I believe we need to choose which version of client-go to use based on the version of Kubernetes. What do you think?

@aojea
Copy link
Contributor

aojea commented May 12, 2019

reading the Versioning chapter of the README they are following semver so I think that we should use latest stable tag? in this case is v1.11.0
What do you think?

@marckhouzam
Copy link
Author

@aojea you're right that v1.11.0 would be better. I'm just not sure if all versions of kubernetes have the same data structure to read the kubeconfig file. So if Kind prepared a cluster for, say, k8s v1.10, will client-go v1.11 still work?

I'm out of my depth here...

@BenTheElder
Copy link
Member

kind indeed supports multiple versions of kubernetes. out of the box there is a default version, but kind's initial support was (and is) actually for developing kubernetes, where we may be building fairly arbitrary (v1.11+) versions of kubernetes and then running them with kind.

@neolit123
Copy link
Member

I'm just not sure if all versions of kubernetes have the same data structure to read the kubeconfig file

the client-go Config type is unlikely to change anytime soon. if it does, v1 will be supported for a long period of time. thus using any recent version of client-go/tools/clientcmd should be fine.

@BenTheElder BenTheElder added this to the 0.4 milestone May 15, 2019
@k8s-ci-robot
Copy link
Contributor

@marckhouzam: 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, 2019
@BenTheElder
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2019
@BenTheElder BenTheElder modified the milestones: v0.4.0, v0.5.0 Jun 25, 2019
Copy link
Member

@munnerz munnerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, I added a few comments 😄 thanks!

gopkg.in/airbrake/gobrake.v2 v2.0.9 // indirect
gopkg.in/gemnasium/logrus-airbrake-hook.v2 v2.1.2 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
k8s.io/api v0.0.0-20181026145037-6e4b5aa967ee // indirect
k8s.io/apimachinery v0.0.0-20181126191516-4a9a8137c0a1
k8s.io/client-go v7.0.0+incompatible
k8s.io/client-go v10.0.0+incompatible
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect that k8s.io/apimachinery, k8s.io/api and potentially even k8s.io/klog are bumped in the go.sum file after this change? It seems like go mod tidy (or hack/update-vendor.sh) needs running 😄

@@ -35,6 +30,8 @@ import (
"sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer"
"sigs.k8s.io/kind/pkg/cluster/nodes"
"sigs.k8s.io/kind/pkg/exec"

"k8s.io/client-go/tools/clientcmd"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: external imports should exist in the 2nd block (after stdlib imports but before imports within this module)

cmd := n.Command("cat", "/etc/kubernetes/admin.conf")
lines, err := exec.CombinedOutputLines(cmd)
buff, err := exec.Output(cmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm not too familiar here, so please excuse my question if it's a stupid one!) why the switch from CombinedOutputLines to Output?

@BenTheElder BenTheElder removed this from the v0.5.0 milestone Aug 20, 2019
@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kind-verify d21c9f5 link /test pull-kind-verify
pull-kind-build d21c9f5 link /test pull-kind-build
pull-kind-conformance-parallel-1-14 d21c9f5 link /test pull-kind-conformance-parallel-1-14
pull-kind-conformance-parallel-1-13 d21c9f5 link /test pull-kind-conformance-parallel-1-13
pull-kind-conformance-parallel-1-12 d21c9f5 link /test pull-kind-conformance-parallel-1-12
pull-kind-conformance-parallel d21c9f5 link /test pull-kind-conformance-parallel
pull-kind-conformance-parallel-1-15 d21c9f5 link /test pull-kind-conformance-parallel-1-15
pull-kind-unit d21c9f5 link /test pull-kind-unit
pull-kind-conformance-parallel-1-16 d21c9f5 link /test pull-kind-conformance-parallel-1-16
pull-kind-e2e-kubernetes d21c9f5 link /test pull-kind-e2e-kubernetes

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

thanks again for the PR, we've removed client-go as a direct or indirect dependency for the kind binary which makes it easier to import in the future and the binary much smaller.

I'm going to look at doing something similar without client-go but without regex as part of #850

@marckhouzam
Copy link
Author

Sorry for losing track of this.
I'll be checking out #850.

stg-0 added a commit to stg-0/kind that referenced this pull request Mar 25, 2024
* Added required images to images reference

* Added eks images doc

* improve doc

* improve doc

* improve doc

* improve doc

* added docs

* added docs

* improve gitignore

* removed images tool

* rollback to provider.go

* rollback to provider.go

* improve docs

* fixed links

* Update stratio-docs/es/modules/offline-installations/pages/aws/v1.26.x/eks/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/aws/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/azure/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/azure/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/azure/v1.26.x/aks/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/azure/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/commons/v1.26.x/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/gcp/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/gcp/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/gcp/v1.26.x/vms/images.adoc

Co-authored-by: stg <[email protected]>

* Update stratio-docs/es/modules/offline-installations/pages/offline-installations.adoc

Co-authored-by: stg <[email protected]>

* fixed docs

* offline installation review and restructuration

* .

* dead page links

* dead page links

---------

Co-authored-by: stg <[email protected]>
Co-authored-by: tperez-stratio <[email protected]>
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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants