feat: Add custom DNS and pod annotations support #548
feat: Add custom DNS and pod annotations support #548labibfrag wants to merge 5 commits intohyperledger-labs:mainfrom
Conversation
Signed-off-by: Labib Farag <labib.farag@ibm.com>
|
Hi @labibfrag, thanks for the pull request! At first glance it looks great- I'll check it out properly and hopefully get it merged when I'm back at work after Easter. Thanks again! |
There was a problem hiding this comment.
Hi @labibfrag, thanks again for the PR.
The main thing I've spotted looking through the code is that there's no validation for the two new variables, which would then need testing in cmd/run/main_test.go
It would also be nice to add an integration test or two in test/integration/testdata
Before doing that though, I'd also like to discuss a bit more about the background to the PR since there is no related open issue.
The example settings you included were a great clue, thank you, and I think they point to a potential problem. From what I can tell istio will be looking for labels, not annotations, in which case this PR might not do what you need it to.
The builder also uses Jobs which could potentially cause some complexity, although reading https://istio.io/latest/blog/2023/native-sidecars/ does sound promising. Also, there was an example in there that makes me think that custom labels may not be necessary. For example if FABRIC_K8S_BUILDER_NAMESPACE is set to hlf-chaincode, then this might work without any additional changes...
kubectl label namespace hlf-chaincode istio-injection=enabled
Or maybe even...
kubectl label namespace hlf-chaincode istio.io/dataplane-mode=ambient
I was less sure about why the nameserver changes were required. Is that also istio related, or something related to accessing chaincode images? Reading https://kubernetes.io/docs/tasks/administer-cluster/dns-custom-nameservers/ I was wondering whether this kind of thing would be useful but that's just wild speculation!
chaincode.local:53 {
errors
cache 30
forward . 10.150.0.1
}
Anyway, if there are ways to make use of the builder in a service mesh without any changes, that would be ideal. While I'm keen to minimise k8s config in the builder, if there are things you can't achieve without changes, e.g. custom labels rather than annotations, this PR is a great start!
|
|
||
| func getNameServers(logger *log.CmdLogger) string { | ||
| nameServers := util.GetOptionalEnv(util.NameServersVariable, "") | ||
| logger.Debugf("%s=%s", util.NameServersVariable, nameServers) |
There was a problem hiding this comment.
There should be some validation for the nameserver variable. I think it's something like up to 3 IP addresses but I don't know for sure/the details.
There was a problem hiding this comment.
yes it can be 3 servers
nameservers: a list of IP addresses that will be used as DNS servers for the Pod. There can be at most 3 IP addresses specified.
but it's admin responsibility to identify the servers , we can add validation / Warning about length
do you agree ?
| annotationsStr := util.GetOptionalEnv(util.CustomAnnotationsVariable, "") | ||
| logger.Debugf("%s=%s", util.CustomAnnotationsVariable, annotationsStr) | ||
|
|
||
| annotations := util.ParseAnnotations(annotationsStr) |
There was a problem hiding this comment.
There should be some validation for the annotations somewhere. The builder does label validation for example.
There was a problem hiding this comment.
what type of validation we can add for this annotation ?
|
Can this PR be closed now that #553 is open? |
|
Aha, thanks, glad I checked! :) |
3 participants
feat: Add custom DNS and pod annotations support
Add support for custom DNS nameservers and pod annotations via environment variables to enable advanced Kubernetes configurations like service mesh integration.
Features:
Changes:
Usage:
When FABRIC_K8S_BUILDER_NAME_SERVERS is set, pods use dnsPolicy: None with the specified nameserver. When empty, default Kubernetes DNS is used.
Custom annotations are merged with existing chaincode pod annotations, enabling integration with service meshes like Istio and monitoring tools like Prometheus.