address review comments

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

Author: inteon

pkg/datagatherer/oidc/oidc.go

@@ -17,7 +17,7 @@ import (
 	"github.com/jetstack/preflight/pkg/kubeconfig"
 )
 
-// OIDCDiscovery contains the configuration for the k8s-discovery data-gatherer
+// OIDCDiscovery contains the configuration for the oidc data-gatherer.
 type OIDCDiscovery struct {
 	// KubeConfigPath is the path to the kubeconfig file. If empty, will assume it runs in-cluster.
 	KubeConfigPath string `yaml:"kubeconfig"`
@@ -49,7 +49,7 @@ func (c *OIDCDiscovery) NewDataGatherer(ctx context.Context) (datagatherer.DataG
 	}, nil
 }
 
-// DataGathererOIDC stores the config for a k8s-discovery datagatherer
+// DataGathererOIDC stores the config for an oidc datagatherer.
 type DataGathererOIDC struct {
 	cl rest.Interface
 }
@@ -79,6 +79,13 @@ func (g *DataGathererOIDC) Fetch() (any, int, error) {
 		return ""
 	}
 
+	if oidcErr != nil {
+		klog.FromContext(ctx).V(4).Error(oidcErr, "Failed to fetch OIDC configuration")
+	}
+	if jwksErr != nil {
+		klog.FromContext(ctx).V(4).Error(jwksErr, "Failed to fetch JWKS")
+	}
+
 	return &api.OIDCDiscoveryData{
 		OIDCConfig:      oidcResponse,
 		OIDCConfigError: errToString(oidcErr),
@@ -97,7 +104,7 @@ func (g *DataGathererOIDC) fetchOIDCConfig(ctx context.Context) (map[string]any,
 	bytes, _ := result.Raw() // we already checked result.Error(), so there is no error here
 	var oidcResponse map[string]any
 	if err := json.Unmarshal(bytes, &oidcResponse); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal OIDC discovery document: %v", err)
+		return nil, fmt.Errorf("failed to unmarshal OIDC discovery document: %v (raw: %q)", err, stringFirstN(string(bytes), 80))
 	}
 
 	return oidcResponse, nil
@@ -118,12 +125,19 @@ func (g *DataGathererOIDC) fetchJWKS(ctx context.Context) (map[string]any, error
 	bytes, _ := result.Raw() // we already checked result.Error(), so there is no error here
 	var jwksResponse map[string]any
 	if err := json.Unmarshal(bytes, &jwksResponse); err != nil {
-		return nil, fmt.Errorf("failed to unmarshal JWKS response: %v", err)
+		return nil, fmt.Errorf("failed to unmarshal JWKS response: %v (raw: %q)", err, stringFirstN(string(bytes), 80))
 	}
 
 	return jwksResponse, nil
 }
 
+func stringFirstN(s string, n int) string {
+	if len(s) <= n {
+		return s
+	}
+	return s[:n]
+}
+
 // based on https://github.com/kubernetes/kubectl/blob/a64ceaeab69eed1f11a9e1bd91cf2c1446de811c/pkg/cmd/util/helpers.go#L244
 func k8sErrorMessage(err error) string {
 	if status, isStatus := err.(apierrors.APIStatus); isStatus {
@@ -142,7 +156,6 @@ func k8sErrorMessage(err error) string {
 	}
 
 	if t, isURL := err.(*url.Error); isURL {
-		klog.V(4).Infof("Connection error: %s %s: %v", t.Op, t.URL, t.Err)
 		if strings.Contains(t.Err.Error(), "connection refused") {
 			host := t.URL
 			if server, err := url.Parse(t.URL); err == nil {

pkg/datagatherer/oidc/oidc_test.go

@@ -1,16 +1,17 @@
 package oidc
 
 import (
+	"bytes"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
 
 	"github.com/jetstack/preflight/api"
-	"github.com/stretchr/testify/require"
 )
 
 func makeRESTClient(t *testing.T, ts *httptest.Server) rest.Interface {
@@ -94,13 +95,14 @@ func TestFetch_Errors(t *testing.T) {
 			},
 			jwksResponse: func(w http.ResponseWriter, r *http.Request) {
 				w.Header().Set("Content-Type", "application/json")
-				_, _ = w.Write([]byte(`}0`))
+				_, _ = w.Write([]byte(`}`))
+				_, _ = w.Write(bytes.Repeat([]byte{'0'}, 5000))
 			},
-			expOIDCConfigError: "failed to unmarshal OIDC discovery document: invalid character '}' looking for beginning of value",
-			expJWKSError:       "failed to unmarshal JWKS response: invalid character '}' looking for beginning of value",
+			expOIDCConfigError: `failed to unmarshal OIDC discovery document: invalid character '}' looking for beginning of value (raw: "}{")`,
+			expJWKSError:       `failed to unmarshal JWKS response: invalid character '}' looking for beginning of value (raw: "}0000000000000000000000000000000000000000000000000000000000000000000000000000000")`,
 		},
 		{
-			name: "permission error (no body)",
+			name: "Forbidden error (no body)",
 			openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
 				http.Error(w, "forbidden", http.StatusForbidden)
 			},
@@ -111,7 +113,7 @@ func TestFetch_Errors(t *testing.T) {
 			expJWKSError:       "failed to get /openid/v1/jwks: Error from server (Forbidden): forbidden",
 		},
 		{
-			name: "permission error (*metav1.Status body)",
+			name: "Forbidden error (*metav1.Status body)",
 			openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
 				w.Header().Set("Content-Type", "application/json")
 				w.WriteHeader(http.StatusForbidden)
@@ -143,6 +145,37 @@ func TestFetch_Errors(t *testing.T) {
 			expOIDCConfigError: `failed to get /.well-known/openid-configuration: Error from server (Forbidden): forbidden: User "system:serviceaccount:default:test" cannot get path "/.well-known/openid-configuration"`,
 			expJWKSError:       `failed to get /openid/v1/jwks: Error from server (Forbidden): forbidden: User "system:serviceaccount:default:test" cannot get path "/openid/v1/jwks"`,
 		},
+		{
+			name: "Unauthorized error (*metav1.Status body)",
+			openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusForbidden)
+				_, _ = w.Write([]byte(`{
+					"kind": "Status",
+					"apiVersion": "v1",
+					"metadata": {},
+					"status": "Failure",
+					"message": "Unauthorized",
+					"reason": "Unauthorized",
+					"code": 401
+				}`))
+			},
+			jwksResponse: func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusForbidden)
+				_, _ = w.Write([]byte(`{
+					"kind": "Status",
+					"apiVersion": "v1",
+					"metadata": {},
+					"status": "Failure",
+					"message": "Unauthorized",
+					"reason": "Unauthorized",
+					"code": 401
+				}`))
+			},
+			expOIDCConfigError: `failed to get /.well-known/openid-configuration: error: You must be logged in to the server (Unauthorized)`,
+			expJWKSError:       `failed to get /openid/v1/jwks: error: You must be logged in to the server (Unauthorized)`,
+		},
 	}
 
 	for _, tc := range tests {