-
Notifications
You must be signed in to change notification settings - Fork 12
Add support for static domain-to-IP mapping in k8s_dns_server for DNSChaos #31
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ajoy-zero The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: xiejunqiao <[email protected]>
Signed-off-by: xiejunqiao <[email protected]>
log.Debugf("records: %v, err: %v", records, err) | ||
|
||
if k.needChaos(chaosPod, records, state.QName()) { | ||
if k.needChaos(chaosPod, records, state.QName()) && chaosPod.Action != ActionStatic { |
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.
Are there any considerations not to move the logic of chaosPod.Action != ActionStatic
into the k.needChaos
?
Theoretically, we need to handle this part of the logic in the function to keep the code clean.
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.
The reason for not moving the chaosPod. Action != ActionStatic logic into k.needChaos() is to avoid changing the original logic, which might introduce unexpected issues or affect existing behavior. Since this part of the code has been stable, I chose to keep the separation to ensure functional consistency and reduce the risk of regression.
Once the overall behavior is verified to be stable, we can consider refactoring this logic into k.needChaos() in a follow-up PR to improve code clarity.
chaos.go
Outdated
} | ||
|
||
// DomainIP Domain and ip mapping | ||
type DomainIP struct { |
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.
This type is not used?
pb/dns.proto
Outdated
string msg = 2; | ||
} | ||
|
||
//定义ip 域名的映射关系 |
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.
Please do not use Chinese as comments.
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.
Thanks for pointing it out — my apologies for including Chinese in the comments. I've removed it and updated the code accordingly.
kubernetes.go
Outdated
|
||
ipPodMap map[string]*PodInfo | ||
|
||
domainAndIPMap map[string]map[string]map[string]string |
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.
Personally, I think it would be better to change the name to domainIPMapByNamespacedName
. Just looking at the data structure, I can't immediately know what it includes, but changing the name would help with understanding.
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.
Thanks for the suggestion — you're absolutely right. The current name domainAndIPMap is too vague, and domainIPMapByNamespacedName
makes the structure much clearer. I've updated the name accordingly.
- Removed unused IpDomainMap type - Replaced Chinese comment with English in IpDomainMap - Renamed domainAndIPMap to domainIPMapByNamespacedName for better clarity Signed-off-by: xiejunqiao <[email protected]>
Really appreciate your suggestions, and apologies for the issues in the original commit. I've made the following changes based on your comments: Removed the unused IpDomainMap type Replaced the Chinese comment with an English one Renamed the variable to domainIPMapByNamespacedName for better readability |
Signed-off-by: Yue Yang <[email protected]>
Backend parses the input into a JSON object {"domain": "google.com", "ip": "1.2.3.4"}.
The parsed mapping is sent to k8s_dns_server for storage and DNS fault injection.
For a given domain, the corresponding IP is returned based on the static mapping.
This change allows static IP resolution for specified domains during fault injection.
manual testing