Add a transport package for clients #16126

krousey · 2015-10-22T20:45:57Z

This is pulling the transport logic out of pkg/client/unversioned/[transport|helpers|debugging].go and completely replacing the utility function in pkg/client/unversioned/auth.

@kubernetes/goog-csi

k8s-github-robot · 2015-10-22T21:10:53Z

Labelling this PR as size/XXL

k8s-bot · 2015-10-22T21:26:10Z

GCE e2e test build/test passed for commit a89c002de780ce9b2844b5d266e17ed4426ba649.

lavalamp · 2015-10-22T23:50:53Z

I skimmed and nothing jumped out at me.

krousey · 2015-10-26T22:00:52Z

I moved some unit tests over that I missed. I also changed the unversioned client to use the new transport. This breaks the build overall, but pkg/client/unversioned builds and passes tests locally.

k8s-bot · 2015-10-26T22:02:35Z

GCE e2e build/test failed for commit e16649968ddf25825cb2d7b514e70449b5f7365e.

krousey · 2015-10-27T21:08:25Z

Moved more packages over... I'm probably going to rebase now and repush... build will likely still be broken until I track down all the uses of the old client config.

k8s-bot · 2015-10-27T21:09:37Z

GCE e2e build/test failed for commit c15645564051038bea9db3e27a48325f5c166cb9.

k8s-bot · 2015-10-27T21:13:55Z

GCE e2e build/test failed for commit 07e8fcd9adecba1cb21dff9e71be37f2378dbfdd.

krousey · 2015-10-28T00:41:00Z

Ok this should be ready for a full review now.

krousey · 2015-10-28T00:41:54Z

Let me know when you want me to squash... I hope the e2e passes 😄

caesarxuchao · 2015-10-28T00:42:55Z

Great. I'll take a look. Thanks.

k8s-bot · 2015-10-28T01:10:43Z

GCE e2e test build/test passed for commit 09be9089ffa20ced1f49dd025d8bfe5ca899176a.

krousey · 2015-10-28T06:44:46Z

@karlkfi I couldn't figure out what was wrong with the latest mesos smoke test. Any ideas?

krousey · 2015-10-28T17:01:52Z

@k8s-bot unit test this

krousey · 2015-10-28T17:02:16Z

cc @lavalamp I think @caesarxuchao is unavailable today.

krousey · 2015-10-28T17:31:42Z

cc @sttts for the last commit

Thanks 😄

krousey · 2015-10-28T17:32:11Z

@k8s-bot unit test this

sttts · 2015-10-28T17:38:05Z

Last change looks sensible. Let's wait for teamcity.

liggitt · 2015-11-16T17:19:05Z

pkg/client/transport/cache.go

