feat: validation enforcing vnet guid is set for overlay cni clusters #675
Conversation
…d google uuid lib based on this benchmark test https://gist.github.com/mattes/69a4ab7027b9e8ee952b5843e7ca6955
…gers vnet guid validation, this modifies that test to pass in an empty string
| @@ -235,28 +223,3 @@ func getCABundle(restConfig *rest.Config) (*string, error) { | |||
| } | |||
| return ptr.String(base64.StdEncoding.EncodeToString(transportConfig.TLS.CAData)), nil | |||
| } | |||
|
|
|||
| func getVnetGUID(cfg *auth.Config, subnetID string) (string, error) { | |||
There was a problem hiding this comment.
Do we document our options anywhere?
Helm chart?
Website?
Go struct?
All of the above?
We should document what this is and how users should set it somewhere. I am also not 100% sure what the experience should be for selfhosted Karpenter. It might be OK to support getting the VNETGUID automatically if the user doesn't set it, but allowing them to set it to avoid the need to assign vnet/read permissions to standalone.
Obviously in the NAP case, VNET Guid will always be supplied.
There was a problem hiding this comment.
We don't have a great set of documented requirements here.
CC: @wdarko1 fyi
There was a problem hiding this comment.
I would prefer that we make it a fully managed field. VNET GUID unlike bootstrap token for example is rather static and only needed once and easy enough to resolve in our configure_values.sh script.
There are other fields that may require special treatment for self hosted like bootstrap token, but I don't think vnet guid needs to be one of them.
I would prefer to rid ourselves of that dependency.
There was a problem hiding this comment.
The options should be reflected in the chart values files - and documented at least there. Right now the list of values needs an update.
For self-hosted, the configure-values.sh serves as a bridge for generating configuration appropriate for a given cluster; the values template it is targeting is currently passing the options via environment variables, bypassing even chart values that exist. Ideally values serve as the chart "contract" - and the script/template should be updated to target values instead of environment variables.
There was a problem hiding this comment.
Yeah as far as OSS is concerned, parameters should flow from Helm values to the pod somehow. Everything needs to be configurable via values.yaml (for the chart) and we should at least put documentation into values.yaml that describes each option and what it does.
If that's not what we're doing now, should we either fix it here or create a workitem tracking it? I see that our values.yaml already does some of this. Another example is ASO
There was a problem hiding this comment.
I see what we're doing now is just using the broad values.yaml env overrides, which yeah is fine but ideally we'd use specific parameters with specific documentation (IMO).
| @@ -235,28 +223,3 @@ func getCABundle(restConfig *rest.Config) (*string, error) { | |||
| } | |||
| return ptr.String(base64.StdEncoding.EncodeToString(transportConfig.TLS.CAData)), nil | |||
| } | |||
|
|
|||
| func getVnetGUID(cfg *auth.Config, subnetID string) (string, error) { | |||
Description
After we roll out the v0.7.2 release, we can remove the fallback to the vnet client. This pr also introduces stricter validation to vnet guid to make it clear its required.
How was this change tested?
Does this change impact docs?
Release Note