Add configurable timeout settings for all validation webhooks#218
Open
alam-cloud wants to merge 1 commit intoyokecd:mainfrom
Open
Add configurable timeout settings for all validation webhooks#218alam-cloud wants to merge 1 commit intoyokecd:mainfrom
alam-cloud wants to merge 1 commit intoyokecd:mainfrom
Conversation
davidmdm
reviewed
Nov 30, 2025
cmd/atc/resources.go
Outdated
| if cfg.ExternalResourceValidationWebhookTimeout > 0 { | ||
| return ptr.To(cfg.ExternalResourceValidationWebhookTimeout) | ||
| } | ||
| return ptr.To[int32](30) |
Collaborator
There was a problem hiding this comment.
This one should be very short.
This is because this webhook applies to all resources, and we if the airway is not available we don't want to block admission if we can avoid it.
Also, the entirety of the implementation of this webhook is in memory:
- looking up a sync.Map key/value
- writing an event to an in-memory queue
And so should take on the order of milliseconds to execute.
davidmdm
reviewed
Nov 30, 2025
cmd/atc-installer/installer/run.go
Outdated
| CacheFS string `json:"cacheFS,omitzero" Description:"controls location to mount empty dir for wasm module fs cache. Defaults to /tmp if unset"` | ||
| AirwayValidationWebhookTimeout int `json:"airwayValidationWebhookTimeout,omitzero" Description:"timeout in seconds for airway instance validation webhooks (default: 10)"` | ||
| ResourceValidationWebhookTimeout int `json:"resourceValidationWebhookTimeout,omitzero" Description:"timeout in seconds for resource/event dispatching validation webhooks (default: 5)"` | ||
| ExternalResourceValidationWebhookTimeout int `json:"externalResourceValidationWebhookTimeout,omitzero" Description:"timeout in seconds for external resource validation webhooks (default: 30)"` |
Collaborator
There was a problem hiding this comment.
Remember to change the default here to 1.
davidmdm
reviewed
Nov 30, 2025
cmd/atc/resources.go
Outdated
| } | ||
|
|
||
| // Get timeout value for airway validation webhook, defaulting to 10 seconds if not configured | ||
| airwayTimeoutSeconds := func() *int32 { |
Collaborator
There was a problem hiding this comment.
perhaps wrap this logic of checking an int32 and returning a default if <=0 into a small utility function?
This is taking up a lot of vertical space for something that could probably be read inline in a declarative way:
Example:
Timeout: withDefault(cfg.AirwayValidationWebhookTimeout, 10)
alam-cloud
force-pushed
the
atc-configure-validation-webhook-timeout-v2
branch
from
6274844 to
cb52118
Compare
davidmdm
reviewed
Dec 4, 2025
| flightTimeoutSeconds := withDefault(cfg.FlightValidationWebhookTimeout, 30) | ||
| resourceTimeoutSeconds := withDefault(cfg.ResourceValidationWebhookTimeout, 10) | ||
| externalResourceTimeoutSeconds := withDefault(cfg.ExternalResourceValidationWebhookTimeout, 1) | ||
| airwayValidation := &admissionregistrationv1.ValidatingWebhookConfiguration{ |
Collaborator
There was a problem hiding this comment.
Can we inline these values into CRD definitions using standard cmp.Or and ptr.To?
alam-cloud
force-pushed
the
atc-configure-validation-webhook-timeout-v2
branch
from
cb52118 to
48614aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add configurable timeout settings for all validation webhooks
Summary
This PR adds configurable timeout settings for all four types of validation webhooks in the ATC (Air Traffic Controller) system. Previously, timeouts were hardcoded or only partially configurable. Now each webhook type can have its own timeout configuration.
Changes
New Configuration Fields
Added four new timeout configuration fields to the installer:
AirwayValidationWebhookTimeout- Timeout for airway instance validation webhooks (default: 10 seconds)ResourceValidationWebhookTimeout- Timeout for resource/event dispatching validation webhooks (default: 5 seconds)ExternalResourceValidationWebhookTimeout- Timeout for external resource validation webhooks (default: 30 seconds)FlightValidationWebhookTimeout- Timeout for flight validation webhooks (default: 30 seconds)Files Modified
cmd/atc-installer/installer/run.goConfigstructcmd/atc/config.goConfigstructconf.Varcmd/atc/resources.goTimeoutSecondsfield inflightValidationwebhookTesting
Backward Compatibility
All timeout fields default to their previous hardcoded values if not explicitly configured, ensuring backward compatibility.