Copilot commented on code in PR #2865:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2865#discussion_r3966509191
##########
internal/provider/apisix/provider.go:
##########
@@ -299,9 +390,37 @@ func (d *apisixProvider) Start(ctx context.Context) error {
}
func (d *apisixProvider) sync(ctx context.Context) error {
- statusesMap, err := d.client.Sync(ctx)
+ inputs, resourceErr := d.buildSyncInputs()
+ statusesMap, syncErr := d.client.Sync(ctx, inputs)
d.handleADCExecutionErrors(statusesMap)
- return err
+ return errors.Join(resourceErr, syncErr)
+}
+
+// buildSyncInputs organizes this round's full config set -- every
GatewayProxy AIC
+// currently knows about, each with its merged translated resource snapshot --
into the
+// input the adc client package needs. The client package never gathers this
itself.
+func (d *apisixProvider) buildSyncInputs() ([]adcclient.SyncInput, error) {
+ configs := d.configManager.List()
+ if len(configs) == 0 {
+ return nil, nil
+ }
+
+ inputs := make([]adcclient.SyncInput, 0, len(configs))
+ var errs []error
+ for _, config := range configs {
+ resources, err := d.store.GetResources(config.Name)
+ if err != nil {
+ d.log.Error(err, "failed to get resources from store",
"name", config.Name)
+ errs = append(errs, fmt.Errorf("config %s: %w",
config.Name, err))
+ continue
+ }
+ inputs = append(inputs, adcclient.SyncInput{
+ Name: config.Name,
+ Config: config,
+ Resources: resources,
+ })
+ }
+ return inputs, errors.Join(errs...)
}
Review Comment:
`applyResourceState()`/`removeResourceState()` protect
`configManager`+`store` mutations with `d.Lock()`, but `buildSyncInputs()`
reads both without any lock. If `Update/Delete` can run concurrently with the
sync loop, this can lead to races and potentially unsafe iteration (depending
on `ConfigManager`/`Store` internals). Consider taking a read lock around
`List()` + the corresponding `GetResources()` reads (or switching to an RWMutex
and using RLock here) to make reads consistent with the write-side locking.
##########
internal/adc/client/executor.go:
##########
@@ -301,58 +261,6 @@ func (e *HTTPADCExecutor)
runHTTPValidateForSingleServer(ctx context.Context, se
return e.handleHTTPValidateResponse(resp, serverAddr)
}
-// parseArgs parses the command line arguments to extract labels, types, and
file path
-func (e *HTTPADCExecutor) parseArgs(args []string) (map[string]string,
[]string, string, error) {
- labels := make(map[string]string)
- var types []string
- var filePath string
-
- for i := 0; i < len(args); i++ {
- switch args[i] {
- case "-f":
- if i+1 < len(args) {
- filePath = args[i+1]
- i++
- }
- case "--label-selector":
- if i+1 < len(args) {
- labelPair := args[i+1]
- parts := strings.SplitN(labelPair, "=", 2)
- if len(parts) == 2 {
- labels[parts[0]] = parts[1]
- }
- i++
- }
- case "--include-resource-type":
- if i+1 < len(args) {
- types = append(types, args[i+1])
- i++
- }
- }
- }
-
- if filePath == "" {
- return nil, nil, "", errors.New("file path not found in args")
- }
-
- return labels, types, filePath, nil
-}
-
-// loadResourcesFromFile loads ADC resources from the specified file
-func (e *HTTPADCExecutor) loadResourcesFromFile(filePath string)
(*adctypes.Resources, error) {
- data, err := os.ReadFile(filePath)
- if err != nil {
- return nil, fmt.Errorf("failed to read file: %w", err)
- }
-
- var resources adctypes.Resources
- if err := json.Unmarshal(data, &resources); err != nil {
- return nil, fmt.Errorf("failed to unmarshal resources: %w", err)
- }
-
- return &resources, nil
-}
-
// buildHTTPRequest builds the HTTP request for ADC Server
func (e *HTTPADCExecutor) buildHTTPRequest(ctx context.Context, serverAddr
string, config adctypes.Config, labels map[string]string, types []string,
resources *adctypes.Resources, path string) (*http.Request, error) {
Review Comment:
The parameter name `types` is ambiguous and also clashes with the imported
`types` package name used elsewhere in this file. Rename this parameter to
something explicit like `resourceTypes` to improve readability and avoid
accidental shadowing issues.
##########
internal/adc/client/client.go:
##########
@@ -283,56 +171,63 @@ func (c *Client) Validate(ctx context.Context, task Task)
error {
return nil
}
-func (c *Client) Sync(ctx context.Context)
(map[string]types.ADCExecutionErrors, error) {
- c.syncMu.Lock()
- defer c.syncMu.Unlock()
- c.log.Info("syncing all resources")
+// SyncInput is one GatewayProxy's complete sync unit. AIC builds it entirely
from its own
+// bookkeeping (which resources target this config, their merged translated
snapshot)
+// before handing it over -- this package never reaches back into AIC's state
to gather
+// anything itself, it only translates, sends, and interprets the response.
+type SyncInput struct {
+ // Name is the cacheKey: the GatewayProxy's own identity.
+ Name string
+ Config adctypes.Config
+ Resources *adctypes.Resources
+ ResourceTypes []string
+ Labels map[string]string
+}
- configs := c.ConfigManager.List()
+// MarshalLog implements logr.Marshaler so logging a SyncInput never dumps the
+// secret-bearing Resources body. Config redacts its own Token via
Config.MarshalJSON.
+func (in SyncInput) MarshalLog() any {
+ return map[string]any{
+ "name": in.Name,
+ "config": in.Config,
+ "labels": in.Labels,
+ "resourceTypes": in.ResourceTypes,
+ "resources": in.Resources.MarshalLog(),
+ }
+}
Review Comment:
`SyncInput.MarshalLog()` unconditionally calls `in.Resources.MarshalLog()`,
which will panic if `Resources` is nil. This is currently reachable because
`Sync()` logs `inputs` before it filters out entries with `Resources == nil`.
Fix by making `MarshalLog()` handle a nil `Resources` (e.g., return `nil`/a
placeholder for `resources`) and/or avoid logging full inputs (log input
names/counts) before validating entries.
##########
internal/provider/apisix/provider.go:
##########
@@ -222,25 +233,105 @@ func (d *apisixProvider) Delete(ctx context.Context, obj
client.Object) error {
// and it is not possible to perform scheduled synchronization
// on deleted gateway level resources
if len(resourceTypes) == 0 {
- return d.client.Delete(ctx, adcclient.Task{
- Key: nnk,
- Name: nnk.String(),
- Labels: labels,
- })
+ removed, err := d.removeResourceState(nnk, resourceTypes,
labels)
+ if err != nil {
+ return err
+ }
+ d.syncEvictedConfigsNow(ctx, removed, resourceTypes, labels)
+ return nil
+ }
+
+ removed, err := d.removeResourceState(nnk, resourceTypes, labels)
+ if err != nil {
+ return err
}
- delta, err := d.client.DeleteConfig(ctx, adcclient.Task{
- Key: nnk,
- Name: nnk.String(),
- Labels: labels,
- ResourceTypes: resourceTypes,
- })
- // Syncing pushes the whole store to every data plane. Objects this
controller
- // never configured delete nothing, and reconciles for them are
frequent, so
- // notify only when the store actually changed.
- if len(delta.Deleted) > 0 {
+ // Syncing pushes the whole store to every data plane. Objects this
controller never
+ // configured delete nothing, and reconciles for them are frequent, so
notify only
+ // when the store actually changed.
+ if len(removed) > 0 {
d.syncNotify()
}
- return err
+ return nil
+}
+
+// applyResourceState upserts a resource's config associations and its
contribution to each
+// target config's cached resource snapshot -- the AIC-side bookkeeping the
adc client
+// package no longer holds itself.
+func (d *apisixProvider) applyResourceState(
+ rk types.NamespacedNameKind,
+ configs map[types.NamespacedNameKind]adctypes.Config,
+ resourceTypes []string,
+ resources *adctypes.Resources,
+ labels map[string]string,
+) error {
Review Comment:
This PR introduces new provider-owned bookkeeping behavior
(`applyResourceState`, `removeResourceState`, `buildSyncInputs`,
`syncEvictedConfigsNow`) that replaces the old client-owned state transitions.
Given existing Go unit tests in this area, add tests covering at least: (1)
`Delete` on gateway-level resources triggering the immediate empty sync for
evicted configs, and (2) `buildSyncInputs` error aggregation while still
syncing healthy configs.
##########
internal/adc/client/client.go:
##########
@@ -387,88 +282,45 @@ func (c *Client) push(ctx context.Context, config
adctypes.Config, args []string
return alsoReport, err
}
-func (c *Client) sync(ctx context.Context, task Task) error {
- c.log.V(1).Info("syncing resources", "task", task)
-
- if len(task.Configs) == 0 {
- c.log.Info("no adc configs provided")
- return nil
- }
+func (c *Client) syncOne(ctx context.Context, in SyncInput) error {
+ c.log.V(1).Info("syncing resources", "input", in)
var errs types.ADCExecutionErrors
- // Record file I/O duration
- fileIOStart := time.Now()
- // every task resources is the same, so we can use the first config to
prepare the sync file
- syncFilePath, cleanup, err := prepareSyncFile(task.Resources)
- if err != nil {
- pkgmetrics.RecordFileIODuration("prepare_sync_file", "failure",
time.Since(fileIOStart).Seconds())
- return err
+ config := in.Config
+ if config.BackendType == "" {
+ config.BackendType = c.defaultMode
}
- pkgmetrics.RecordFileIODuration("prepare_sync_file",
adctypes.StatusSuccess, time.Since(fileIOStart).Seconds())
- defer cleanup()
- c.log.V(1).Info("prepared sync file", "path", syncFilePath)
-
- args := BuildADCExecuteArgs(syncFilePath, task.Labels,
task.ResourceTypes)
-
- for _, config := range task.Configs {
- // Record sync duration for each config
- startTime := time.Now()
- resourceType := strings.Join(task.ResourceTypes, ",")
- if resourceType == "" {
- resourceType = "all"
- }
- if config.BackendType == "" {
- config.BackendType = c.defaultMode
- }
- alsoReport, err := c.push(ctx, config, args)
- errs.Errors = append(errs.Errors, alsoReport...)
+ startTime := time.Now()
+ resourceType := strings.Join(in.ResourceTypes, ",")
+ if resourceType == "" {
+ resourceType = "all"
+ }
- duration := time.Since(startTime).Seconds()
+ alsoReport, err := c.push(ctx, config, in.Resources, in.Labels,
in.ResourceTypes)
+ errs.Errors = append(errs.Errors, alsoReport...)
- status := adctypes.StatusSuccess
- if err != nil {
- status = "failure"
- c.log.Error(err, "failed to execute adc command",
"config", config)
+ duration := time.Since(startTime).Seconds()
- var execErr types.ADCExecutionError
- if errors.As(err, &execErr) {
- errs.Errors = append(errs.Errors, execErr)
- pkgmetrics.RecordExecutionError(config.Name,
execErr.Name)
- } else {
- pkgmetrics.RecordExecutionError(config.Name,
"unknown")
- }
+ status := adctypes.StatusSuccess
+ if err != nil {
+ status = "failure"
+ c.log.Error(err, "failed to execute adc command", "config",
config)
Review Comment:
This log message still refers to executing an 'adc command', but the
refactor removed the CLI/arg-based execution path and now uses HTTP
sync/validate directly. Updating the message to reflect the current behavior
(e.g., 'failed to sync with ADC' / 'failed to execute ADC request') will make
operational logs less misleading.
##########
internal/provider/apisix/status.go:
##########
@@ -109,7 +109,7 @@ func (d *apisixProvider) updateStatus(nnk
types.NamespacedNameKind, condition me
}),
})
case types.KindHTTPRoute:
- parentRefs :=
d.client.ConfigManager.GetConfigRefsByResourceKey(nnk)
+ parentRefs := d.configManager.GetConfigRefsByResourceKey(nnk)
Review Comment:
Status updates now read `d.configManager` (and `d.store` elsewhere in this
file) directly rather than through the old
client-owned/possibly-internally-synchronized accessors. Since provider
mutations are guarded by `d.Lock()` in other paths, these read paths should use
the same synchronization strategy (e.g., RLock/Lock) to avoid data races or
inconsistent reads during concurrent updates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
