Skip to content

Commit f333965

Browse files
authored
[extension/oidcauth] move validation logic to the configuration (#30460)
**Description:** Move validation logic from the component creation to config validation.
1 parent ec822df commit f333965

File tree

5 files changed

+51
-30
lines changed

5 files changed

+51
-30
lines changed
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: enhancement
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: oidcauthextension
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Move validation logic outside of the extension creation, to the configuration validation
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [30460]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# If your change doesn't affect end users or the exported elements of any package,
21+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []

extension/oidcauthextension/config.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,13 @@ type Config struct {
3030
// Optional.
3131
GroupsClaim string `mapstructure:"groups_claim"`
3232
}
33+
34+
func (c *Config) Validate() error {
35+
if c.Audience == "" {
36+
return errNoAudienceProvided
37+
}
38+
if c.IssuerURL == "" {
39+
return errNoIssuerURL
40+
}
41+
return nil
42+
}

extension/oidcauthextension/extension.go

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,7 @@ var (
4444
errNotAuthenticated = errors.New("authentication didn't succeed")
4545
)
4646

47-
func newExtension(cfg *Config, logger *zap.Logger) (auth.Server, error) {
48-
if cfg.Audience == "" {
49-
return nil, errNoAudienceProvided
50-
}
51-
if cfg.IssuerURL == "" {
52-
return nil, errNoIssuerURL
53-
}
54-
47+
func newExtension(cfg *Config, logger *zap.Logger) auth.Server {
5548
if cfg.Attribute == "" {
5649
cfg.Attribute = defaultAttribute
5750
}
@@ -60,7 +53,7 @@ func newExtension(cfg *Config, logger *zap.Logger) (auth.Server, error) {
6053
cfg: cfg,
6154
logger: logger,
6255
}
63-
return auth.NewServer(auth.WithServerStart(oe.start), auth.WithServerAuthenticate(oe.authenticate)), nil
56+
return auth.NewServer(auth.WithServerStart(oe.start), auth.WithServerAuthenticate(oe.authenticate))
6457
}
6558

6659
func (e *oidcExtension) start(context.Context, component.Host) error {

extension/oidcauthextension/extension_test.go

Lines changed: 11 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@ func TestOIDCAuthenticationSucceeded(t *testing.T) {
3636
Audience: "unit-test",
3737
GroupsClaim: "memberships",
3838
}
39-
p, err := newExtension(config, zap.NewNop())
40-
require.NoError(t, err)
39+
p := newExtension(config, zap.NewNop())
4140

4241
err = p.Start(context.Background(), componenttest.NewNopHost())
4342
require.NoError(t, err)
@@ -195,11 +194,10 @@ func TestOIDCFailedToLoadIssuerCAFromPathInvalidContent(t *testing.T) {
195194

196195
func TestOIDCInvalidAuthHeader(t *testing.T) {
197196
// prepare
198-
p, err := newExtension(&Config{
197+
p := newExtension(&Config{
199198
Audience: "some-audience",
200199
IssuerURL: "http://example.com",
201200
}, zap.NewNop())
202-
require.NoError(t, err)
203201

204202
// test
205203
ctx, err := p.Authenticate(context.Background(), map[string][]string{"authorization": {"some-value"}})
@@ -211,11 +209,10 @@ func TestOIDCInvalidAuthHeader(t *testing.T) {
211209

212210
func TestOIDCNotAuthenticated(t *testing.T) {
213211
// prepare
214-
p, err := newExtension(&Config{
212+
p := newExtension(&Config{
215213
Audience: "some-audience",
216214
IssuerURL: "http://example.com",
217215
}, zap.NewNop())
218-
require.NoError(t, err)
219216

220217
// test
221218
ctx, err := p.Authenticate(context.Background(), make(map[string][]string))
@@ -227,14 +224,13 @@ func TestOIDCNotAuthenticated(t *testing.T) {
227224

228225
func TestProviderNotReacheable(t *testing.T) {
229226
// prepare
230-
p, err := newExtension(&Config{
227+
p := newExtension(&Config{
231228
Audience: "some-audience",
232229
IssuerURL: "http://example.com",
233230
}, zap.NewNop())
234-
require.NoError(t, err)
235231

236232
// test
237-
err = p.Start(context.Background(), componenttest.NewNopHost())
233+
err := p.Start(context.Background(), componenttest.NewNopHost())
238234

239235
// verify
240236
assert.Error(t, err)
@@ -247,11 +243,10 @@ func TestFailedToVerifyToken(t *testing.T) {
247243
oidcServer.Start()
248244
defer oidcServer.Close()
249245

250-
p, err := newExtension(&Config{
246+
p := newExtension(&Config{
251247
IssuerURL: oidcServer.URL,
252248
Audience: "unit-test",
253249
}, zap.NewNop())
254-
require.NoError(t, err)
255250

256251
err = p.Start(context.Background(), componenttest.NewNopHost())
257252
require.NoError(t, err)
@@ -305,8 +300,7 @@ func TestFailedToGetGroupsClaimFromToken(t *testing.T) {
305300
},
306301
} {
307302
t.Run(tt.casename, func(t *testing.T) {
308-
p, err := newExtension(tt.config, zap.NewNop())
309-
require.NoError(t, err)
303+
p := newExtension(tt.config, zap.NewNop())
310304

311305
err = p.Start(context.Background(), componenttest.NewNopHost())
312306
require.NoError(t, err)
@@ -414,10 +408,9 @@ func TestMissingClient(t *testing.T) {
414408
}
415409

416410
// test
417-
p, err := newExtension(config, zap.NewNop())
411+
err := config.Validate()
418412

419413
// verify
420-
assert.Nil(t, p)
421414
assert.Equal(t, errNoAudienceProvided, err)
422415
}
423416

@@ -428,10 +421,9 @@ func TestMissingIssuerURL(t *testing.T) {
428421
}
429422

430423
// test
431-
p, err := newExtension(config, zap.NewNop())
424+
err := config.Validate()
432425

433426
// verify
434-
assert.Nil(t, p)
435427
assert.Equal(t, errNoIssuerURL, err)
436428
}
437429

@@ -441,12 +433,11 @@ func TestShutdown(t *testing.T) {
441433
Audience: "some-audience",
442434
IssuerURL: "http://example.com/",
443435
}
444-
p, err := newExtension(config, zap.NewNop())
445-
require.NoError(t, err)
436+
p := newExtension(config, zap.NewNop())
446437
require.NotNil(t, p)
447438

448439
// test
449-
err = p.Shutdown(context.Background()) // for now, we never fail
440+
err := p.Shutdown(context.Background()) // for now, we never fail
450441

451442
// verify
452443
assert.NoError(t, err)

extension/oidcauthextension/factory.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,5 +33,5 @@ func createDefaultConfig() component.Config {
3333
}
3434

3535
func createExtension(_ context.Context, set extension.CreateSettings, cfg component.Config) (extension.Extension, error) {
36-
return newExtension(cfg.(*Config), set.Logger)
36+
return newExtension(cfg.(*Config), set.Logger), nil
3737
}

0 commit comments

Comments
 (0)