Allow in-cluster config for oc #5722

smarterclayton · 2015-11-05T02:26:16Z

Because we set the default env value to empty, we can't use the default
in cluster config for 'oc' (when you run inside a container, oc works).
It's really important for container integration scenarios that oc uses
the service account token by default, just like kubeconfig.

Regression from upstream kubectl behavior, blocks a significant amount of
real integrations inside containers.

smarterclayton · 2015-11-05T02:27:07Z

@liggitt @deads2k @csrwng this unblocks running oc from within a container under the service account token, which means we can use oc inside our containers automatically.

smarterclayton · 2015-11-05T02:27:11Z

[test]

smarterclayton · 2015-11-05T03:29:35Z

I'm going to also fix the stupid message "error in default cluster"

liggitt · 2015-11-05T04:33:57Z

pkg/cmd/util/clientcmd/factory.go

@@ -33,24 +35,62 @@ import (
 	routegen "github.com/openshift/origin/pkg/route/generator"
 )

+// defaultClusterConfigURL is a local name that is used to identify when the client config
+// is unspecified.
+const defaultClusterConfigURL = "https://origin-server.local:8443"


liggitt · 2015-11-05T04:38:13Z

How does this fix in cluster config?

smarterclayton · 2015-11-05T04:47:13Z

In cluster config doesn't work if the default cluster host value is empty.

deads2k · 2015-11-05T13:28:01Z

I'm going to also fix the stupid message "error in default cluster"

Yeah, I remember someone's pull being rejected as "ugly". I look forward to seeing the beautiful version. :)

deads2k · 2015-11-05T13:35:29Z

pkg/cmd/util/clientcmd/factory.go

