Merge pull request #18709 from sosiouxme/20180221-diagnostic-reorg

Automatic merge from submit-queue (batch tested with PRs 18778, 18709, 18876, 18897, 18652).

diagnostic reorg and NetworkCheck fix

The leading commits of this PR change nothing visible to the user. Internally, they remove a direct dependency on docker and reorganize the two diagnostics that currently run diagnostics in a pod, PodDiagnostic and NetworkCheck. The reorg brings files for each under one directory so it's easier to find and understand them.

The final two commits address #16596 by making NetworkCheck a cluster diagnostic and constructing the pod kubeconfig from the (now guaranteed to be an admin) config.

Commits can be squashed or split out into separate PRs if preferable. I would expect it to be rather confusing to review as a whole, especially since a lot of the changes are just stuff moving around; easier to to grasp one commit at a time.

Author: openshift-merge-robot

pkg/oc/admin/diagnostics/client.go

@@ -7,24 +7,23 @@ import (
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
 
 	clientdiags "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/client"
-	networkdiags "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/network"
+	"github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/client/pod"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types"
 )
 
 // availableClientDiagnostics returns definitions of client diagnostics that can be executed
 // during a single run of diagnostics. Add more diagnostics to the list as they are defined.
 func availableClientDiagnostics() types.DiagnosticList {
-	return types.DiagnosticList{clientdiags.ConfigContext{}, &clientdiags.DiagnosticPod{}, &networkdiags.NetworkDiagnostic{}}
+	return types.DiagnosticList{clientdiags.ConfigContext{}, &pod.DiagnosticPod{}}
 }
 
 // buildClientDiagnostics builds client Diagnostic objects based on the rawConfig passed in.
 // Returns the Diagnostics built, and any fatal errors encountered during the building of diagnostics.
 func (o DiagnosticsOptions) buildClientDiagnostics(rawConfig *clientcmdapi.Config) ([]types.Diagnostic, error) {
 	available := availableClientDiagnostics().Names()
 
-	networkClient, err := o.Factory.OpenshiftInternalNetworkClient()
 	kubeClient, clientErr := o.Factory.ClientSet()
-	if clientErr != nil || err != nil {
+	if clientErr != nil {
 		o.Logger().Notice("CED0001", "Could not configure a client, so client diagnostics are limited to testing configuration and connection")
 		available = sets.NewString(clientdiags.ConfigContextsName)
 	}
@@ -45,24 +44,14 @@ func (o DiagnosticsOptions) buildClientDiagnostics(rawConfig *clientcmdapi.Confi
 					diagnostics = append(diagnostics, diagnostic)
 				}
 			}
-		case clientdiags.DiagnosticPodName:
-			dp := o.ParameterizedDiagnostics[diagnosticName].(*clientdiags.DiagnosticPod)
+		case pod.DiagnosticPodName:
+			dp := o.ParameterizedDiagnostics[diagnosticName].(*pod.DiagnosticPod)
 			dp.KubeClient = kubeClient
 			dp.Namespace = rawConfig.Contexts[rawConfig.CurrentContext].Namespace
 			dp.Level = o.LogOptions.Level
 			dp.Factory = o.Factory
 			dp.PreventModification = dp.PreventModification || o.PreventModification
 			diagnostics = append(diagnostics, dp)
-		case networkdiags.NetworkDiagnosticName:
-			nd := o.ParameterizedDiagnostics[diagnosticName].(*networkdiags.NetworkDiagnostic)
-			nd.KubeClient = kubeClient
-			nd.NetNamespacesClient = networkClient.Network()
-			nd.ClusterNetworkClient = networkClient.Network()
-			nd.ClientFlags = o.ClientFlags
-			nd.Level = o.LogOptions.Level
-			nd.Factory = o.Factory
-			nd.PreventModification = o.PreventModification
-			diagnostics = append(diagnostics, nd)
 		default:
 			return nil, fmt.Errorf("unknown diagnostic: %v", diagnosticName)
 		}

