-
Notifications
You must be signed in to change notification settings - Fork 361
feat: add certificate conflict detection to admission webhooks #2603
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.
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
Conversation
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces certificate conflict detection to admission webhooks to prevent SSL/TLS configuration mismatches across Gateway, Ingress, and ApisixTls resources that share the same GatewayProxy. The feature ensures that when multiple resources configure TLS for the same hostname, they must use identical certificates to avoid conflicting SSL configurations in APISIX.
- Adds reusable SSL utility functions for certificate extraction and validation
- Implements a comprehensive conflict detector that validates SSL configurations across resource types
- Integrates conflict detection into admission webhooks for all three resource types
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
test/e2e/webhook/ssl_conflict.go | Comprehensive e2e tests covering conflict scenarios between resources |
internal/webhook/v1/ssl/conflict_detector_test.go | Unit tests for the conflict detection logic |
internal/webhook/v1/ssl/conflict_detector.go | Core conflict detection implementation |
internal/webhook/v1/ingress_webhook.go | Integration of SSL conflict detection into Ingress webhook |
internal/webhook/v1/gateway_webhook.go | Integration of SSL conflict detection into Gateway webhook |
internal/webhook/v1/apisixtls_webhook.go | Integration of SSL conflict detection into ApisixTls webhook |
internal/ssl/util.go | Reusable SSL utility functions for certificate handling |
internal/adc/translator/ingress.go | Updated to use new SSL utilities |
internal/adc/translator/gateway.go | Refactored to use new SSL utilities, removing duplicate code |
internal/adc/translator/apisixupstream.go | Updated to use new SSL utilities |
internal/adc/translator/apisixtls.go | Updated to use new SSL utilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license-eye has totally checked 349 files.
Valid | Invalid | Ignored | Fixed |
---|---|---|---|
252 | 1 | 96 | 0 |
Click to see the invalid file list
- internal/controller/indexer/ssl_host.go
Signed-off-by: Ashing Zheng <[email protected]>
// existing resources that are associated with the same GatewayProxy. Best-effort: | ||
// failures while enumerating existing resources or reading Secrets will be logged | ||
// and result in no conflicts instead of blocking the admission. | ||
func (d *ConflictDetector) DetectConflicts(ctx context.Context, obj client.Object, newMappings []HostCertMapping) ([]SSLConflict, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest adding this check:
if len(newMappings) == 0 { return }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error
unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
seen[mapping.Host] = mapping.CertificateHash | ||
} | ||
|
||
if len(seen) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conflicts > 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
Type of change:
What this PR does / why we need it:
When the TLS configuration of the Gateway/Ingress/ApisixTls resources has the same host but uses different certificates, we should intercept.
Pre-submission checklist: