Skip to content

Commit cdd571f

Browse files
c4rloc4rlo
authored
Error handling fixes & add nilerr linter (#6776)
* golangci: add nilerr We already had nilnesserr, but they are separate implementations and nilerr has open PR gostaticanalysis/nilerr#23 which would have caught a bug in our code. Signed-off-by: Carlo Teubner <cteubner1@bloomberg.net> * agent bootstrap: fix error handling We were failing to propagate an error in one particular case. This kind of thing should be caught by the nilerr and nilnesserr linters, but neither of them does because of the presence of defer in this function. For nilerr, gostaticanalysis/nilerr#23 would fix that. Signed-off-by: Carlo Teubner <cteubner1@bloomberg.net> * oidc-discovery-provider: fix missing error check Signed-off-by: Carlo Teubner <cteubner1@bloomberg.net> --------- Signed-off-by: Carlo Teubner <cteubner1@bloomberg.net>
1 parent 0153ceb commit cdd571f

File tree

5 files changed

+27
-14
lines changed

5 files changed

+27
-14
lines changed

‎.golangci.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ linters:
1414
- mirror
1515
- misspell
1616
- nakedret
17+
- nilerr
1718
- nilnesserr
1819
- nolintlint
1920
- predeclared

‎pkg/agent/agent.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func (a *Agent) Run(ctx context.Context) error {
184184
} else if a.c.RebootstrapMode != RebootstrapNever {
185185
startTime, err := a.c.TrustBundleSources.GetStartTime()
186186
if err != nil {
187-
return nil
187+
return err
188188
}
189189
seconds := time.Since(startTime)
190190
if seconds < a.c.RebootstrapDelay {

‎support/oidc-discovery-provider/handler.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ type Handler struct {
3131
http.Handler
3232
}
3333

34-
func NewHandler(log logrus.FieldLogger, domainPolicy DomainPolicy, source JWKSSource, allowInsecureScheme bool, setKeyUse bool, jwtIssuer *url.URL, jwksURI *url.URL, serverPathPrefix string) *Handler {
34+
func NewHandler(log logrus.FieldLogger, domainPolicy DomainPolicy, source JWKSSource, allowInsecureScheme, setKeyUse bool, jwtIssuer, jwksURI *url.URL, serverPathPrefix string) (*Handler, error) {
3535
if serverPathPrefix == "" {
3636
serverPathPrefix = "/"
3737
}
@@ -49,18 +49,18 @@ func NewHandler(log logrus.FieldLogger, domainPolicy DomainPolicy, source JWKSSo
4949
mux := http.NewServeMux()
5050
wkPath, err := url.JoinPath(serverPathPrefix, "/.well-known/openid-configuration")
5151
if err != nil {
52-
return nil
52+
return nil, err
5353
}
5454
jwksPath, err := url.JoinPath(serverPathPrefix, "/keys")
5555
if err != nil {
56-
return nil
56+
return nil, err
5757
}
5858

5959
mux.Handle(wkPath, handlers.ProxyHeaders(http.HandlerFunc(h.serveWellKnown)))
6060
mux.Handle(jwksPath, http.HandlerFunc(h.serveKeys))
6161

6262
h.Handler = mux
63-
return h
63+
return h, nil
6464
}
6565

6666
func (h *Handler) serveWellKnown(w http.ResponseWriter, r *http.Request) {

‎support/oidc-discovery-provider/handler_test.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,8 @@ func TestHandlerHTTPS(t *testing.T) {
173173
require.NoError(t, err)
174174
w := httptest.NewRecorder()
175175

176-
h := NewHandler(log, domainAllowlist(t, "localhost", "domain.test"), source, false, testCase.setKeyUse, nil, nil, "")
176+
h, err := NewHandler(log, domainAllowlist(t, "localhost", "domain.test"), source, false, testCase.setKeyUse, nil, nil, "")
177+
require.NoError(t, err)
177178
h.ServeHTTP(w, r)
178179

179180
t.Logf("HEADERS: %q", w.Header())
@@ -285,7 +286,8 @@ func TestHandlerHTTPInsecure(t *testing.T) {
285286
require.NoError(t, err)
286287
w := httptest.NewRecorder()
287288

288-
h := NewHandler(log, domainAllowlist(t, "localhost", "domain.test"), source, true, false, nil, nil, "")
289+
h, err := NewHandler(log, domainAllowlist(t, "localhost", "domain.test"), source, true, false, nil, nil, "")
290+
require.NoError(t, err)
289291
h.ServeHTTP(w, r)
290292

291293
t.Logf("HEADERS: %q", w.Header())
@@ -458,7 +460,8 @@ func TestHandlerHTTP(t *testing.T) {
458460
require.NoError(t, err)
459461
w := httptest.NewRecorder()
460462

461-
h := NewHandler(log, domainAllowlist(t, "domain.test", "xn--n38h.test"), source, false, false, nil, nil, "")
463+
h, err := NewHandler(log, domainAllowlist(t, "domain.test", "xn--n38h.test"), source, false, false, nil, nil, "")
464+
require.NoError(t, err)
462465
h.ServeHTTP(w, r)
463466

464467
t.Logf("HEADERS: %q", w.Header())
@@ -569,7 +572,8 @@ func TestHandlerProxied(t *testing.T) {
569572
r.Header.Add("X-Forwarded-Scheme", "https")
570573
r.Header.Add("X-Forwarded-Host", "domain.test")
571574
w := httptest.NewRecorder()
572-
h := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, nil, "")
575+
h, err := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, nil, "")
576+
require.NoError(t, err)
573577
h.ServeHTTP(w, r)
574578
t.Logf("HEADERS: %q", w.Header())
575579
assert.Equal(t, testCase.code, w.Code)
@@ -719,7 +723,8 @@ func TestHandlerJWTIssuer(t *testing.T) {
719723
w := httptest.NewRecorder()
720724

721725
u, _ := url.Parse(testCase.jwtIssuer)
722-
h := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, u, nil, "")
726+
h, err := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, u, nil, "")
727+
require.NoError(t, err)
723728
h.ServeHTTP(w, r)
724729

725730
t.Logf("HEADERS: %q", w.Header())
@@ -781,7 +786,8 @@ func TestHandlerJWTIssuerAndJWKSURI(t *testing.T) {
781786

782787
u, _ := url.Parse(testCase.jwtIssuer)
783788
j, _ := url.Parse(testCase.jwksURI)
784-
h := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, u, j, "")
789+
h, err := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, u, j, "")
790+
require.NoError(t, err)
785791
h.ServeHTTP(w, r)
786792

787793
t.Logf("HEADERS: %q", w.Header())
@@ -932,7 +938,8 @@ func TestHandlerAdvertisedURL(t *testing.T) {
932938
w := httptest.NewRecorder()
933939

934940
u, _ := url.Parse(testCase.jwksURI)
935-
h := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, u, "")
941+
h, err := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, u, "")
942+
require.NoError(t, err)
936943
h.ServeHTTP(w, r)
937944

938945
t.Logf("HEADERS: %q", w.Header())
@@ -1059,7 +1066,8 @@ func TestHandlerPrefix(t *testing.T) {
10591066
r.Header.Add("X-Forwarded-Host", "domain.test")
10601067
w := httptest.NewRecorder()
10611068

1062-
h := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, nil, testCase.serverPathPrefix)
1069+
h, err := NewHandler(log, domainAllowlist(t, "domain.test"), source, false, false, nil, nil, testCase.serverPathPrefix)
1070+
require.NoError(t, err)
10631071
h.ServeHTTP(w, r)
10641072

10651073
t.Logf("HEADERS: %q", w.Header())

‎support/oidc-discovery-provider/main.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,11 @@ func run(configPath string, expandEnv bool) error {
9393
}
9494
}
9595

96-
var handler http.Handler = NewHandler(log, domainPolicy, source, config.AllowInsecureScheme, config.SetKeyUse, jwtIssuer, jwksURI, config.ServerPathPrefix)
96+
var handler http.Handler
97+
handler, err = NewHandler(log, domainPolicy, source, config.AllowInsecureScheme, config.SetKeyUse, jwtIssuer, jwksURI, config.ServerPathPrefix)
98+
if err != nil {
99+
return err
100+
}
97101
if config.LogRequests {
98102
log.Info("Logging all requests")
99103
handler = logHandler(log, handler)

0 commit comments

Comments
 (0)