Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix panic when updating the envoy-gateway-config configMap #5066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix panic when updating the envoy-gateway-config configMap #5066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions internal/cmd/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@
// setupRunners starts all the runners required for the Envoy Gateway to
// fulfill its tasks.
func setupRunners(ctx context.Context, cfg *config.Server) (err error) {
// The Elected channel is used to block the tasks that are waiting for the leader to be elected.
// It will be closed once the leader is elected in the controller manager.
cfg.Elected = make(chan struct{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot use one channel to block multiple threads (status updater and infra runner)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The channel is closed after an EG instance is elected, so all the blocked threads will be notified.


Check warning on line 134 in internal/cmd/server.go

View check run for this annotation

Codecov / codecov/patch

internal/cmd/server.go#L131-L134 

Added lines #L131 - L134 were not covered by tests
// Setup the Extension Manager
var extMgr types.Manager
if cfg.EnvoyGateway.Provider.Type == egv1a1.ProviderTypeKubernetes {
Expand Down
17 changes: 6 additions & 11 deletions internal/envoygateway/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package config

import (
"errors"
"sync"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
"github.com/envoyproxy/gateway/api/v1alpha1/validation"
Expand Down Expand Up @@ -37,23 +36,19 @@ type Server struct {
DNSDomain string
// Logger is the logr implementation used by Envoy Gateway.
Logger logging.Logger
// Elected chan is used to signal what a leader is elected
Elected *sync.WaitGroup
// Elected chan is used to signal when an EG instance is elected as leader.
Elected chan struct{}
}

// New returns a Server with default parameters.
func New() (*Server, error) {
server := &Server{
return &Server{
EnvoyGateway: egv1a1.DefaultEnvoyGateway(),
Namespace: env.Lookup("ENVOY_GATEWAY_NAMESPACE", DefaultNamespace),
DNSDomain: env.Lookup("KUBERNETES_CLUSTER_DOMAIN", DefaultDNSDomain),
// the default logger
Logger: logging.DefaultLogger(egv1a1.LogLevelInfo),
Elected: &sync.WaitGroup{},
}
// Block the tasks that are waiting for the leader to be elected
server.Elected.Add(1)
return server, nil
Logger: logging.DefaultLogger(egv1a1.LogLevelInfo),
Elected: make(chan struct{}),
}, nil
}

// Validate validates a Server config.
Expand Down
8 changes: 6 additions & 2 deletions internal/infrastructure/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,12 @@
if r.EnvoyGateway.Provider.Type == egv1a1.ProviderTypeKubernetes &&
!ptr.Deref(r.EnvoyGateway.Provider.Kubernetes.LeaderElection.Disable, false) {
go func() {
r.Elected.Wait()
initInfra()
select {
case <-ctx.Done():
return
case <-r.Elected:
initInfra()

Check warning on line 79 in internal/infrastructure/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

internal/infrastructure/runner/runner.go#L75-L79 

Added lines #L75 - L79 were not covered by tests
}
}()
return
}
Expand Down
12 changes: 7 additions & 5 deletions internal/provider/kubernetes/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,9 @@
}

// newGatewayAPIController
func newGatewayAPIController(mgr manager.Manager, cfg *config.Server, su Updater,
func newGatewayAPIController(ctx context.Context, mgr manager.Manager, cfg *config.Server, su Updater,
resources *message.ProviderResources,
) error {
ctx := context.Background()

// Gather additional resources to watch from registered extensions
var extServerPoliciesGVKs []schema.GroupVersionKind
var extGVKs []schema.GroupVersionKind
Expand Down Expand Up @@ -138,8 +136,12 @@
if cfg.EnvoyGateway.Provider.Type == egv1a1.ProviderTypeKubernetes &&
!ptr.Deref(cfg.EnvoyGateway.Provider.Kubernetes.LeaderElection.Disable, false) {
go func() {
cfg.Elected.Wait()
r.subscribeAndUpdateStatus(ctx, cfg.EnvoyGateway.EnvoyGatewaySpec.ExtensionManager != nil)
select {
case <-ctx.Done():
return

Check warning on line 141 in internal/provider/kubernetes/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/kubernetes/controller.go#L140-L141 

Added lines #L140 - L141 were not covered by tests
case <-cfg.Elected:
r.subscribeAndUpdateStatus(ctx, cfg.EnvoyGateway.EnvoyGatewaySpec.ExtensionManager != nil)
}
}()
} else {
r.subscribeAndUpdateStatus(ctx, cfg.EnvoyGateway.EnvoyGatewaySpec.ExtensionManager != nil)
Expand Down
8 changes: 5 additions & 3 deletions internal/provider/kubernetes/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ type Provider struct {
}

// New creates a new Provider from the provided EnvoyGateway.
func New(restCfg *rest.Config, svrCfg *ec.Server, resources *message.ProviderResources) (*Provider, error) {
func New(ctx context.Context, restCfg *rest.Config, svrCfg *ec.Server, resources *message.ProviderResources) (*Provider, error) {
// TODO: Decide which mgr opts should be exposed through envoygateway.provider.kubernetes API.

mgrOpts := manager.Options{
Expand Down Expand Up @@ -95,7 +95,7 @@ func New(restCfg *rest.Config, svrCfg *ec.Server, resources *message.ProviderRes
}

// Create and register the controllers with the manager.
if err := newGatewayAPIController(mgr, svrCfg, updateHandler.Writer(), resources); err != nil {
if err := newGatewayAPIController(ctx, mgr, svrCfg, updateHandler.Writer(), resources); err != nil {
return nil, fmt.Errorf("failted to create gatewayapi controller: %w", err)
}

Expand All @@ -112,7 +112,9 @@ func New(restCfg *rest.Config, svrCfg *ec.Server, resources *message.ProviderRes
// Emit elected & continue with the tasks that require leadership.
go func() {
<-mgr.Elected()
svrCfg.Elected.Done()
// Close the elected channel to signal that this EG instance has been elected as leader.
// This allows the tasks that require leadership to proceed.
close(svrCfg.Elected)
}()

return &Provider{
Expand Down
6 changes: 3 additions & 3 deletions internal/provider/kubernetes/kubernetes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestProvider(t *testing.T) {
svr, err := config.New()
require.NoError(t, err)
resources := new(message.ProviderResources)
provider, err := New(cliCfg, svr, resources)
provider, err := New(context.Background(), cliCfg, svr, resources)
require.NoError(t, err)
ctx, cancel := context.WithCancel(ctrl.SetupSignalHandler())
go func() {
Expand Down Expand Up @@ -1274,7 +1274,7 @@ func TestNamespacedProvider(t *testing.T) {
LeaderElection: egv1a1.DefaultLeaderElection(),
}
resources := new(message.ProviderResources)
provider, err := New(cliCfg, svr, resources)
provider, err := New(context.Background(), cliCfg, svr, resources)
require.NoError(t, err)
ctx, cancel := context.WithCancel(context.Background())
go func() {
Expand Down Expand Up @@ -1334,7 +1334,7 @@ func TestNamespaceSelectorProvider(t *testing.T) {
LeaderElection: egv1a1.DefaultLeaderElection(),
}
resources := new(message.ProviderResources)
provider, err := New(cliCfg, svr, resources)
provider, err := New(context.Background(), cliCfg, svr, resources)
require.NoError(t, err)
ctx, cancel := context.WithCancel(context.Background())
go func() {
Expand Down
6 changes: 3 additions & 3 deletions internal/provider/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
var p provider.Provider
switch r.EnvoyGateway.Provider.Type {
case egv1a1.ProviderTypeKubernetes:
p, err = r.createKubernetesProvider()
p, err = r.createKubernetesProvider(ctx)

Check warning on line 46 in internal/provider/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/runner/runner.go#L46 

Added line #L46 was not covered by tests
if err != nil {
return fmt.Errorf("failed to create kubernetes provider: %w", err)
}
Expand All @@ -69,13 +69,13 @@
return nil
}

func (r *Runner) createKubernetesProvider() (*kubernetes.Provider, error) {
func (r *Runner) createKubernetesProvider(ctx context.Context) (*kubernetes.Provider, error) {

Check warning on line 72 in internal/provider/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/runner/runner.go#L72 

Added line #L72 was not covered by tests
cfg, err := ctrl.GetConfig()
if err != nil {
return nil, fmt.Errorf("failed to get kubeconfig: %w", err)
}

p, err := kubernetes.New(cfg, &r.Config.Server, r.ProviderResources)
p, err := kubernetes.New(ctx, cfg, &r.Config.Server, r.ProviderResources)

Check warning on line 78 in internal/provider/runner/runner.go

View check run for this annotation

Codecov / codecov/patch

internal/provider/runner/runner.go#L78 

Added line #L78 was not covered by tests
if err != nil {
return nil, fmt.Errorf("failed to create provider %s: %w", egv1a1.ProviderTypeKubernetes, err)
}
Expand Down
2 changes: 1 addition & 1 deletion release-notes/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ new features: |
Added support for GEP-1731 (HTTPRoute Retries)

bug fixes: |

Fixed a panic that occurred following update to the envoy-gateway-config ConfigMap

# Enhancements that improve performance.
performance improvements: |
Expand Down
Loading