+	}
+
+	// Cache a single transport for these options
+	c.transports[key] = &http.Transport{


these defaults aren't equivalent to what we had before... it is not setting the same Dial impl as the DefaultTransport

Actually I missed that. sorry.

Is there a reason not to use the same defaulting function as before, just setting the custom tlsConfig?

Avoiding bloated util libraries to prevent needless dependencies. One of
the main overall goals of the client work is to make it easier for people
to import this.
On Nov 16, 2015 5:20 PM, "Jordan Liggitt" notifications@github.com wrote:

In pkg/client/transport/cache.go
#16126 (comment)
:

if t, ok := c.transports[key]; ok {

return t, nil

}

// Get the TLS options for this client config

tlsConfig, err := TLSConfigFor(config)

if err != nil {

return nil, err

}

// The options didn't require a custom TLS config

if tlsConfig == nil {

return http.DefaultTransport, nil

}

// Cache a single transport for these options

c.transports[key] = &http.Transport{

Is there a reason not to use the same defaulting function as before, just
setting the custom tlsConfig?

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16126/files#r45010982.

If anything, that function belongs in this package. Just don't want that code copied and pasted in lots of places again (for the exact reason that the refactor caused a behavior change...)

Perhaps. It's really not that much code. And I don't want to add more to this PR.

liggitt · 2015-11-16T17:20:08Z

when doing a big refactor like this, please call out subtle behavior changes like https://github.com/kubernetes/kubernetes/pull/16126/files#diff-0aac276a340a1b1786ed17ad728ddf99R62 or #16126 (diff) if they are intentional...

krousey · 2015-11-16T22:51:32Z

I rebased and fixed the issues you pointed out. The fixes are only in the last 3 commits, but they are all "new" commits because of a rebase.

k8s-bot · 2015-11-16T23:02:32Z

GCE e2e build/test failed for commit 4ac36a556deed64aceb2793d1dd39b758cb1861c.

krousey · 2015-11-17T00:48:07Z

@k8s-bot test this please

k8s-bot · 2015-11-17T01:23:06Z

GCE e2e build/test failed for commit 4ac36a556deed64aceb2793d1dd39b758cb1861c.

krousey · 2015-11-17T17:35:15Z

@k8s-bot e2e test this

k8s-bot · 2015-11-17T18:07:02Z

GCE e2e test build/test passed for commit 4ac36a556deed64aceb2793d1dd39b758cb1861c.

k8s-bot · 2015-11-18T01:10:22Z

GCE e2e test build/test passed for commit 4ac36a556deed64aceb2793d1dd39b758cb1861c.

krousey · 2015-11-19T17:23:19Z

@liggitt Any other concerns?

Should this be ok now?

cc @caesarxuchao

liggitt · 2015-11-19T17:31:25Z

pkg/client/transport/config.go

+// BasicAuthConfig holds a username and password used for basic
+// authentication.
+type BasicAuthConfig struct {
+	User     string


nit, Username to match existing field name, and to clarify it is a name, not a User object

krousey · 2015-11-19T19:01:08Z

@liggitt nit addressed

k8s-bot · 2015-11-19T19:34:19Z

GCE e2e test build/test passed for commit 5781f3ded5ba793bf7d4e8c6b777dba8958be450.

krousey · 2015-11-19T22:36:12Z

@liggitt I moved the auth fields to the top level config. If this looks fine to you, I will squash for the merge.

liggitt · 2015-11-19T22:43:48Z

lgtm, squash away

caesarxuchao · 2015-11-19T22:49:34Z

Thanks @liggitt @krousey. LGTM.

Please add the LGTM label yourself after squash :)

k8s-bot · 2015-11-19T23:18:20Z

GCE e2e test build/test passed for commit 989816274a72d9ff6691dfe47bfc70531905a1fa.

k8s-bot · 2015-11-19T23:36:44Z

GCE e2e test build/test passed for commit 9b75b88.

k8s-github-robot · 2015-11-20T09:15:19Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-11-20T09:45:13Z

GCE e2e test build/test passed for commit 9b75b88.

k8s-github-robot · 2015-11-20T09:47:21Z

Automatic merge from submit-queue

Auto commit by PR queue bot

krousey assigned lavalamp Oct 22, 2015

googlebot added the cla: yes label Oct 22, 2015

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 22, 2015

lavalamp assigned caesarxuchao and unassigned lavalamp Oct 22, 2015

caesarxuchao closed this Oct 23, 2015

caesarxuchao reopened this Oct 23, 2015

krousey force-pushed the client_breakup branch from a89c002 to e166499 Compare October 26, 2015 21:59

krousey force-pushed the client_breakup branch from e166499 to c156455 Compare October 27, 2015 21:07

krousey force-pushed the client_breakup branch from c156455 to 07e8fcd Compare October 27, 2015 21:12

krousey force-pushed the client_breakup branch from 07e8fcd to 09be908 Compare October 28, 2015 00:40

krousey changed the title ~~[WIP] Add a transport package for clients~~ Add a transport package for clients Oct 28, 2015

liggitt reviewed Nov 16, 2015
View reviewed changes

krousey force-pushed the client_breakup branch from 9000353 to 4ac36a5 Compare November 16, 2015 22:50

liggitt reviewed Nov 19, 2015
View reviewed changes

Move client's tranport to a separate package

9b75b88

krousey force-pushed the client_breakup branch from 9898162 to 9b75b88 Compare November 19, 2015 22:56

krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2015

k8s-github-robot pushed a commit that referenced this pull request Nov 20, 2015

Merge pull request #16126 from krousey/client_breakup

f0b07c9

Auto commit by PR queue bot

k8s-github-robot merged commit f0b07c9 into kubernetes:master Nov 20, 2015

wojtek-t mentioned this pull request Dec 8, 2015

Set timeout for clients created in e2e tests #18340

Merged

This was referenced Jan 20, 2016

Client transports no longer implement WrappedRoundTripper() #19885

Closed

Re-export Basic/Bearer/User-Agent transport helpers #19886

Closed

Add a transport package for clients #16126

Add a transport package for clients #16126

Conversation

krousey commented Oct 22, 2015

k8s-github-robot commented Oct 22, 2015

k8s-bot commented Oct 22, 2015

lavalamp commented Oct 22, 2015

krousey commented Oct 26, 2015

k8s-bot commented Oct 26, 2015

krousey commented Oct 27, 2015

k8s-bot commented Oct 27, 2015

k8s-bot commented Oct 27, 2015

krousey commented Oct 28, 2015

krousey commented Oct 28, 2015

caesarxuchao commented Oct 28, 2015

k8s-bot commented Oct 28, 2015

krousey commented Oct 28, 2015

krousey commented Oct 28, 2015

krousey commented Oct 28, 2015

krousey commented Oct 28, 2015

krousey commented Oct 28, 2015

sttts commented Oct 28, 2015

liggitt Nov 16, 2015

Choose a reason for hiding this comment

krousey Nov 16, 2015

Choose a reason for hiding this comment

liggitt Nov 17, 2015

Choose a reason for hiding this comment

krousey Nov 17, 2015

Choose a reason for hiding this comment

liggitt Nov 17, 2015

Choose a reason for hiding this comment

krousey Nov 19, 2015

Choose a reason for hiding this comment

liggitt commented Nov 16, 2015

krousey commented Nov 16, 2015

k8s-bot commented Nov 16, 2015

krousey commented Nov 17, 2015

k8s-bot commented Nov 17, 2015

krousey commented Nov 17, 2015

k8s-bot commented Nov 17, 2015

k8s-bot commented Nov 18, 2015

krousey commented Nov 19, 2015

liggitt Nov 19, 2015

Choose a reason for hiding this comment

krousey commented Nov 19, 2015

k8s-bot commented Nov 19, 2015

krousey commented Nov 19, 2015

liggitt commented Nov 19, 2015

caesarxuchao commented Nov 19, 2015

k8s-bot commented Nov 19, 2015

k8s-bot commented Nov 19, 2015

k8s-github-robot commented Nov 20, 2015

k8s-bot commented Nov 20, 2015

k8s-github-robot commented Nov 20, 2015