pkg/oc/admin/diagnostics/cluster.go

@@ -14,10 +14,12 @@ import (
 	appsclient "github.com/openshift/origin/pkg/apps/generated/internalclientset"
 	oauthorizationclient "github.com/openshift/origin/pkg/authorization/generated/internalclientset"
 	imageclient "github.com/openshift/origin/pkg/image/generated/internalclientset"
+	networkclient "github.com/openshift/origin/pkg/network/generated/internalclientset"
 	oauthclient "github.com/openshift/origin/pkg/oauth/generated/internalclientset"
 	clustdiags "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/cluster"
 	agldiags "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging"
 	appcreate "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/cluster/app_create"
+	networkdiags "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/cluster/network"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types"
 	osclientcmd "github.com/openshift/origin/pkg/oc/cli/util/clientcmd"
 	projectclient "github.com/openshift/origin/pkg/project/generated/internalclientset"
@@ -41,6 +43,7 @@ func availableClusterDiagnostics() types.DiagnosticList {
 		&clustdiags.NodeDefinitions{},
 		&clustdiags.RouteCertificateValidation{},
 		&clustdiags.ServiceExternalIPs{},
+		&networkdiags.NetworkDiagnostic{},
 	}
 }
 
@@ -54,14 +57,14 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf
 
 	var kclusterClient kclientset.Interface
 
-	config, kclusterClient, serverUrl, err := o.findClusterClients(rawConfig)
+	config, kclusterClient, rawAdminConfig, err := o.findClusterClients(rawConfig)
+	if err != nil {
+		return nil, err
+	}
 	if config == nil {
 		o.Logger().Notice("CED1002", "Could not configure a client with cluster-admin permissions for the current server, so cluster diagnostics will be skipped")
 		return nil, nil
 	}
-	if err != nil {
-		return nil, err
-	}
 	imageClient, err := imageclient.NewForConfig(config)
 	if err != nil {
 		return nil, err
@@ -90,6 +93,10 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf
 	if err != nil {
 		return nil, err
 	}
+	networkClient, err := networkclient.NewForConfig(config)
+	if err != nil {
+		return nil, err
+	}
 
 	diagnostics := []types.Diagnostic{}
 	for _, diagnosticName := range requestedDiagnostics {
@@ -111,6 +118,7 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf
 		case clustdiags.NodeDefinitionsName:
 			d = &clustdiags.NodeDefinitions{KubeClient: kclusterClient}
 		case clustdiags.MasterNodeName:
+			serverUrl := rawAdminConfig.Clusters[rawAdminConfig.Contexts[rawAdminConfig.CurrentContext].Cluster].Server
 			d = &clustdiags.MasterNode{KubeClient: kclusterClient, ServerUrl: serverUrl, MasterConfigFile: o.MasterConfigLocation}
 		case clustdiags.ClusterRegistryName:
 			d = &clustdiags.ClusterRegistry{KubeClient: kclusterClient, ImageStreamClient: imageClient.Image(), PreventModification: o.PreventModification}
@@ -126,6 +134,17 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf
 			d = &clustdiags.ServiceExternalIPs{MasterConfigFile: o.MasterConfigLocation, KclusterClient: kclusterClient}
 		case clustdiags.RouteCertificateValidationName:
 			d = &clustdiags.RouteCertificateValidation{SARClient: kclusterClient.Authorization(), RESTConfig: config}
+		case networkdiags.NetworkDiagnosticName:
+			nd := o.ParameterizedDiagnostics[diagnosticName].(*networkdiags.NetworkDiagnostic)
+			nd.KubeClient = kclusterClient
+			nd.NetNamespacesClient = networkClient.Network()
+			nd.ClusterNetworkClient = networkClient.Network()
+			nd.ClientFlags = o.ClientFlags
+			nd.Level = o.LogOptions.Level
+			nd.Factory = o.Factory
+			nd.RawConfig = rawAdminConfig
+			nd.PreventModification = o.PreventModification
+			d = nd
 		default:
 			return nil, fmt.Errorf("unknown diagnostic: %v", diagnosticName)
 		}
@@ -135,84 +154,96 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf
 }
 
 // attempts to find which context in the config might be a cluster-admin for the server in the current context.
