Conversation
| // DefaultDNSServer is the default DNS server for libvirt. | ||
| DefaultDNSServer = "8.8.8.8" | ||
| // DefaultIfName is the default interface name for libvirt. | ||
| DefaultIfName = "tt0" |
There was a problem hiding this comment.
Let's get "tectonic" out of this - how about osbr0
There was a problem hiding this comment.
sounds good. I should have confirmed with you earlier, I just grabbed what was in defaulted in the Terraform config.
There was a problem hiding this comment.
at this point, it's completely arbitrary. oh, did you know that Linux interface names have no restrictions on characters? Let's just call it 🌉
| if c.Platform != PlatformLibvirt { | ||
| return errs | ||
| } | ||
| if err := validate.PrefixError("libvirt network ipRange", validate.SubnetCIDR(c.Libvirt.Network.IPRange)); err != nil { |
There was a problem hiding this comment.
It's not 100% necessary, but if you want to validate that this doesn't overlap with the cluster and service cidrs, that would be awesome!
installer/pkg/config/validate.go
Outdated
| if err := validate.PrefixError("libvirt uri", validate.NonEmpty(c.Libvirt.URI)); err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| if err := validate.PrefixError("libvirt imagePath", validate.NonEmpty(c.Libvirt.QOWImagePath)); err != nil { |
There was a problem hiding this comment.
Is it possible to validate that this is a valid path on disk? Even more amazing would be validating that the first four bytes match QFI\xFB
|
A few suggestions, but looks great! |
|
Thanks @squeed! I made the requested additions. It meant some refactoring of the CIDR comparison code so the PR grew a little bit. Please take another look when you can. |
|
I also replaced all occurrences of |
|
rebased again hehe |
This commit adds validation for all libvirt fields. Additionally, it moves the default values for these validated fields from Terraform to Golang so that the defaults are handled consistently and we do not accidentally omit configuration values by forgetting to plumb values through Terraform.
DefaultIfName was added in f245ca6 (installer: validate libvirt, 2018-06-06, coreos/tectonic-installer#3265), but the last consumer was removed by 619db92 (pkg/types/config: Break into pkg/validate and pkg/tfvars, 2018-10-03, openshift#412). We currently feed the default bridge name in via pkg/asset/installconfig/libvirt's defaultNetworkIfName. The AWS and OpenStack default variables I'm also removing here have a similar history and current default location.
3 participants
This commit adds validation for all libvirt fields. Additionally, it
moves the default values for these validated fields from Terraform to
Golang so that the defaults are handled consistently and we do not
accidentally omit configuration values by forgetting to plumb values
through Terraform.
Addresses: INST-1089
CC @enxebre @squeed
We need to set up our libvirt smoke tests so we can properly test future changes like this.