Skip to content

Commit 0f08950

Browse files
inteoninteon
committed
improve k8s error messages
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
1 parent 21b6f27 commit 0f08950

File tree

2 files changed

+167
-66
lines changed

2 files changed

+167
-66
lines changed

‎pkg/datagatherer/oidc/oidc.go‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,13 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"net/url"
8+
"strings"
79

10+
apierrors "k8s.io/apimachinery/pkg/api/errors"
11+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
812
"k8s.io/client-go/rest"
13+
"k8s.io/klog/v2"
914

1015
"github.com/jetstack/preflight/api"
1116
"github.com/jetstack/preflight/pkg/datagatherer"
@@ -84,11 +89,12 @@ func (g *DataGathererOIDC) Fetch() (any, int, error) {
8489

8590
func (g *DataGathererOIDC) fetchOIDCConfig(ctx context.Context) (map[string]any, error) {
8691
// Fetch the OIDC discovery document from the well-known endpoint.
87-
bytes, err := g.cl.Get().AbsPath("/.well-known/openid-configuration").Do(ctx).Raw()
88-
if err != nil {
89-
return nil, fmt.Errorf("failed to get OIDC discovery document: %v", err)
92+
result := g.cl.Get().AbsPath("/.well-known/openid-configuration").Do(ctx)
93+
if err := result.Error(); err != nil {
94+
return nil, fmt.Errorf("failed to get /.well-known/openid-configuration: %s", k8sErrorMessage(err))
9095
}
9196

97+
bytes, _ := result.Raw()
9298
var oidcResponse map[string]any
9399
if err := json.Unmarshal(bytes, &oidcResponse); err != nil {
94100
return nil, fmt.Errorf("failed to unmarshal OIDC discovery document: %v", err)
@@ -104,15 +110,48 @@ func (g *DataGathererOIDC) fetchJWKS(ctx context.Context) (map[string]any, error
104110
// - on fully private AWS EKS clusters, the URL is still public and might not
105111
// be reachable from within the cluster (https://github.com/aws/containers-roadmap/issues/2038)
106112
// So we are using the default path instead, which we think should work in most cases.
107-
bytes, err := g.cl.Get().AbsPath("/openid/v1/jwks").Do(ctx).Raw()
108-
if err != nil {
109-
return nil, fmt.Errorf("failed to get JWKS from jwks_uri: %v", err)
113+
result := g.cl.Get().AbsPath("/openid/v1/jwks").Do(ctx)
114+
if err := result.Error(); err != nil {
115+
return nil, fmt.Errorf("failed to get /openid/v1/jwks: %s", k8sErrorMessage(err))
110116
}
111117

118+
bytes, _ := result.Raw()
112119
var jwksResponse map[string]any
113120
if err := json.Unmarshal(bytes, &jwksResponse); err != nil {
114121
return nil, fmt.Errorf("failed to unmarshal JWKS response: %v", err)
115122
}
116123

117124
return jwksResponse, nil
118125
}
126+
127+
// based on https://github.com/kubernetes/kubectl/blob/a64ceaeab69eed1f11a9e1bd91cf2c1446de811c/pkg/cmd/util/helpers.go#L244
128+
func k8sErrorMessage(err error) string {
129+
if status, isStatus := err.(apierrors.APIStatus); isStatus {
130+
switch s := status.Status(); {
131+
case s.Reason == metav1.StatusReasonUnauthorized:
132+
return fmt.Sprintf("error: You must be logged in to the server (%s)", s.Message)
133+
case len(s.Reason) > 0:
134+
return fmt.Sprintf("Error from server (%s): %s", s.Reason, err.Error())
135+
default:
136+
return fmt.Sprintf("Error from server: %s", err.Error())
137+
}
138+
}
139+
140+
if apierrors.IsUnexpectedObjectError(err) {
141+
return fmt.Sprintf("Server returned an unexpected response: %s", err.Error())
142+
}
143+
144+
if t, isURL := err.(*url.Error); isURL {
145+
klog.V(4).Infof("Connection error: %s %s: %v", t.Op, t.URL, t.Err)
146+
if strings.Contains(t.Err.Error(), "connection refused") {
147+
host := t.URL
148+
if server, err := url.Parse(t.URL); err == nil {
149+
host = server.Host
150+
}
151+
return fmt.Sprintf("The connection to the server %s was refused - did you specify the right host or port?", host)
152+
}
153+
return fmt.Sprintf("Unable to connect to the server: %v", t.Err)
154+
}
155+
156+
return fmt.Sprintf("error: %v", err)
157+
}

‎pkg/datagatherer/oidc/oidc_test.go‎

Lines changed: 122 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"k8s.io/client-go/rest"
1111

1212
"github.com/jetstack/preflight/api"
13+
"github.com/stretchr/testify/require"
1314
)
1415

1516
func makeRESTClient(t *testing.T, ts *httptest.Server) rest.Interface {
@@ -50,72 +51,133 @@ func TestFetch_Success(t *testing.T) {
5051
g := &DataGathererOIDC{cl: rc}
5152

5253
anyRes, count, err := g.Fetch()
53-
if err != nil {
54-
t.Fatalf("Fetch returned error: %v", err)
55-
}
56-
if count != 1 {
57-
t.Fatalf("expected count 1, got %d", count)
58-
}
54+
require.NoError(t, err)
55+
require.Equal(t, 1, count)
5956

6057
res, ok := anyRes.(*api.OIDCDiscoveryData)
61-
if !ok {
62-
t.Fatalf("unexpected result type: %T", anyRes)
63-
}
58+
require.True(t, ok, "unexpected result type")
6459

65-
if res.OIDCConfig == nil {
66-
t.Fatalf("expected OIDCConfig, got nil")
67-
}
68-
if iss, _ := res.OIDCConfig["issuer"].(string); iss != "https://example" {
69-
t.Fatalf("unexpected issuer: %v", res.OIDCConfig["issuer"])
70-
}
60+
require.NotNil(t, res.OIDCConfig)
61+
require.Equal(t, "https://example", res.OIDCConfig["issuer"].(string))
62+
require.Empty(t, res.OIDCConfigError)
7163

72-
if res.JWKS == nil {
73-
t.Fatalf("expected JWKS, got nil")
74-
}
75-
if _, ok := res.JWKS["keys"].([]any); !ok {
76-
t.Fatalf("expected keys to be a slice, got %#v", res.JWKS["keys"])
77-
}
64+
require.NotNil(t, res.JWKS)
65+
_, ok = res.JWKS["keys"].([]any)
66+
require.True(t, ok, "unexpected result type")
67+
require.Empty(t, res.JWKSError)
7868
}
7969

8070
func TestFetch_Errors(t *testing.T) {
81-
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
82-
switch r.URL.Path {
83-
case "/.well-known/openid-configuration":
84-
// return server error
85-
http.Error(w, "boom", http.StatusInternalServerError)
86-
case "/openid/v1/jwks":
87-
// return invalid JSON
88-
w.Header().Set("Content-Type", "application/json")
89-
_, _ = w.Write([]byte(`}{`))
90-
default:
91-
http.NotFound(w, r)
92-
}
93-
}))
94-
defer ts.Close()
95-
96-
rc := makeRESTClient(t, ts)
97-
g := &DataGathererOIDC{cl: rc}
98-
99-
anyRes, _, err := g.Fetch()
100-
if err != nil {
101-
t.Fatalf("Fetch returned error: %v", err)
102-
}
103-
104-
res, ok := anyRes.(*api.OIDCDiscoveryData)
105-
if !ok {
106-
t.Fatalf("unexpected result type: %T", anyRes)
107-
}
108-
109-
if res.OIDCConfig != nil {
110-
t.Fatalf("expected nil OIDCConfig on error, got %#v", res.OIDCConfig)
111-
}
112-
if res.OIDCConfigError != "failed to get OIDC discovery document: an error on the server (\"boom\") has prevented the request from succeeding" {
113-
t.Fatalf("unexpected OIDCConfigError: %q", res.OIDCConfigError)
114-
}
115-
if res.JWKS != nil {
116-
t.Fatalf("expected nil JWKS on malformed JSON, got %#v", res.JWKS)
117-
}
118-
if res.JWKSError != "failed to unmarshal JWKS response: invalid character '}' looking for beginning of value" {
119-
t.Fatalf("unexpected JWKSError: %q", res.JWKSError)
71+
tests := []struct {
72+
name string
73+
openidConfigurationResponse func(w http.ResponseWriter, r *http.Request)
74+
jwksResponse func(w http.ResponseWriter, r *http.Request)
75+
expOIDCConfigError string
76+
expJWKSError string
77+
}{
78+
{
79+
name: "5xx errors",
80+
openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
81+
http.Error(w, "boom", http.StatusInternalServerError)
82+
},
83+
jwksResponse: func(w http.ResponseWriter, r *http.Request) {
84+
http.Error(w, "boom", http.StatusInternalServerError)
85+
},
86+
expOIDCConfigError: `failed to get /.well-known/openid-configuration: Error from server (InternalError): an error on the server ("boom") has prevented the request from succeeding`,
87+
expJWKSError: `failed to get /openid/v1/jwks: Error from server (InternalError): an error on the server ("boom") has prevented the request from succeeding`,
88+
},
89+
{
90+
name: "malformed JSON",
91+
openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
92+
w.Header().Set("Content-Type", "application/json")
93+
_, _ = w.Write([]byte(`}{`))
94+
},
95+
jwksResponse: func(w http.ResponseWriter, r *http.Request) {
96+
w.Header().Set("Content-Type", "application/json")
97+
_, _ = w.Write([]byte(`}0`))
98+
},
99+
expOIDCConfigError: "failed to unmarshal OIDC discovery document: invalid character '}' looking for beginning of value",
100+
expJWKSError: "failed to unmarshal JWKS response: invalid character '}' looking for beginning of value",
101+
},
102+
{
103+
name: "permission error (no body)",
104+
openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
105+
http.Error(w, "forbidden", http.StatusForbidden)
106+
},
107+
jwksResponse: func(w http.ResponseWriter, r *http.Request) {
108+
http.Error(w, "forbidden", http.StatusForbidden)
109+
},
110+
expOIDCConfigError: "failed to get /.well-known/openid-configuration: Error from server (Forbidden): forbidden",
111+
expJWKSError: "failed to get /openid/v1/jwks: Error from server (Forbidden): forbidden",
112+
},
113+
{
114+
name: "permission error (*metav1.Status body)",
115+
openidConfigurationResponse: func(w http.ResponseWriter, r *http.Request) {
116+
w.Header().Set("Content-Type", "application/json")
117+
w.WriteHeader(http.StatusForbidden)
118+
_, _ = w.Write([]byte(`{
119+
"kind":"Status",
120+
"apiVersion":"v1",
121+
"metadata":{},
122+
"status":"Failure",
123+
"message":"forbidden: User \"system:serviceaccount:default:test\" cannot get path \"/.well-known/openid-configuration\"",
124+
"reason":"Forbidden",
125+
"details":{},
126+
"code":403
127+
}`))
128+
},
129+
jwksResponse: func(w http.ResponseWriter, r *http.Request) {
130+
w.Header().Set("Content-Type", "application/json")
131+
w.WriteHeader(http.StatusForbidden)
132+
_, _ = w.Write([]byte(`{
133+
"kind":"Status",
134+
"apiVersion":"v1",
135+
"metadata":{},
136+
"status":"Failure",
137+
"message":"forbidden: User \"system:serviceaccount:default:test\" cannot get path \"/openid/v1/jwks\"",
138+
"reason":"Forbidden",
139+
"details":{},
140+
"code":403
141+
}`))
142+
},
143+
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"`,
144+
expJWKSError: `failed to get /openid/v1/jwks: Error from server (Forbidden): forbidden: User "system:serviceaccount:default:test" cannot get path "/openid/v1/jwks"`,
145+
},
146+
}
147+
148+
for _, tc := range tests {
149+
t.Run(tc.name, func(t *testing.T) {
150+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
151+
switch r.URL.Path {
152+
case "/.well-known/openid-configuration":
153+
tc.openidConfigurationResponse(w, r)
154+
return
155+
case "/openid/v1/jwks":
156+
tc.jwksResponse(w, r)
157+
return
158+
default:
159+
t.Fatalf("unexpected request path: %s", r.URL.Path)
160+
}
161+
}))
162+
defer ts.Close()
163+
164+
rc := makeRESTClient(t, ts)
165+
g := &DataGathererOIDC{cl: rc}
166+
167+
anyRes, count, err := g.Fetch()
168+
require.NoError(t, err)
169+
require.Equal(t, 1, count)
170+
171+
res, ok := anyRes.(*api.OIDCDiscoveryData)
172+
require.True(t, ok, "unexpected result type")
173+
174+
require.Nil(t, res.OIDCConfig)
175+
require.NotEmpty(t, res.OIDCConfigError)
176+
require.Equal(t, tc.expOIDCConfigError, res.OIDCConfigError)
177+
178+
require.Nil(t, res.JWKS)
179+
require.NotEmpty(t, res.JWKSError)
180+
require.Equal(t, tc.expJWKSError, res.JWKSError)
181+
})
120182
}
121183
}

0 commit comments

Comments
 (0)