-// returns config for the context chosen, kclusterClient for same, serverUrl of same, and any fatal error
-func (o DiagnosticsOptions) findClusterClients(rawConfig *clientcmdapi.Config) (*rest.Config, kclientset.Interface, string, error) {
+// returns openshift client config for the context chosen, kclusterClient and raw config of same, and any fatal error
+func (o DiagnosticsOptions) findClusterClients(rawConfig *clientcmdapi.Config) (*rest.Config, kclientset.Interface, *clientcmdapi.Config, error) {
 	if o.ClientClusterContext != "" { // user has specified cluster context to use
 		context, exists := rawConfig.Contexts[o.ClientClusterContext]
 		if !exists {
 			configErr := fmt.Errorf("Specified '%s' as cluster-admin context, but it was not found in your client configuration.", o.ClientClusterContext)
 			o.Logger().Error("CED1003", configErr.Error())
-			return nil, nil, "", configErr
+			return nil, nil, nil, configErr
 		}
-		config, kube, serverUrl, err := o.makeClusterClients(rawConfig, o.ClientClusterContext, context)
-		if err != nil || config == nil {
-			return nil, nil, "", err
-		}
-		return config, kube, serverUrl, nil
+		return o.makeClusterClients(rawConfig, o.ClientClusterContext, context)
 	}
 	currentContext, exists := rawConfig.Contexts[rawConfig.CurrentContext]
 	if !exists { // config specified cluster admin context that doesn't exist; complain and quit
 		configErr := fmt.Errorf("Current context '%s' not found in client configuration; will not attempt cluster diagnostics.", rawConfig.CurrentContext)
 		o.Logger().Error("CED1004", configErr.Error())
-		return nil, nil, "", configErr
+		return nil, nil, nil, configErr
 	}
+
 	// check if current context is already cluster admin
-	config, kube, serverUrl, err := o.makeClusterClients(rawConfig, rawConfig.CurrentContext, currentContext)
+	config, kube, rawAdminConfig, err := o.makeClusterClients(rawConfig, rawConfig.CurrentContext, currentContext)
 	if err == nil && config != nil {
-		return config, kube, serverUrl, nil
+		return config, kube, rawAdminConfig, nil
 	}
+
 	// otherwise, for convenience, search for a context with the same server but with the system:admin user
 	for name, context := range rawConfig.Contexts {
 		if context.Cluster == currentContext.Cluster && name != rawConfig.CurrentContext && strings.HasPrefix(context.AuthInfo, "system:admin/") {
-			config, kube, serverUrl, err := o.makeClusterClients(rawConfig, name, context)
+			config, kube, rawAdminConfig, err := o.makeClusterClients(rawConfig, name, context)
 			if err != nil || config == nil {
 				break // don't try more than one such context, they'll probably fail the same
 			}
-			return config, kube, serverUrl, nil
+			return config, kube, rawAdminConfig, nil
 		}
 	}
-	return nil, nil, "", nil
+	return nil, nil, nil, nil
 }
 
 // makes the client from the specified context and determines whether it is a cluster-admin.
-func (o DiagnosticsOptions) makeClusterClients(rawConfig *clientcmdapi.Config, contextName string, context *clientcmdapi.Context) (*rest.Config, kclientset.Interface, string, error) {
+func (o DiagnosticsOptions) makeClusterClients(rawConfig *clientcmdapi.Config, contextName string, context *clientcmdapi.Context) (*rest.Config, kclientset.Interface, *clientcmdapi.Config, error) {
 	overrides := &clientcmd.ConfigOverrides{Context: *context}
 	clientConfig := clientcmd.NewDefaultClientConfig(*rawConfig, overrides)
-	serverUrl := rawConfig.Clusters[context.Cluster].Server
 	factory := osclientcmd.NewFactory(clientConfig)
+
+	// create a config for making openshift clients
 	config, err := factory.ClientConfig()
 	if err != nil {
-		o.Logger().Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err))
-		return nil, nil, "", nil
+		o.Logger().Debug("CED1006", fmt.Sprintf("Error creating client config for context '%s':\n%v", contextName, err))
+		return nil, nil, nil, nil
 	}
+
+	// create a kube client
+	kubeClient, err := factory.ClientSet()
+	if err != nil {
+		o.Logger().Debug("CED1006", fmt.Sprintf("Error creating kube client for context '%s':\n%v", contextName, err))
+		return nil, nil, nil, nil
+	}
+
 	o.Logger().Debug("CED1005", fmt.Sprintf("Checking if context is cluster-admin: '%s'", contextName))
-	if kubeClient, err := factory.ClientSet(); err != nil {
-		o.Logger().Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err))
-		return nil, nil, "", nil
-	} else {
-		subjectAccessReview := &authorization.SelfSubjectAccessReview{
-			Spec: authorization.SelfSubjectAccessReviewSpec{
-				ResourceAttributes: &authorization.ResourceAttributes{
-					// if you can do everything, you're the cluster admin.
-					Verb:     "*",
-					Group:    "*",
-					Resource: "*",
-				},
+	subjectAccessReview := &authorization.SelfSubjectAccessReview{
+		Spec: authorization.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &authorization.ResourceAttributes{
+				// if you can do everything, you're the cluster admin.
+				Verb:     "*",
+				Group:    "*",
+				Resource: "*",
 			},
-		}
-		if resp, err := kubeClient.Authorization().SelfSubjectAccessReviews().Create(subjectAccessReview); err != nil {
-			if regexp.MustCompile(`User "[\w:]+" cannot create \w+ at the cluster scope`).MatchString(err.Error()) {
-				o.Logger().Debug("CED1007", fmt.Sprintf("Context '%s' does not have cluster-admin access:\n%v", contextName, err))
-				return nil, nil, "", nil
-			} else {
-				o.Logger().Error("CED1008", fmt.Sprintf("Unknown error testing cluster-admin access for context '%s':\n%v", contextName, err))
-				return nil, nil, "", err
-			}
-		} else if resp.Status.Allowed {
-			o.Logger().Info("CED1009", fmt.Sprintf("Using context for cluster-admin access: '%s'", contextName))
-			return config, kubeClient, serverUrl, nil
-		}
+		},
+	}
+	resp, err := kubeClient.Authorization().SelfSubjectAccessReviews().Create(subjectAccessReview)
+	if err != nil && regexp.MustCompile(`User "[\w:]+" cannot create \w+ at the cluster scope`).MatchString(err.Error()) {
+		o.Logger().Debug("CED1007", fmt.Sprintf("Context '%s' does not have cluster-admin access:\n%v", contextName, err))
+		return nil, nil, nil, nil
+	}
+	if err != nil {
+		o.Logger().Error("CED1008", fmt.Sprintf("Unknown error testing cluster-admin access for context '%s':\n%v", contextName, err))
+		return nil, nil, nil, err
+	}
+	if !resp.Status.Allowed {
+		o.Logger().Debug("CED1010", fmt.Sprintf("Context does not have cluster-admin access: '%s'", contextName))
+		return nil, nil, nil, nil
+	}
+
+	o.Logger().Info("CED1009", fmt.Sprintf("Using context for cluster-admin access: '%s'", contextName))
+	adminConfig := rawConfig.DeepCopy()
+	adminConfig.CurrentContext = contextName
+	if err := clientcmdapi.MinifyConfig(adminConfig); err != nil {
+		return nil, nil, nil, err
+	}
+	if err := clientcmdapi.FlattenConfig(adminConfig); err != nil {
+		return nil, nil, nil, err
 	}
-	o.Logger().Debug("CED1010", fmt.Sprintf("Context does not have cluster-admin access: '%s'", contextName))
-	return nil, nil, "", nil
+	return config, kubeClient, adminConfig, nil
 }