+	if err != nil {
+		return nil, err
+	}
+	if isDefaultConfig(cfg) {


So if we actually want to solve it this way, why not try to do it upstream? I can't say that I'm really a fan of this.

csrwng · 2015-11-05T15:59:44Z

I tested this and it now works without having to include the --token. However, I would also expect 'oc' to default to the pod's namespace, instead it defaults to 'default' and you have to specify '-n namespace'.

deads2k · 2015-11-05T16:00:11Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/unversioned/clientcmd/validation.go

@@ -143,6 +157,10 @@ func ConfirmUsable(config clientcmdapi.Config, passedContextName string) error {
 func validateClusterInfo(clusterName string, clusterInfo clientcmdapi.Cluster) []error {
 	validationErrors := make([]error, 0)

+	if reflect.DeepEqual(clientcmdapi.Cluster{}, clusterInfo) {


I would expect you have to have problems with nil maps versus empty maps. If you don't have an issue now, be sure to add a test that goes through the load method. I remember a pull upstream that was looking at having empty maps instead of nil ones.

A nil map would still be someone setup the config. I will add a test.

On Thu, Nov 5, 2015 at 11:00 AM, David Eads [email protected]
wrote:

In
Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/unversioned/clientcmd/validation.go
#5722 (comment):

@@ -143,6 +157,10 @@ func ConfirmUsable(config clientcmdapi.Config, passedContextName string) error {
func validateClusterInfo(clusterName string, clusterInfo clientcmdapi.Cluster) []error {
validationErrors := make([]error, 0)

if reflect.DeepEqual(clientcmdapi.Cluster{}, clusterInfo) {

I would expect you have to have problems with nil maps versus empty maps.
If you don't have an issue now, be sure to add a test that goes through the
load method. I remember a pull upstream that was looking at having empty
maps instead of nil ones.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5722/files#r44028845.

csrwng · 2015-11-05T16:03:31Z

related BZ https://bugzilla.redhat.com/show_bug.cgi?id=1278180

deads2k · 2015-11-05T16:07:25Z

At first glance, this looks like it simply changes the message. How is this changing behavior?

deads2k · 2015-11-05T16:07:48Z

pkg/cmd/util/clientcmd/factory.go

+func (c defaultingClientConfig) ClientConfig() (*kclient.Config, error) {
+	cfg, err := c.nested.ClientConfig()
+	if err != nil {
+		if kclientcmd.IsEmptyCluster(err) {


Why would I be inspecting and transforming the error here instead of CheckErr?

More invasive change, also it would result in a because Kube would
have different messages than us.

On Thu, Nov 5, 2015 at 11:07 AM, David Eads [email protected]
wrote:

In pkg/cmd/util/clientcmd/factory.go
#5722 (comment):

+// RawConfig calls the nested method
+func (c defaultingClientConfig) RawConfig() (kclientcmdapi.Config, error) {

return c.nested.RawConfig()
+}

+// Namespace calls the nested method
+func (c defaultingClientConfig) Namespace() (string, bool, error) {

return c.nested.Namespace()
+}

+// ClientConfig returns a complete client config
+func (c defaultingClientConfig) ClientConfig() (*kclient.Config, error) {

cfg, err := c.nested.ClientConfig()

if err != nil {

if kclientcmd.IsEmptyCluster(err) {

Why would I be inspecting and transforming the error here instead of
CheckErr?

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5722/files#r44029872.

smarterclayton · 2015-11-05T17:56:32Z

Now with upstream tests, a cleaner core story, and comments addressed

deads2k · 2015-11-05T17:57:19Z

More invasive change, also it would result in a because Kube would
have different messages than us.

I think we'll want different messages at some point. We should probably add a ErrorInterpretter or some such to kube.

deads2k · 2015-11-05T17:59:04Z

I'm still not seeing where this is changing the client you get for an empty config, but it does give a prettier message.

lgetm

smarterclayton · 2015-11-05T18:01:43Z

It does not any more - Jordan's feedback convinced me not to do that.

On Thu, Nov 5, 2015 at 12:59 PM, David Eads [email protected]
wrote:

I'm still not seeing where this is changing the client you get for an
empty config, but it does give a prettier message.

lgetm

—
Reply to this email directly or view it on GitHub
#5722 (comment).

liggitt · 2015-11-05T20:50:04Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api/helpers.go

@@ -31,6 +31,14 @@ func init() {
 	redactedBytes = []byte(string(sDec))
 }

+// IsConfigEmpty returns true if the config is empty.
+func IsConfigEmpty(config *Config) bool {
+	return len(config.AuthInfos) == 0 && len(config.Clusters) == 0 && len(config.Contexts) == 0 &&


does api.Semantic.DeepEqual(config, &Config{}) not work? That treats nil==map[]{}

I can't require it from here because of circular dependencies.

smarterclayton · 2015-11-05T21:04:43Z

@csrwng do you have KUBECONFIG set to a real file or KUBERNETES_MASTER set?

smarterclayton · 2015-11-05T21:07:50Z

[test]

csrwng · 2015-11-05T21:07:58Z

neither:

bash-4.2$ env | grep KUBECONFIG
bash-4.2$ env | grep KUBERNETES_MASTER

liggitt · 2015-11-05T21:08:25Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/client/unversioned/clientcmd/validation.go

@@ -49,6 +55,16 @@ func IsContextNotFound(err error) bool {
 	return strings.Contains(err.Error(), "context was not found for specified context")
 }

+// IsEmptyConfig returns true if the provided error indicates the provided configuration
+// is empty.
+func IsEmptyConfig(err error) bool {


IsEmptyConfigError, to keep us sane, since we also have IsConfigEmpty

just saw we have other similarly named functions, nm for now I guess

smarterclayton · 2015-11-06T00:18:16Z

Now uses POD_NAMESPACE iff EmptyConfig is returned.

csrwng · 2015-11-06T14:02:38Z

verified that setting POD_NAMESPACE works

smarterclayton · 2015-11-06T15:10:10Z

Working on the e2e test now.

On Fri, Nov 6, 2015 at 9:02 AM, Cesar Wong [email protected] wrote:

verified that setting POD_NAMESPACE works

—
Reply to this email directly or view it on GitHub
#5722 (comment).

smarterclayton · 2015-11-06T17:14:33Z

Should go green, can I get a final sign off on this?

deads2k · 2015-11-06T17:36:00Z

pkg/cmd/util/clientcmd/factory.go

+		return nil, err
+	}
+
+	if icc, err := kclient.InClusterConfig(); err == nil {


I think we should switch to client/unversionted/clientcmd/inClusterClientConfig, but we can do that later.

Yeah it would need to be public and other things have to change (it doesn't implement ClientConfig)

I think we should switch to client/unversionted/clientcmd/inClusterClientConfig, but we can do that later.

Yeah, the .Possible method makes the behavior more obvious. Can we get a TODO?

Added todo, waiting to see if this goes green before pushing (then i'll stick merge on it unless you find more things)

deads2k · 2015-11-06T17:38:36Z

one request for a comment.

lgtm, otherwise.

csrwng · 2015-11-06T17:47:22Z

working for me

smarterclayton · 2015-11-06T17:50:57Z

[merge] - last jenkins wedged

openshift-bot · 2015-11-06T18:56:18Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6960/) (Image: devenv-rhel7_2649)

Because we set the default env value to empty, we can't use the default in cluster config for 'oc' (when you run inside a container, oc works). It's really important for container integration scenarios that oc uses the service account token by default, just like kubeconfig.

smarterclayton · 2015-11-06T19:41:58Z

Temporarily disabling e2e tests so this can merge (cesar has manually validated), but ca.crt generated into the running containers under hack/test-end-to-end-docker.sh has newlines, and ca.crt generated into hack/test-end-to-end.sh does not.

openshift-bot · 2015-11-06T19:48:20Z

Evaluated for origin test up to 13cc7c6

openshift-bot · 2015-11-06T19:48:31Z

Evaluated for origin merge up to 13cc7c6

openshift-bot · 2015-11-06T20:45:20Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6960/)

Merged by openshift-bot

smarterclayton added priority/P1 approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 5, 2015

smarterclayton self-assigned this Nov 5, 2015

smarterclayton modified the milestone: 1.1.0 Nov 5, 2015

smarterclayton force-pushed the allow_incluster_config branch from 03a8358 to 72e4e06 Compare November 5, 2015 03:40

liggitt reviewed Nov 5, 2015
View reviewed changes

smarterclayton force-pushed the allow_incluster_config branch 2 times, most recently from 1a12da7 to 41215b8 Compare November 5, 2015 05:21

deads2k reviewed Nov 5, 2015
View reviewed changes

csrwng mentioned this pull request Nov 5, 2015

examples/gitserver is broken #5600

Closed

smarterclayton force-pushed the allow_incluster_config branch from 41215b8 to 6896447 Compare November 5, 2015 15:56

deads2k reviewed Nov 5, 2015
View reviewed changes

smarterclayton force-pushed the allow_incluster_config branch from 6896447 to db665a7 Compare November 5, 2015 17:52

liggitt reviewed Nov 5, 2015
View reviewed changes

liggitt assigned liggitt and unassigned smarterclayton Nov 5, 2015

smarterclayton force-pushed the allow_incluster_config branch from a420428 to 0d57c59 Compare November 6, 2015 00:18

smarterclayton force-pushed the allow_incluster_config branch 2 times, most recently from ab12593 to b96c13c Compare November 6, 2015 01:55

smarterclayton force-pushed the allow_incluster_config branch from b96c13c to a5f322e Compare November 6, 2015 16:41

deads2k reviewed Nov 6, 2015
View reviewed changes

smarterclayton force-pushed the allow_incluster_config branch from a5f322e to 13cc7c6 Compare November 6, 2015 19:40

openshift-bot pushed a commit that referenced this pull request Nov 6, 2015

Merge pull request #5722 from smarterclayton/allow_incluster_config

49be0e9

Merged by openshift-bot

openshift-bot merged commit 49be0e9 into openshift:master Nov 6, 2015

Allow in-cluster config for oc #5722

Allow in-cluster config for oc #5722

Conversation

smarterclayton commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

Choose a reason for hiding this comment

liggitt commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

deads2k commented Nov 5, 2015

Choose a reason for hiding this comment

csrwng commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Nov 5, 2015

deads2k commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Nov 5, 2015

deads2k commented Nov 5, 2015

deads2k commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Nov 5, 2015

smarterclayton commented Nov 5, 2015

csrwng commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Nov 6, 2015

csrwng commented Nov 6, 2015

smarterclayton commented Nov 6, 2015

smarterclayton commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Nov 6, 2015

csrwng commented Nov 6, 2015

smarterclayton commented Nov 6, 2015

openshift-bot commented Nov 6, 2015

smarterclayton commented Nov 6, 2015

openshift-bot commented Nov 6, 2015

openshift-bot commented Nov 6, 2015

openshift-bot commented Nov 6, 2015