Skip to content

Commit 4789243

Browse files
AlinsRanCopilot
andcommitted
fix: address PR review comments
- fail-open on ADC validator init error in ApisixRoute, ApisixConsumer, Consumer webhooks - redact TLS payload from logs in buildAPISIXValidateRequest (log bodyLen only) - set TLS MinVersion to 1.2 in newBackendHTTPClient - return JSON unmarshal error in extractCredentialKey instead of swallowing it - default supportsEndpointSlice to false in PrepareApisixRouteForValidation - rename waitExponentialBackoffWithTimeout to waitConstantIntervalWithTimeout (Factor=1 is constant, not exponential) - restore WaitPodsAvailable timeout to 2 minutes to match original behavior - add cache-propagation sleep after recreating TLS Secret in e2e test - tighten expectAdmissionDenied to assert NotFound error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 0db12c2 commit 4789243

8 files changed

Lines changed: 26 additions & 15 deletions

File tree

internal/adc/client/executor.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ func (e *HTTPADCExecutor) buildAPISIXValidateRequest(ctx context.Context, server
312312
"url", validateURL,
313313
"server", serverAddr,
314314
"cacheKey", config.Name,
315-
"body", body,
315+
"bodyLen", len(jsonData),
316316
)
317317

318318
req, err := http.NewRequestWithContext(ctx, http.MethodPost, validateURL, bytes.NewBuffer(jsonData))
@@ -327,10 +327,11 @@ func (e *HTTPADCExecutor) buildAPISIXValidateRequest(ctx context.Context, server
327327

328328
func (e *HTTPADCExecutor) newBackendHTTPClient(config adctypes.Config) *http.Client {
329329
transport := http.DefaultTransport.(*http.Transport).Clone()
330+
if transport.TLSClientConfig == nil {
331+
transport.TLSClientConfig = &tls.Config{}
332+
}
333+
transport.TLSClientConfig.MinVersion = tls.VersionTLS12
330334
if !config.TlsVerify {
331-
if transport.TLSClientConfig == nil {
332-
transport.TLSClientConfig = &tls.Config{}
333-
}
334335
transport.TLSClientConfig.InsecureSkipVerify = true
335336
}
336337

internal/controller/webhook_validation.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func PrepareApisixRouteForValidation(ctx context.Context, c client.Client, log l
4545
Client: c,
4646
Log: log,
4747
ICGV: networkingv1.SchemeGroupVersion,
48-
supportsEndpointSlice: true,
48+
supportsEndpointSlice: false,
4949
}
5050
if err := reconciler.processApisixRoute(tctx, route); err != nil {
5151
return nil, err

internal/webhook/v1/apisixconsumer_webhook.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,8 @@ func (v *ApisixConsumerCustomValidator) ValidateCreate(ctx context.Context, obj
7676

7777
warnings := v.collectWarnings(ctx, consumer)
7878
if v.initErr != nil {
79-
return warnings, v.initErr
79+
apisixConsumerLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
80+
return warnings, nil
8081
}
8182
return warnings, v.adcValidator.Validate(ctx, consumer)
8283
}
@@ -93,7 +94,8 @@ func (v *ApisixConsumerCustomValidator) ValidateUpdate(ctx context.Context, oldO
9394

9495
warnings := v.collectWarnings(ctx, consumer)
9596
if v.initErr != nil {
96-
return warnings, v.initErr
97+
apisixConsumerLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
98+
return warnings, nil
9799
}
98100
return warnings, v.adcValidator.Validate(ctx, consumer)
99101
}

internal/webhook/v1/apisixroute_webhook.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ func (v *ApisixRouteCustomValidator) ValidateCreate(ctx context.Context, obj run
7474

7575
warnings := v.collectWarnings(ctx, route)
7676
if v.initErr != nil {
77-
return warnings, v.initErr
77+
apisixRouteLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
78+
return warnings, nil
7879
}
7980
return warnings, v.adcValidator.Validate(ctx, route)
8081
}
@@ -91,7 +92,8 @@ func (v *ApisixRouteCustomValidator) ValidateUpdate(ctx context.Context, oldObj,
9192

9293
warnings := v.collectWarnings(ctx, route)
9394
if v.initErr != nil {
94-
return warnings, v.initErr
95+
apisixRouteLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
96+
return warnings, nil
9597
}
9698
return warnings, v.adcValidator.Validate(ctx, route)
9799
}

internal/webhook/v1/consumer_webhook.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,8 @@ func (v *ConsumerCustomValidator) ValidateCreate(ctx context.Context, obj runtim
7777

7878
warnings := v.collectWarnings(ctx, consumer)
7979
if v.initErr != nil {
80-
return warnings, v.initErr
80+
consumerLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
81+
return warnings, nil
8182
}
8283
if err := v.validateDuplicateKeyAuthCredentials(ctx, consumer); err != nil {
8384
return warnings, err
@@ -97,7 +98,8 @@ func (v *ConsumerCustomValidator) ValidateUpdate(ctx context.Context, oldObj, ne
9798

9899
warnings := v.collectWarnings(ctx, consumer)
99100
if v.initErr != nil {
100-
return warnings, v.initErr
101+
consumerLog.Error(v.initErr, "ADC validator init failed, skipping ADC validation")
102+
return warnings, nil
101103
}
102104
if err := v.validateDuplicateKeyAuthCredentials(ctx, consumer); err != nil {
103105
return warnings, err
@@ -224,7 +226,7 @@ func (v *ConsumerCustomValidator) extractCredentialKey(ctx context.Context, cons
224226
Key string `json:"key"`
225227
}
226228
if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil {
227-
return "", nil
229+
return "", err
228230
}
229231
return cfg.Key, nil
230232
}

test/e2e/framework/k8s.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ func (f *Framework) applySSLSecret(namespace, name string, cert, pkey, caCert []
261261
}
262262

263263
func WaitPodsAvailable(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions) error {
264-
return WaitPodsAvailableWithTimeout(t, kubeOps, opts, time.Minute)
264+
return WaitPodsAvailableWithTimeout(t, kubeOps, opts, 2*time.Minute)
265265
}
266266

267267
func WaitPodsAvailableWithTimeout(t testing.TestingT, kubeOps *k8s.KubectlOptions, opts metav1.ListOptions, timeout time.Duration) error {
@@ -295,7 +295,7 @@ func WaitPodsAvailableWithTimeout(t testing.TestingT, kubeOps *k8s.KubectlOption
295295
}
296296
return true, nil
297297
}
298-
err := waitExponentialBackoffWithTimeout(condFunc, timeout)
298+
err := waitConstantIntervalWithTimeout(condFunc, timeout)
299299
if err != nil && lastErr != nil {
300300
return lastErr
301301
}
@@ -329,7 +329,7 @@ func describePodStatus(pod corev1.Pod) string {
329329
)
330330
}
331331

332-
func waitExponentialBackoffWithTimeout(condFunc func() (bool, error), timeout time.Duration) error {
332+
func waitConstantIntervalWithTimeout(condFunc func() (bool, error), timeout time.Duration) error {
333333
steps := int(timeout / (2 * time.Second))
334334
if steps < 1 {
335335
steps = 1

test/e2e/webhook/apisixtls.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,9 @@ spec:
142142
err = s.NewKubeTlsSecret(serverSecret, serverCert.String(), serverKey.String())
143143
Expect(err).NotTo(HaveOccurred(), "creating valid server TLS secret")
144144

145+
// Wait for the webhook cache to reflect the recreated Secret before submitting ApisixTls.
146+
time.Sleep(2 * time.Second)
147+
145148
By("creating corrected ApisixTls")
146149
err = s.CreateResourceFromString(tlsYAML)
147150
Expect(err).NotTo(HaveOccurred(), "creating corrected ApisixTls")

test/e2e/webhook/helpers.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,4 +245,5 @@ func expectAdmissionDenied(s *scaffold.Scaffold, resourceType, resourceName stri
245245

246246
_, getErr := s.GetOutputFromString(resourceType, resourceName, "-o", "yaml")
247247
Expect(getErr).To(HaveOccurred(), fmt.Sprintf("resource %s/%s should not exist after admission rejection", resourceType, resourceName))
248+
Expect(getErr.Error()).To(ContainSubstring("not found"), fmt.Sprintf("expected NotFound error for %s/%s", resourceType, resourceName))
248249
}

0 commit comments

Comments
 (0)