Skip to content

Commit

Permalink
fixup: return err if failing to compile KeySelector CEL expression
Browse files Browse the repository at this point in the history 
Signed-off-by: KevFan <[email protected]>
  • Loading branch information
@KevFan
KevFan committed Feb 25, 2025
1 parent 01a81eb commit 51a230a
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 19 deletions.
6 changes: 5 additions & 1 deletion controllers/auth_config_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,11 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf
return nil, err
}

translatedIdentity.APIKey = identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, string(identity.ApiKey.KeySelector), authCred, r.Client, ctxWithLogger)
if apiKeyIdentity, err := identity_evaluators.NewApiKeyIdentity(identityCfgName, selector, namespace, string(identity.ApiKey.KeySelector), authCred, r.Client, ctxWithLogger); err != nil {
return nil, err
} else {
translatedIdentity.APIKey = apiKeyIdentity
}

// MTLS
case api.X509ClientCertificateAuthentication:
Expand Down
3 changes: 2 additions & 1 deletion controllers/secret_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,11 @@ func newSecretReconcilerTest(mockCtrl *gomock.Controller, secretLabels map[strin
fakeK8sClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(&secret).Build()

apiKeyLabelSelectors, _ := labels.Parse("target=echo-api")
apiKeyIdentity, _ := identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO())
indexedAuthConfig := &evaluators.AuthConfig{
Labels: map[string]string{"namespace": "authorino", "name": "api-protection"},
IdentityConfigs: []auth.AuthConfigEvaluator{&fakeAPIKeyIdentityConfig{
evaluator: identity_evaluators.NewApiKeyIdentity("api-key", apiKeyLabelSelectors, "", "", auth.NewAuthCredential("", ""), fakeK8sClient, context.TODO()),
evaluator: apiKeyIdentity,
}},
}
indexMock := mock_index.NewMockIndex(mockCtrl)
Expand Down
6 changes: 3 additions & 3 deletions pkg/evaluators/identity/api_key.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type APIKey struct {
k8sClient k8s_client.Reader
}

func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectorExpression string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) *APIKey {
func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespace string, keySelectorExpression string, authCred auth.AuthCredentials, k8sClient k8s_client.Reader, ctx context.Context) (*APIKey, error) {
if keySelectorExpression == "" {
keySelectorExpression = defaultKeySelectorExpression
}
Expand All @@ -51,7 +51,7 @@ func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespac
expr, err := cel.NewKeySelectorExpression(keySelectorExpression)
if err != nil {
logger.Error(err, "failed to create key selector expression")
return nil
return nil, err
}

apiKey := &APIKey{
Expand All @@ -66,7 +66,7 @@ func NewApiKeyIdentity(name string, labelSelectors k8s_labels.Selector, namespac
if err := apiKey.loadSecrets(context.TODO()); err != nil {
logger.Error(err, credentialsFetchingErrorMsg)
}
return apiKey
return apiKey, nil
}

// loadSecrets will load the matching k8s secrets from the cluster to the cache of trusted API keys
Expand Down
42 changes: 28 additions & 14 deletions pkg/evaluators/identity/api_key_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ func TestNewApiKeyIdentityAllNamespaces(t *testing.T) {
defer ctrl.Finish()

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())

assert.NilError(t, err)
assert.Equal(t, apiKey.Name, "jedi")
assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant")
assert.Equal(t, apiKey.Namespace, "")
Expand All @@ -51,8 +52,9 @@ func TestNewApiKeyIdentitySingleNamespace(t *testing.T) {
defer ctrl.Finish()

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "ns1", "", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())

assert.NilError(t, err)
assert.Equal(t, apiKey.Name, "jedi")
assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant")
assert.Equal(t, apiKey.Namespace, "ns1")
Expand All @@ -70,8 +72,9 @@ func TestNewApiKeyIdentityMultipleKeySelectors(t *testing.T) {
defer ctrl.Finish()

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "ns1", "['no_op','api_key_2']", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "ns1", "['no_op','api_key_2']", mock_auth.NewMockAuthCredentials(ctrl), testAPIKeyK8sClient, context.TODO())

assert.NilError(t, err)
assert.Equal(t, apiKey.Name, "jedi")
assert.Equal(t, apiKey.LabelSelectors.String(), "planet=coruscant")
assert.Equal(t, apiKey.Namespace, "ns1")
Expand All @@ -95,7 +98,8 @@ func TestCallSuccess(t *testing.T) {
authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil)

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
assert.NilError(t, err)
auth, err := apiKey.Call(pipelineMock, context.TODO())

assert.NilError(t, err)
Expand All @@ -111,9 +115,10 @@ func TestCallNoApiKeyFail(t *testing.T) {
authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("", fmt.Errorf("something went wrong getting the API Key"))

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
assert.NilError(t, err)

_, err := apiKey.Call(pipelineMock, context.TODO())
_, err = apiKey.Call(pipelineMock, context.TODO())

assert.Error(t, err, "something went wrong getting the API Key")
}
Expand All @@ -127,17 +132,19 @@ func TestCallInvalidApiKeyFail(t *testing.T) {
authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ASithLightSaber", nil)

selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
_, err := apiKey.Call(pipelineMock, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
assert.NilError(t, err)
_, err = apiKey.Call(pipelineMock, context.TODO())

assert.Error(t, err, "the API Key provided is invalid")
}

func TestLoadSecretsSuccess(t *testing.T) {
selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, testAPIKeyK8sClient, nil)
apiKey, err := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, testAPIKeyK8sClient, nil)
assert.NilError(t, err)

err := apiKey.loadSecrets(context.TODO())
err = apiKey.loadSecrets(context.TODO())
assert.NilError(t, err)
assert.Equal(t, len(apiKey.secrets), 2)

Expand All @@ -152,12 +159,19 @@ func TestLoadSecretsSuccess(t *testing.T) {

func TestLoadSecretsFail(t *testing.T) {
selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, &flawedAPIkeyK8sClient{}, context.TODO())
apiKey, err := NewApiKeyIdentity("X-API-KEY", selector, "", "", nil, &flawedAPIkeyK8sClient{}, context.TODO())
assert.NilError(t, err)

err := apiKey.loadSecrets(context.TODO())
err = apiKey.loadSecrets(context.TODO())
assert.Error(t, err, "something terribly wrong happened")
}

func TestCELCompileFail(t *testing.T) {
selector, _ := k8s_labels.Parse("planet=coruscant")
_, err := NewApiKeyIdentity("X-API-KEY", selector, "", "stringLiteral", nil, testAPIKeyK8sClient, context.TODO())
assert.Assert(t, err != nil)
}

func BenchmarkAPIKeyAuthn(b *testing.B) {
ctrl := gomock.NewController(b)
defer ctrl.Finish()
Expand All @@ -166,9 +180,9 @@ func BenchmarkAPIKeyAuthn(b *testing.B) {
authCredMock := mock_auth.NewMockAuthCredentials(ctrl)
authCredMock.EXPECT().GetCredentialsFromReq(gomock.Any()).Return("ObiWanKenobiLightSaber", nil).MinTimes(1)
selector, _ := k8s_labels.Parse("planet=coruscant")
apiKey := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
apiKey, err := NewApiKeyIdentity("jedi", selector, "", "", authCredMock, testAPIKeyK8sClient, context.TODO())
assert.NilError(b, err)

var err error
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, err = apiKey.Call(pipelineMock, context.TODO())
Expand Down

0 comments on commit 51a230a

Please sign in to comment.