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

add kubeconfig types #2861

Merged
merged 2 commits into from Jan 7, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Dec 11, 2014

Adds the ability to read a kubeconfig file (issue #1755).

Addresses #2644 to allow binding to different names and provides suggested names with a user specified prefix.

@bgrant0607
Copy link
Member

@deads2k
Copy link
Contributor Author

deads2k commented Dec 11, 2014

@smarterclayton simple renames squashed in. APIServer and APIVersion captialized. CmdBuilder clarified to FlagBasedBuilder. IsReady() renamed to Complete().

@deads2k deads2k force-pushed the deads-add-clicfg-types branch 2 times, most recently from e162676 to d8acc0a Compare December 15, 2014 21:33
@deads2k
Copy link
Contributor Author

deads2k commented Dec 15, 2014

@smarterclayton read path complete including merging and overriding.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 17, 2014

@smarterclayton Comments addressed except

Remove Client() method.

It does work when you won't want to skew server and client versions. If my intent is to get a client.Client for a particular server,authInfo pair, it doesn't seem unreasonable to provide checking that the client is likely to work. Sort of a validation check before returning. Given that we already have a flag for this in both kubernetes and openshift, it seems reasonable to have a common spot to have it checked. Using the factory doesn't seem very clean. That prevents this code from standing alone and the Factory in kubectl seems only tangentially related to what this code is trying to do.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 17, 2014

@smarterclayton moved match-server-version

return util.SliceToError(validationErrors)
}

// validateAuthInfo looks for conflicts and errors in the cluster info
Copy link
Member

Choose a reason for hiding this comment

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

validateClusterInfo looks for conflicts...

@derekwaynecarr
Copy link
Member

minor nit on godoc, but looks good to me.

@smarterclayton
Copy link
Contributor

Will look later today

On Dec 17, 2014, at 3:27 PM, David Eads notifications@github.com wrote:

@smarterclayton moved match-server-version


Reply to this email directly or view it on GitHub.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 22, 2014

@derekwaynecarr typo fixed.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 5, 2015

@ghodss @jlowdermilk This enables reading multiple cli configuration profiles #1755. Do you guys have comments?

func (builder *builder) Config() (*client.Config, error) {
if builder.config != nil {
return builder.config, nil
// Client implements KubeConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Forget to delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forget to delete this?

Yes. Tidied up.

@j3ffml
Copy link
Contributor

j3ffml commented Jan 5, 2015

LGTM, this fits the design proposed in #1755 nicely. Just nits above.

Might be helpful to add a .md doc that includes an explanation of load order. Maybe cut/paste the text of the comment on getKubeConfig

@j3ffml j3ffml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 5, 2015
@deads2k
Copy link
Contributor Author

deads2k commented Jan 6, 2015

Might be helpful to add a .md doc that includes an explanation of load order.

@jlowdermilk Just a doc under kubernetes/docs?

@deads2k deads2k closed this Jan 6, 2015
@smarterclayton
Copy link
Contributor

Some final comments (one substantial concern about validation, one gap with flag names), otherwise is in good shape to me.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 6, 2015

@smarterclayton Got the renames. I put the error type export in a separate commit so you can confirm that's how you want it.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 7, 2015

@smarterclayton tweaked, rebased, squashed.

@smarterclayton
Copy link
Contributor

LGTM

smarterclayton added a commit that referenced this pull request Jan 7, 2015
@smarterclayton smarterclayton merged commit 585d2c8 into kubernetes:master Jan 7, 2015
smarterclayton added a commit to smarterclayton/kubernetes that referenced this pull request Jan 7, 2015


ClientConfig changed right as rollingupdate was introduced.
@deads2k deads2k deleted the deads-add-clicfg-types branch January 7, 2015 18:54
@deads2k deads2k mentioned this pull request Jan 7, 2015
smarterclayton added a commit that referenced this pull request Jan 7, 2015
Fix inflight merge conflict by adapting rollingupdate to #2861
@deads2k deads2k mentioned this pull request Jan 7, 2015
@brendandburns
Copy link
Contributor

I think that this broke e2e because we didn't make changes to cluster turn up to write the appropriate config file.

@brendandburns
Copy link
Contributor

I would lean towards rolling this back, and then have a re-revert with changes to kube-up function cluster/*/

Sorry!

@zmerlynn
Copy link
Member

zmerlynn commented Jan 7, 2015

/sub (chasing this with @brendanburns)

@deads2k
Copy link
Contributor Author

deads2k commented Jan 7, 2015

@brendanburns All command line flags were kept exactly the same and the file isn't supposed to be required, however vagrant e2e has always been hit or miss so I got the same results as I got on master and considered it good.

Would you like me to do the revert? Also, do you have a gist with the failure?

brendandburns added a commit that referenced this pull request Jan 7, 2015
…ter_conflict

Revert "Fix inflight merge conflict by adapting rollingupdate to #2861"
@zmerlynn
Copy link
Member

zmerlynn commented Jan 7, 2015

The e2e failure looks about like this:

�[0;32mKubernetes cluster is running.  The master is running at:

�[0;33m  https://130.211.187.55

�[0;32mThe user name and password to use is located in ~/.kubernetes_auth.�[0m

... calling validate-cluster
Project: kubernetes-jenkins
Project: kubernetes-jenkins
Running: /jenkins-master-data/jobs/kubernetes-e2e-gce/workspace/kubernetes/cluster/../cluster/gce/../../cluster/../cluster/gce/../../cluster/../cluster/gce/../../platforms/linux/amd64/kubectl get minions -o template -t {{range.items}}{{.id}}
{{end}}
F0107 20:34:08.663798   22537 get.go:75] Get https://130.211.187.55/api/v1beta1/minions?namespace=default: x509: certificate signed by unknown authority
2015/01/07 20:34:08 Error running up: exit status 255
2015/01/07 20:34:08 Error starting e2e cluster. Aborting.
exit status 1

@deads2k
Copy link
Contributor Author

deads2k commented Jan 7, 2015

Ok, well that's pretty bad. I see the problem. .kubernetes_auth contains information for the server to identify the user and information for the client to identify the server. I lost the latter half during a refactor.

I'll put together a change to restore that and show that succeeding. I'm sorry about this.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 7, 2015

@zmerlynn Forgive my ignorance, but is there an easy way for me to run e2e using the gke provider from outside google (getting started lists gce, but not gke)?

While there was a bug in respecting "Insecure" from a .kubernetes_auth file, that didn't affect the vagrant environment either way and I noticed that "--auth_path" is handled differently for gke depending on whether it is kubecfg of kubectl. Basically, it means that making sure auth for e2e runs properly vagrant (It did before too) doesn't mean that it will work on gke.

Alternatively, if the ~/.kubernetes_auth from your failing test had "Insecure":true, then we're golden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants