feat: implements aws oidc backendSecurityPolicy API #306

aabchoo · 2025-02-06T20:27:13Z

Commit Message

The PR implements the backendSecurityPolicy API for AWS's OIDC credentials.

Related Issues/PRs (if applicable)

Special notes for reviewers (if applicable)

I've tested the implementation with our SSO (oauth2), and was able to query bedrock.

Checklist (you don't need to keep in your PR description):

The PR title follows the same format as the merged PRs.
I have read the CONTRIBUTING.md (for first-time contributors).
I have made sure that make precommit test passes locally.
I have written tests for any new line of code unless there's existing tests that cover the changes.
I have updated the documentation accordingly.
I am sure the coding style is consistent with the rest of the codebase.

Signed-off-by: Aaron Choo <[email protected]>

mathetake · 2025-02-06T20:57:06Z

I think you are missing two things (not serious):

Adds the secret watcher index to make the refresh picked up

ai-gateway/internal/controller/controller.go

Line 185 in 737cbf5

// TODO: OIDC.
Reconcile is only called for the new events, so we need to trigger it for all existing resources somehow. https://github.com/envoyproxy/ai-gateway/compare/foraaron?expand=1 This is my recommendation to do so

otherwise, looking good

aabchoo · 2025-02-06T21:31:15Z

One scenario I realize that needs to be address is:

OIDC credentials or AWS specific fields are updated
Either or both values are cached/not expired.

We may need to cache the oidc credentials and aws specific fields to compare. Else users will have to wait til expiration time (ie 1 hour for AWS or however long for OIDC)

Signed-off-by: Aaron Choo <[email protected]>

mathetake · 2025-02-07T16:36:07Z

internal/controller/backend_security_policy.go

+			for _, backendSecurityPolicy := range backendSecPolicyList.Items {
+				if isBackendSecurityPolicyAuthOIDC(backendSecurityPolicy.Spec) {
+					backendSecurityPolicy.Annotations["refresh"] = refreshTime
+				}
+				err = b.client.Update(ctx, &backendSecurityPolicy)
+				if err != nil {
+					b.logger.Error(err, "failed to trigger refresh for existing backendSecPolicy resource", "name", backendSecurityPolicy.Name)
+				}
+			}


you can use Patch instead of Update for less heavy operation https://github.com/envoyproxy/ai-gateway/blob/d31f4ef633dad79c56f0728a8fa50a54fd7bad0d/internal/controller/sink.go#L454C80-L454C85

Signed-off-by: Aaron Choo <[email protected]>

yuzisun · 2025-02-07T23:19:49Z

internal/controller/backend_security_policy.go

+		var backendSecPolicyList aigv1a1.BackendSecurityPolicyList
+		err = b.client.List(ctx, &backendSecPolicyList)
+		if err != nil {
+			b.logger.Error(err, "failed to trigger refresh for existing backendSecPolicy resources")


Suggested change

b.logger.Error(err, "failed to trigger refresh for existing backendSecPolicy resources")

b.logger.Error(err, "failed to list existing backendSecPolicy resources")

yuzisun · 2025-02-07T23:24:16Z

internal/controller/backend_security_policy.go

+		if err != nil {
+			b.logger.Error(err, "failed to trigger refresh for existing backendSecPolicy resources")
+		} else {
+			for _, backendSecurityPolicy := range backendSecPolicyList.Items {


This looks a bit odd to trigger the reconciliation for all security policies on a single resource reconciliation function. Controller runtime might have a way to resync all the resources periodically.

Controller runtime might have a way to resync all the resources periodically.

ha! if there's a way, then that should be much better

@aabchoo sorry it seems like i was misunderstanding the default behavior

operator-framework/operator-sdk#6035 (comment)

you can delete this all patching stuff!

wait, this is operator-sdk... not sure about controller-runtime (operator-sdk using it under the hood?)

yeah i think this applies to controller-runtime as well, let's delete this startup patching stuff

mathetake · 2025-02-07T23:26:33Z

internal/controller/backend_security_policy.go

+func isBackendSecurityPolicyAuthOIDC(spec aigv1a1.BackendSecurityPolicySpec) bool {
+	if spec.AWSCredentials != nil {
+		return spec.AWSCredentials.OIDCExchangeToken != nil
+	}
+	return false
+}
+
+func getBackendSecurityPolicyAuthOIDC(spec aigv1a1.BackendSecurityPolicySpec) *egv1a1.OIDC {
+	if isBackendSecurityPolicyAuthOIDC(spec) {
+		if spec.AWSCredentials.OIDCExchangeToken != nil {
+			return &spec.AWSCredentials.OIDCExchangeToken.OIDC
+		}
+	}
+	return nil
+}


think carefully.... you won't need these two functions ...

yuzisun · 2025-02-07T23:31:37Z

internal/controller/oauth/client_credentials_provider.go

+)
+
+// ClientCredentialsProvider implements the standard OAuth2 client credentials flow
+type ClientCredentialsProvider struct {


Client credential is a grant type for oauth not provider

yuzisun · 2025-02-07T23:33:36Z

internal/controller/oauth/client_credentials_provider.go

+		return nil, fmt.Errorf("failed to create request: %w", err)
+	}
+
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")


Should we use the oauth client library instead of the raw curl request?

yuzisun · 2025-02-07T23:39:24Z

internal/controller/oauth/oidc_provider.go

+}
+
+// OIDCMetadata represents the OpenID Connect provider metadata
+type OIDCMetadata struct {


We can use this type from go-oidc
https://github.com/coreos/go-oidc/blob/v3/oidc/oidc.go#L181

yuzisun · 2025-02-09T07:42:12Z

internal/controller/oauth/base_provider.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+// BaseProvider implements common OAuth functionality


It is not clear to me for the relationship between BaseProvider and OidcProvider. If it is common OAuth functions it is better to be defined as util functions.

internal/controller/oauth/types.go

internal/controller/oauth/oidc_provider.go

internal/controller/sink.go

yuzisun · 2025-02-09T13:27:30Z

internal/controller/oauth/types.go

+}
+
+// Provider defines the interface for OAuth token providers
+type Provider interface {


@missBerg I think the Provider interface is confusing, from the code we are not actually implementing different oauth2/oidc providers like GitHub, google etc if that was what this is meant. It is implementing the OIDC protocol itself.

Signed-off-by: Dan Sun <[email protected]>

aabchoo added 7 commits February 6, 2025 14:59

oauth providers to access oidc and related functions

21a9757

Signed-off-by: Aaron Choo <[email protected]>

add aws oidc sts fetch

975c038

Signed-off-by: Aaron Choo <[email protected]>

linting

0f40360

Signed-off-by: Aaron Choo <[email protected]>

enable proxy for aws and mounted cm

740b59b

Signed-off-by: Aaron Choo <[email protected]>

update go packages

bc86391

Signed-off-by: Aaron Choo <[email protected]>

Merge remote-tracking branch 'origin/main' into aaron/oidc-revisited

b4bd924

precommit test

cbf234a

Signed-off-by: Aaron Choo <[email protected]>

aabchoo added feature go Pull requests that update Go code labels Feb 6, 2025

remove comment

3739f50

Signed-off-by: Aaron Choo <[email protected]>

aabchoo added 2 commits February 6, 2025 17:39

set up index for aws oidc

b3000aa

Signed-off-by: Aaron Choo <[email protected]>

trigger refresh on controller start up

7d44ec0

Signed-off-by: Aaron Choo <[email protected]>

mathetake reviewed Feb 7, 2025

View reviewed changes

aabchoo added 3 commits February 7, 2025 11:49

backend security policy tests

71f537a

Signed-off-by: Aaron Choo <[email protected]>

patch reconcile annotation

45634e1

Signed-off-by: Aaron Choo <[email protected]>

testing

f6cee6a

Signed-off-by: Aaron Choo <[email protected]>

yuzisun reviewed Feb 7, 2025

View reviewed changes

mathetake reviewed Feb 7, 2025

View reviewed changes

yuzisun reviewed Feb 7, 2025

View reviewed changes

yuzisun reviewed Feb 9, 2025

View reviewed changes

internal/controller/oauth/types.go Outdated Show resolved Hide resolved

internal/controller/oauth/oidc_provider.go Outdated Show resolved Hide resolved

internal/controller/oauth/oidc_provider.go Outdated Show resolved Hide resolved

yuzisun reviewed Feb 9, 2025

View reviewed changes

yuzisun added 3 commits February 9, 2025 13:59

Refactor oauth2 code

1d346ac

Signed-off-by: Dan Sun <[email protected]>

Add AWS rotator test

af1353b

Signed-off-by: Dan Sun <[email protected]>

Mock token source

924f43c

Signed-off-by: Dan Sun <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implements aws oidc backendSecurityPolicy API #306

feat: implements aws oidc backendSecurityPolicy API #306

aabchoo commented Feb 6, 2025 •

edited

Loading

mathetake commented Feb 6, 2025

aabchoo commented Feb 6, 2025

mathetake Feb 7, 2025

yuzisun Feb 7, 2025

yuzisun Feb 7, 2025

mathetake Feb 7, 2025

mathetake Feb 7, 2025

mathetake Feb 7, 2025

mathetake Feb 7, 2025

mathetake Feb 7, 2025

yuzisun Feb 7, 2025

yuzisun Feb 7, 2025

yuzisun Feb 7, 2025

yuzisun Feb 9, 2025

yuzisun Feb 9, 2025

	b.logger.Error(err, "failed to trigger refresh for existing backendSecPolicy resources")
	b.logger.Error(err, "failed to list existing backendSecPolicy resources")

feat: implements aws oidc backendSecurityPolicy API #306

Are you sure you want to change the base?

feat: implements aws oidc backendSecurityPolicy API #306

Conversation

aabchoo commented Feb 6, 2025 • edited Loading

mathetake commented Feb 6, 2025

aabchoo commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabchoo commented Feb 6, 2025 •

edited

Loading