pkg/oc/admin/diagnostics/diagnostics.go

@@ -13,13 +13,12 @@ import (
 	kutilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
-	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 
 	"github.com/openshift/origin/pkg/client/config"
 	"github.com/openshift/origin/pkg/cmd/flagtypes"
-	clientdiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/client"
+	poddiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/client/pod/in_pod"
+	networkpoddiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/cluster/network/in_pod"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/log"
-	networkdiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/network"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/options"
 	"github.com/openshift/origin/pkg/oc/admin/diagnostics/util"
@@ -120,7 +119,7 @@ func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Comma
 		Use:   name,
 		Short: "Diagnose common cluster problems",
 		Long:  fmt.Sprintf(longDescription, fullName),
-		Run:   commandRunFunc(o),
+		Run:   util.CommandRunFunc(o),
 	}
 	cmd.SetOutput(out) // for output re: usage / help
 	o.bindCommonFlags(cmd.Flags())
@@ -135,8 +134,8 @@ func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Comma
 	}
 	// add hidden in-pod subcommands
 	cmd.AddCommand(
-		NewCommandPodDiagnostics(clientdiag.InPodDiagnosticRecommendedName, out),
-		NewCommandNetworkPodDiagnostics(networkdiag.InPodNetworkCheckRecommendedName, out),
+		poddiag.NewCommandPodDiagnostics(poddiag.InPodDiagnosticRecommendedName, out),
+		networkpoddiag.NewCommandNetworkPodDiagnostics(networkpoddiag.InPodNetworkCheckRecommendedName, out),
 	)
 
 	return cmd
