Skip to content

Commit

Permalink
Prevent partial application of Envoy extensions
Browse files Browse the repository at this point in the history
Ensure that non-required extensions do not change xDS resources before
exiting on failure by cloning proto messages prior to applying each
extension.

To support this change, also move `CanApply` checks up a layer and make
them prior to attempting extension application, s.t. we avoid
unnecessary copies where extensions can't be applied.

Last, ensure that we do not allow panics from `CanApply` or `Extend`
checks to escape the attempted extension application.
  • Loading branch information
zalimeni committed Jul 27, 2023
1 parent cbfeb6c commit a920c71
Show file tree
Hide file tree
Showing 12 changed files with 537 additions and 34 deletions.
3 changes: 3 additions & 0 deletions .changelog/18068.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
xds: Prevent partial application of non-Required Envoy extensions in the case of failure.
```
74 changes: 54 additions & 20 deletions agent/xds/delta.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func (s *Server) processDelta(stream ADSDeltaStream, reqCh <-chan *envoy_discove
s.ResourceMapMutateFn(newResourceMap)
}

if err = s.applyEnvoyExtensions(newResourceMap, cfgSnap, node); err != nil {
if newResourceMap, err = s.applyEnvoyExtensions(newResourceMap, cfgSnap, node); err != nil {
// err is already the result of calling status.Errorf
return err
}
Expand Down Expand Up @@ -403,30 +403,30 @@ func (s *Server) processDelta(stream ADSDeltaStream, reqCh <-chan *envoy_discove
}
}

func (s *Server) applyEnvoyExtensions(resources *xdscommon.IndexedResources, cfgSnap *proxycfg.ConfigSnapshot, node *envoy_config_core_v3.Node) error {
func (s *Server) applyEnvoyExtensions(resources *xdscommon.IndexedResources, cfgSnap *proxycfg.ConfigSnapshot, node *envoy_config_core_v3.Node) (*xdscommon.IndexedResources, error) {
var err error
envoyVersion := xdscommon.DetermineEnvoyVersionFromNode(node)
consulVersion, err := goversion.NewVersion(version.Version)

if err != nil {
return status.Errorf(codes.InvalidArgument, "failed to parse Consul version")
return nil, status.Errorf(codes.InvalidArgument, "failed to parse Consul version")
}

serviceConfigs := extensionruntime.GetRuntimeConfigurations(cfgSnap)
for _, cfgs := range serviceConfigs {
for _, cfg := range cfgs {
err = applyEnvoyExtension(s.Logger, cfgSnap, resources, cfg, envoyVersion, consulVersion)
resources, err = validateAndApplyEnvoyExtension(s.Logger, cfgSnap, resources, cfg, envoyVersion, consulVersion)

if err != nil {
return err
return nil, err
}
}
}

return nil
return resources, nil
}

func applyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot, resources *xdscommon.IndexedResources, runtimeConfig extensioncommon.RuntimeConfig, envoyVersion, consulVersion *goversion.Version) error {
func validateAndApplyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot, resources *xdscommon.IndexedResources, runtimeConfig extensioncommon.RuntimeConfig, envoyVersion, consulVersion *goversion.Version) (*xdscommon.IndexedResources, error) {
logFn := logger.Warn
if runtimeConfig.EnvoyExtension.Required {
logFn = logger.Error
Expand Down Expand Up @@ -460,14 +460,14 @@ func applyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot,
logFn("failed to parse Envoy extension version constraint", errorParams...)

if ext.Required {
return status.Errorf(codes.InvalidArgument, "failed to parse Envoy version constraint for extension %q for service %q", ext.Name, svc.Name)
return nil, status.Errorf(codes.InvalidArgument, "failed to parse Envoy version constraint for extension %q for service %q", ext.Name, svc.Name)
}
return nil
return resources, nil
}

if !c.Check(envoyVersion) {
logger.Info("skipping envoy extension due to Envoy version constraint violation", errorParams...)
return nil
return resources, nil
}
}

Expand All @@ -477,14 +477,14 @@ func applyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot,
logFn("failed to parse Consul extension version constraint", errorParams...)

if ext.Required {
return status.Errorf(codes.InvalidArgument, "failed to parse Consul version constraint for extension %q for service %q", ext.Name, svc.Name)
return nil, status.Errorf(codes.InvalidArgument, "failed to parse Consul version constraint for extension %q for service %q", ext.Name, svc.Name)
}
return nil
return resources, nil
}

if !c.Check(consulVersion) {
logger.Info("skipping envoy extension due to Consul version constraint violation", errorParams...)
return nil
return resources, nil
}
}

Expand All @@ -496,10 +496,10 @@ func applyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot,
logFn("failed to construct extension", errorParams...)

if ext.Required {
return status.Errorf(codes.InvalidArgument, "failed to construct extension %q for service %q", ext.Name, svc.Name)
return nil, status.Errorf(codes.InvalidArgument, "failed to construct extension %q for service %q", ext.Name, svc.Name)
}

return nil
return resources, nil
}

now = time.Now()
Expand All @@ -510,25 +510,59 @@ func applyEnvoyExtension(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot,
logFn("failed to validate extension arguments", errorParams...)

if ext.Required {
return status.Errorf(codes.InvalidArgument, "failed to validate arguments for extension %q for service %q", ext.Name, svc.Name)
return nil, status.Errorf(codes.InvalidArgument, "failed to validate arguments for extension %q for service %q", ext.Name, svc.Name)
}

return nil
return resources, nil
}

now = time.Now()
_, err = extender.Extend(resources, &runtimeConfig)
resources, err = applyEnvoyExtension(extender, resources, &runtimeConfig)
metrics.MeasureSinceWithLabels([]string{"envoy_extension", "extend"}, now, getMetricLabels(err))
if err != nil {
errorParams = append(errorParams, "error", err)
logFn("failed to apply envoy extension", errorParams...)

if ext.Required {
return status.Errorf(codes.InvalidArgument, "failed to patch xDS resources in the %q extension: %v", ext.Name, err)
return nil, status.Errorf(codes.InvalidArgument, "failed to patch xDS resources in the %q extension: %v", ext.Name, err)
}
}

return nil
return resources, nil
}

// applyEnvoyExtension safely checks whether an extension can be applied, and if so attempts to apply it.
//
// applyEnvoyExtension makes a copy of the provided IndexedResources, then applies the given extension to them.
// The copy ensures against partial application if a non-required extension modifies a resource then fails at a later
// stage; this is necessary because IndexedResources and its proto messages are all passed by reference, and
// non-required extensions do not lead to a terminal failure in xDS updates.
//
// If the application is successful, the modified copy is returned. If not, the original and an error is returned.
// Returning resources in either case allows for applying extensions in a loop and reporting on non-required extension
// failures simultaneously.
func applyEnvoyExtension(extender extensioncommon.EnvoyExtender, resources *xdscommon.IndexedResources, runtimeConfig *extensioncommon.RuntimeConfig) (r *xdscommon.IndexedResources, e error) {
// Don't panic due to an extension misbehaving.
defer func() {
if err := recover(); err != nil {
r = resources
e = fmt.Errorf("attempt to apply Envoy extension %q caused an unexpected panic: %v",
runtimeConfig.EnvoyExtension.Name, err)
}
}()

// First check whether the extension is eligible for application in the current environment.
// Do this before copying indexed resources for the sake of efficiency.
if !extender.CanApply(runtimeConfig) {
return resources, nil
}

newResources, err := extender.Extend(xdscommon.Clone(resources), runtimeConfig)
if err != nil {
return resources, err
}

return newResources, nil
}

// https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#eventual-consistency-considerations
Expand Down
2 changes: 1 addition & 1 deletion agent/xds/delta_envoy_extender_oss_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ end`,
cfgs := extensionruntime.GetRuntimeConfigurations(snap)
for _, extensions := range cfgs {
for _, ext := range extensions {
err := applyEnvoyExtension(hclog.NewNullLogger(), snap, indexedResources, ext, parsedEnvoyVersion, consulVersion)
indexedResources, err = validateAndApplyEnvoyExtension(hclog.NewNullLogger(), snap, indexedResources, ext, parsedEnvoyVersion, consulVersion)
require.NoError(t, err)
}
}
Expand Down
Loading

0 comments on commit a920c71

Please sign in to comment.