Skip to content

Commit

Permalink
[extension/oidcauth] move validation logic to the configuration (#30460)
Browse files Browse the repository at this point in the history
**Description:**
Move validation logic from the component creation to config validation.
  • Loading branch information
atoulme authored Jan 12, 2024
1 parent ec822df commit f333965
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 30 deletions.
27 changes: 27 additions & 0 deletions .chloggen/move_validation_to_config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: oidcauthextension

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Move validation logic outside of the extension creation, to the configuration validation

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [30460]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
10 changes: 10 additions & 0 deletions extension/oidcauthextension/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,13 @@ type Config struct {
// Optional.
GroupsClaim string `mapstructure:"groups_claim"`
}

func (c *Config) Validate() error {
if c.Audience == "" {
return errNoAudienceProvided
}
if c.IssuerURL == "" {
return errNoIssuerURL
}
return nil
}
11 changes: 2 additions & 9 deletions extension/oidcauthextension/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,7 @@ var (
errNotAuthenticated = errors.New("authentication didn't succeed")
)

func newExtension(cfg *Config, logger *zap.Logger) (auth.Server, error) {
if cfg.Audience == "" {
return nil, errNoAudienceProvided
}
if cfg.IssuerURL == "" {
return nil, errNoIssuerURL
}

func newExtension(cfg *Config, logger *zap.Logger) auth.Server {
if cfg.Attribute == "" {
cfg.Attribute = defaultAttribute
}
Expand All @@ -60,7 +53,7 @@ func newExtension(cfg *Config, logger *zap.Logger) (auth.Server, error) {
cfg: cfg,
logger: logger,
}
return auth.NewServer(auth.WithServerStart(oe.start), auth.WithServerAuthenticate(oe.authenticate)), nil
return auth.NewServer(auth.WithServerStart(oe.start), auth.WithServerAuthenticate(oe.authenticate))
}

func (e *oidcExtension) start(context.Context, component.Host) error {
Expand Down
31 changes: 11 additions & 20 deletions extension/oidcauthextension/extension_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ func TestOIDCAuthenticationSucceeded(t *testing.T) {
Audience: "unit-test",
GroupsClaim: "memberships",
}
p, err := newExtension(config, zap.NewNop())
require.NoError(t, err)
p := newExtension(config, zap.NewNop())

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

func TestOIDCInvalidAuthHeader(t *testing.T) {
// prepare
p, err := newExtension(&Config{
p := newExtension(&Config{
Audience: "some-audience",
IssuerURL: "http://example.com",
}, zap.NewNop())
require.NoError(t, err)

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

func TestOIDCNotAuthenticated(t *testing.T) {
// prepare
p, err := newExtension(&Config{
p := newExtension(&Config{
Audience: "some-audience",
IssuerURL: "http://example.com",
}, zap.NewNop())
require.NoError(t, err)

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

func TestProviderNotReacheable(t *testing.T) {
// prepare
p, err := newExtension(&Config{
p := newExtension(&Config{
Audience: "some-audience",
IssuerURL: "http://example.com",
}, zap.NewNop())
require.NoError(t, err)

// test
err = p.Start(context.Background(), componenttest.NewNopHost())
err := p.Start(context.Background(), componenttest.NewNopHost())

// verify
assert.Error(t, err)
Expand All @@ -247,11 +243,10 @@ func TestFailedToVerifyToken(t *testing.T) {
oidcServer.Start()
defer oidcServer.Close()

p, err := newExtension(&Config{
p := newExtension(&Config{
IssuerURL: oidcServer.URL,
Audience: "unit-test",
}, zap.NewNop())
require.NoError(t, err)

err = p.Start(context.Background(), componenttest.NewNopHost())
require.NoError(t, err)
Expand Down Expand Up @@ -305,8 +300,7 @@ func TestFailedToGetGroupsClaimFromToken(t *testing.T) {
},
} {
t.Run(tt.casename, func(t *testing.T) {
p, err := newExtension(tt.config, zap.NewNop())
require.NoError(t, err)
p := newExtension(tt.config, zap.NewNop())

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

// test
p, err := newExtension(config, zap.NewNop())
err := config.Validate()

// verify
assert.Nil(t, p)
assert.Equal(t, errNoAudienceProvided, err)
}

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

// test
p, err := newExtension(config, zap.NewNop())
err := config.Validate()

// verify
assert.Nil(t, p)
assert.Equal(t, errNoIssuerURL, err)
}

Expand All @@ -441,12 +433,11 @@ func TestShutdown(t *testing.T) {
Audience: "some-audience",
IssuerURL: "http://example.com/",
}
p, err := newExtension(config, zap.NewNop())
require.NoError(t, err)
p := newExtension(config, zap.NewNop())
require.NotNil(t, p)

// test
err = p.Shutdown(context.Background()) // for now, we never fail
err := p.Shutdown(context.Background()) // for now, we never fail

// verify
assert.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion extension/oidcauthextension/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ func createDefaultConfig() component.Config {
}

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

0 comments on commit f333965

Please sign in to comment.