@@ -154,7 +153,7 @@ func NewCmdDiagnosticsAll(name string, fullName string, out io.Writer, available
 		Use:   name,
 		Short: "Diagnose common cluster problems",
 		Long:  fmt.Sprintf(longDescriptionAll, fullName),
-		Run:   commandRunFunc(o),
+		Run:   util.CommandRunFunc(o),
 	}
 	cmd.SetOutput(out) // for output re: usage / help
 	o.bindCommonFlags(cmd.Flags())
@@ -176,7 +175,7 @@ func NewCmdDiagnosticsIndividual(name string, fullName string, out io.Writer, di
 		Use:     name,
 		Short:   diagnostic.Description(),
 		Long:    fmt.Sprintf(longDescriptionIndividual, diagnostic.Name(), diagnostic.Description()),
-		Run:     commandRunFunc(o),
+		Run:     util.CommandRunFunc(o),
 		Aliases: []string{diagnostic.Name()},
 	}
 	cmd.SetOutput(out) // for output re: usage / help
@@ -194,27 +193,6 @@ func NewCmdDiagnosticsIndividual(name string, fullName string, out io.Writer, di
 	return cmd
 }
 
-type optionsRunner interface {
-	Logger() *log.Logger
-	Complete(*cobra.Command, []string) error
-	RunDiagnostics() error
-}
-
-// returns a shared function that runs when one of these Commands actually executes
-func commandRunFunc(o optionsRunner) func(c *cobra.Command, args []string) {
-	return func(c *cobra.Command, args []string) {
-		kcmdutil.CheckErr(o.Complete(c, args))
-
-		if err := o.RunDiagnostics(); err != nil {
-			o.Logger().Error("CED3001", fmt.Sprintf("Encountered fatal error while building diagnostics: %s", err.Error()))
-		}
-		o.Logger().Summary()
-		if o.Logger().ErrorsSeen() {
-			os.Exit(1)
-		}
-	}
-}
-
 // returns the logger built according to options (must be Complete()ed)
 func (o *DiagnosticsOptions) Logger() *log.Logger {
 	return o.logger

pkg/oc/admin/diagnostics/diagnostics/pod/auth.go pkg/oc/admin/diagnostics/diagnostics/client/pod/in_pod/auth.go

@@ -1,4 +1,4 @@
-package pod
+package in_pod
 
 import (
 	"crypto/tls"