-
Notifications
You must be signed in to change notification settings - Fork 30
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
Improve NetworkProfile configuration #29
Conversation
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'm not clear what this is fixing, is there an issue that goes with this?
pkg/aks/create.go
Outdated
} else { | ||
// Network Policy Azure can be used only with Network Plugin Azure CNI | ||
if containerservice.NetworkPolicy(to.String(spec.NetworkPolicy)) == containerservice.NetworkPolicyAzure { | ||
networkProfile.NetworkPolicy = containerservice.NetworkPolicyCalico |
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 don't think it's good practice to silently change a configured value behind the user's back like this. It should either be validated and an error returned to let the user correct it, or even just pass the invalid config to AKS and bubble up the error from there.
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.
@cmurphy This is fixing an issue with AKSv2 provisioning where the network configuration is not being set properly so the cluster gets deployed with no network policy enabled and the kubenet network plugin regardless of the options you select in the UI.
As for silently changing a configured value, I agree that it should be validated and rejected with an error or bubble up AKS' error since the UI has logic to make sure you can't select the Azure network policy without the Azure CNI already.
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.
Comments included, thank you
41def04
to
80fec30
Compare
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.
LGTM
No description provided.