Skip to content

Commit

Permalink
OAuth2: Dynamic Secrets
Browse files Browse the repository at this point in the history
Code review points:

> This is a bit confusing. We are creating a variable called sdsConfig but the function makes a config source specifier?
>
> and in the function itself it refers to Ads?
>
> ```go
> ConfigSourceSpecifier: &envoy_config_core_v3.ConfigSource_Ads{
>     Ads: &envoy_config_core_v3.AggregatedConfigSource{},
> },
> ```

`internal/envoy/auth/oauth2_filter.go`: Rename to `makeConfigSource()`.

> commented out code? Is it needed?

`internal/envoy/manager/envoy_callbacks.go`: Remove commented out code.

> Why did this need to change?

`internal/envoy/manager/cache_manager.go`: Revert change of map initialization.

Signed-off-by: Mohamed Bana <mohamed@bana.io>
  • Loading branch information
mbana committed Sep 6, 2022
1 parent 7d82f0e commit 03f91e9
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 14 deletions.
4 changes: 2 additions & 2 deletions internal/envoy/auth/oauth2_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func NewFilterHTTPOAuth2(oauth2Options *options.OAuth2, args *parseAuthOptionsAr
}
authorizationEndpoint := oauth2Options.AuthorizationEndpoint

sdsConfig := makeConfigSourceSpecifier()
sdsConfig := makeConfigSource()

tokenSecret := &envoy_extensions_transport_sockets_tls_v3.SdsSecretConfig{
Name: "client_secret_test_1",
Expand Down Expand Up @@ -176,7 +176,7 @@ func NewFilterHTTPOAuth2(oauth2Options *options.OAuth2, args *parseAuthOptionsAr
return anyOAuth2, nil
}

func makeConfigSourceSpecifier() *envoy_config_core_v3.ConfigSource {
func makeConfigSource() *envoy_config_core_v3.ConfigSource {
return &envoy_config_core_v3.ConfigSource{
ConfigSourceSpecifier: &envoy_config_core_v3.ConfigSource_Ads{
Ads: &envoy_config_core_v3.AggregatedConfigSource{},
Expand Down
2 changes: 1 addition & 1 deletion internal/envoy/manager/cache_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type cacheManager struct {
func NewCacheManager(snapshotCache cache_v3.SnapshotCache, logger logr.Logger) *cacheManager {
return &cacheManager{
SnapshotCache: snapshotCache,
fleetSnapshot: map[string]*cache_v3.Snapshot{},
fleetSnapshot: make(map[string]*cache_v3.Snapshot),
mu: sync.RWMutex{},
logger: logger.WithName("CacheManager"),
}
Expand Down
11 changes: 0 additions & 11 deletions internal/envoy/manager/envoy_callbacks.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,6 @@ func (c *Callbacks) OnDeltaStreamClosed(id int64) {
func (c *Callbacks) OnStreamRequest(id int64, request *envoy_discovery_v3.DiscoveryRequest) error {
c.logger.Info("OnStreamRequest", "id", id, "request.TypeUrl", request.TypeUrl, "request.Node.Cluster", request.Node.Cluster, "request.Node.Id", request.Node.Id)

// if c.cacheManager.IsNodeExist(request.Node.Id) {
// return nil
// }

// TODO(MBana): Could someone please confirm why this is needed here.
// c.logger.V(1).Info("OnStreamRequest setting snapshot", "id", id, "request.TypeUrl", request.TypeUrl, "request.Node.Cluster", request.Node.Cluster, "request.Node.Id", request.Node.Id)
// if err := c.cacheManager.setNodeSnapshot(request.Node.Id, request.Node.Cluster); err != nil {
// c.logger.Error(err, "OnStreamRequest", "id", id, "request.TypeUrl", request.TypeUrl, "request.Node.Cluster", request.Node.Cluster, "request.Node.Id", request.Node.Id)
// return err
// }

return nil
}

Expand Down

0 comments on commit 03f91e9

Please sign in to comment.