Skip to content
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.

Sign up for GitHub

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

Support primary network #36

Merged
merged 7 commits into from
Sep 5, 2024
Merged

Support primary network #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc937da
config: Add 'role' field to config
qinqon Jul 4, 2024
6ba3618
udn: Add FindPrimaryNetwork helper
qinqon Jul 4, 2024
3b5970b
webhook: Check first vm annotation
qinqon Jul 4, 2024
fba614f
types: Add OVN annotation
qinqon Jul 4, 2024
71dfd6c
ipamclaim: Add key composer
qinqon Jul 4, 2024
d008090
webhook: Annotate with primary network ipamclaim
qinqon Jul 4, 2024
bdabec2
vm, controller: Create IPAMClaim for primary
qinqon Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/claims/claims.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ func OwnedByVMLabel(vmiName string) client.MatchingLabels {
virtv1.VirtualMachineLabel: vmiName,
}
}

func ComposeKey(vmName, networkName string) string {
return fmt.Sprintf("%s.%s", vmName, networkName)
}
13 changes: 11 additions & 2 deletions pkg/config/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,18 @@ import (
"fmt"
)

type NetworkRole string

const (
NetworkRolePrimary NetworkRole = "primary"
)

const OVNPrimaryNetworkIPAMClaimAnnotation = "k8s.ovn.org/ovn-udn-ipamclaim-reference"

type RelevantConfig struct {
Name string `json:"name"`
AllowPersistentIPs bool `json:"allowPersistentIPs,omitempty"`
Name string `json:"name"`
AllowPersistentIPs bool `json:"allowPersistentIPs,omitempty"`
Role NetworkRole `json:"role,omitempty"`
}

func NewConfig(nadSpec string) (*RelevantConfig, error) {
Expand Down
253 changes: 166 additions & 87 deletions pkg/ipamclaimswebhook/podmutator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ import (

virtv1 "kubevirt.io/api/core/v1"

"github.com/kubevirt/ipam-extensions/pkg/claims"
"github.com/kubevirt/ipam-extensions/pkg/config"
"github.com/kubevirt/ipam-extensions/pkg/udn"
)

// +kubebuilder:webhook:path=/mutate-v1-pod,mutating=true,failurePolicy=fail,groups="",resources=pods,verbs=create;update,versions=v1,name=ipam-claims.k8s.cni.cncf.io,admissionReviewVersions=v1,sideEffects=None
Expand All @@ -66,101 +68,52 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request)
}

log.Info("webhook handling event")
qinqon marked this conversation as resolved.
Show resolved Hide resolved

vmName, hasVMAnnotation := pod.Annotations["kubevirt.io/domain"]
qinqon marked this conversation as resolved.
Show resolved Hide resolved
if !hasVMAnnotation {
log.Info(
"does not have the kubevirt VM annotation",
)
return admission.Allowed("not a VM")
}
qinqon marked this conversation as resolved.
Show resolved Hide resolved

networkSelectionElements, err := netutils.ParsePodNetworkAnnotation(pod)
if err != nil {
var goodTypeOfError *v1.NoK8sNetworkError
if errors.As(err, &goodTypeOfError) {
return admission.Allowed("no secondary networks requested")
if !errors.As(err, &goodTypeOfError) {
return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse pod network selection elements"))
}
return admission.Errored(http.StatusBadRequest, fmt.Errorf("failed to parse pod network selection elements"))
}

var (
hasChangedNetworkSelectionElements bool
podNetworkSelectionElements = make([]v1.NetworkSelectionElement, 0, len(networkSelectionElements))
)
for _, networkSelectionElement := range networkSelectionElements {
nadName := types.NamespacedName{
Namespace: networkSelectionElement.Namespace,
Name: networkSelectionElement.Name,
}.String()
log.Info(
"iterating network selection elements",
"NAD", nadName,
)
nadKey := types.NamespacedName{
Namespace: networkSelectionElement.Namespace,
Name: networkSelectionElement.Name,
}

nad := v1.NetworkAttachmentDefinition{}
if err := a.Client.Get(context.Background(), nadKey, &nad); err != nil {
if k8serrors.IsNotFound(err) {
return admission.Allowed("NAD not found, will hang on scheduler")
}
return admission.Errored(http.StatusInternalServerError, err)
var newPod *corev1.Pod
hasChangedNetworkSelectionElements, err :=
ensureIPAMClaimRefAtNetworkSelectionElements(ctx, a.Client, pod, vmName, networkSelectionElements)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}
if hasChangedNetworkSelectionElements {
if newPod == nil {
newPod = pod.DeepCopy()
}

pluginConfig, err := config.NewConfig(nad.Spec.Config)
if err != nil {
if err := updatePodSelectionElements(newPod, networkSelectionElements); err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}

if pluginConfig.AllowPersistentIPs {
log.Info(
"will request persistent IPs",
"NAD", nadName,
"network", pluginConfig.Name,
)
vmName, hasVMAnnotation := pod.Annotations["kubevirt.io/domain"]
if !hasVMAnnotation {
log.Info(
"does not have the kubevirt VM annotation",
"NAD", nadName,
"network", pluginConfig.Name,
)
return admission.Allowed("not a VM")
}

vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName}
vmi := &virtv1.VirtualMachineInstance{}
if err := a.Client.Get(context.Background(), vmKey, vmi); err != nil {
return admission.Errored(http.StatusInternalServerError, err)
}

vmiNets := vmiSecondaryNetworks(vmi)
networkName, foundNetworkName := vmiNets[nadKey.String()]
if !foundNetworkName {
log.Info(
"network name not found",
"NAD", nadName,
"network", networkName,
)
podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement)
continue
}

networkSelectionElement.IPAMClaimReference = fmt.Sprintf("%s.%s", vmName, networkName)
log.Info(
"requesting claim",
"NAD", nadName,
"network", pluginConfig.Name,
"claim", networkSelectionElement.IPAMClaimReference,
)
podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement)
hasChangedNetworkSelectionElements = true
continue
}
podNetworkSelectionElements = append(podNetworkSelectionElements, *networkSelectionElement)
}

if len(podNetworkSelectionElements) > 0 {
newPod, err := podWithUpdatedSelectionElements(pod, podNetworkSelectionElements)
if err != nil {
return admission.Errored(http.StatusInternalServerError, err)
newPrimaryNetworkIPAMClaimName, err := findNewPrimaryNetworkIPAMClaimName(ctx, a.Client, pod, vmName)
if err != nil {
return admission.Errored(http.StatusInternalServerError,
fmt.Errorf("failed looking for primary user defined IPAMClaim name: %v", err))
}
if newPrimaryNetworkIPAMClaimName != "" {
if newPod == nil {
newPod = pod.DeepCopy()
}
updatePodWithOVNPrimaryNetworkIPAMClaimAnnotation(newPod, newPrimaryNetworkIPAMClaimName)
}

if reflect.DeepEqual(newPod, pod) || !hasChangedNetworkSelectionElements {
if newPod != nil {
if reflect.DeepEqual(newPod, pod) {
return admission.Allowed("mutation not needed")
}

Expand All @@ -172,6 +125,7 @@ func (a *IPAMClaimsValet) Handle(ctx context.Context, request admission.Request)
log.Info("new pod annotations", "pod", newPod.Annotations)
return admission.PatchResponseFromRaw(request.Object.Raw, marshaledPod)
}

return admission.Allowed("carry on")
}

Expand All @@ -196,12 +150,137 @@ func vmiSecondaryNetworks(vmi *virtv1.VirtualMachineInstance) map[string]string
return indexedSecondaryNetworks
}

func podWithUpdatedSelectionElements(pod *corev1.Pod, networks []v1.NetworkSelectionElement) (*corev1.Pod, error) {
newPod := pod.DeepCopy()
func updatePodSelectionElements(pod *corev1.Pod, networks []*v1.NetworkSelectionElement) error {
newNets, err := json.Marshal(networks)
if err != nil {
return nil, err
return err
}
pod.Annotations[v1.NetworkAttachmentAnnot] = string(newNets)
return nil
}

func updatePodWithOVNPrimaryNetworkIPAMClaimAnnotation(pod *corev1.Pod, primaryNetworkIPAMClaimName string) {
pod.Annotations[config.OVNPrimaryNetworkIPAMClaimAnnotation] = primaryNetworkIPAMClaimName
}
qinqon marked this conversation as resolved.
Show resolved Hide resolved

func ensureIPAMClaimRefAtNetworkSelectionElements(ctx context.Context,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really dislike the fact this returns 3 things.

And the fact we use this response parameter just to indicate something that IMO looks like an error is very puzzling to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it either, but I was trying to maintain the original behaviour where is returning NAD not found, will hang on scheduler if there is no nad for the NSE, we can just return nil, nil but logging it and the webhook response will be "carry on"

Copy link
Contributor

@RamLavi RamLavi Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you return a new error type like "NoNadIgnoreReqest" and expect that case outside of ensureIPAMClaimRefAtNetworkSelectionElements, returning the AdmissionAllowed

var ErrNoNadIgnoreRequest = errors.New("NAD not found, will hang on scheduler")

on hasChangedNetworkSelectionElements function:

if k8serrors.IsNotFound(err) {
  return ErrNoNadIgnoreRequest
}

then outside of it:

hasChangedNetworkSelectionElements, err :=
    ensureIPAMClaimRefAtNetworkSelectionElements(ctx, a.Client, pod, vmName, networkSelectionElements)
if err != nil {
    if errors.Is(err, ErrNoNadIgnoreRequest) {
        return admission.Allowed(ErrNoNadIgnoreRequest.Error())
    }
    return admission.Errored(http.StatusInternalServerError, err)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you return a new error type like "NoNadIgnoreReqest" and expect that case outside of ensureIPAMClaimRefAtNetworkSelectionElements, returning the AdmissionAllowed

var ErrNoNadIgnoreRequest = errors.New("NAD not found, will hang on scheduler")

on hasChangedNetworkSelectionElements function:

if k8serrors.IsNotFound(err) {
  return ErrNoNadIgnoreRequest
}

then outside of it:

hasChangedNetworkSelectionElements, err :=
    ensureIPAMClaimRefAtNetworkSelectionElements(ctx, a.Client, pod, vmName, networkSelectionElements)
if err != nil {
    if errors.Is(err, ErrNoNadIgnoreRequest) {
        return admission.Allowed(ErrNoNadIgnoreRequest.Error())
    }
    return admission.Errored(http.StatusInternalServerError, err)
}

Puff I think this is overkill just to return something at the webhook success, I want to know if returning that specific string is really necessary or logging it is enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, returning bool, error also makes me frown...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, simplified since we don't care about th emssage at webhook.Allowed

Copy link
Collaborator

@maiqueb maiqueb Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no sure you saw it and chose to ignore it, or just didn't see it.

hm, returning bool, error also makes me frown...

Can we try to improve this function's signature ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What the issue with that ? is a function that try to update a map and return if the map was updated or not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do the exact same thing with just error, or returning the map and letting the client understand if it changed or not.

if your function does "update a map and return if the map was updated or not" your function is doing 2 things. The function should do one. Update the map.

We can improve it later on if this is the only item under question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openned an issue to simplify things later on #63

cli client.Client, pod *corev1.Pod, vmName string,
networkSelectionElements []*v1.NetworkSelectionElement) (changed bool, err error) {
log := logf.FromContext(ctx)
hasChangedNetworkSelectionElements := false
for i, networkSelectionElement := range networkSelectionElements {
nadName := fmt.Sprintf("%s/%s", networkSelectionElement.Namespace, networkSelectionElement.Name)
log.Info(
"iterating network selection elements",
"NAD", nadName,
)
nadKey := types.NamespacedName{
Namespace: networkSelectionElement.Namespace,
Name: networkSelectionElement.Name,
}

nad := v1.NetworkAttachmentDefinition{}
if err := cli.Get(context.Background(), nadKey, &nad); err != nil {
if k8serrors.IsNotFound(err) {
log.Info("NAD not found, will hang on scheduler", "NAD", nadName)
return false, nil
}
return false, err
}

pluginConfig, err := config.NewConfig(nad.Spec.Config)
if err != nil {
return false, err
}

if !pluginConfig.AllowPersistentIPs {
continue
}

log.Info(
"will request persistent IPs",
"NAD", nadName,
"network", pluginConfig.Name,
)

vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName}
vmi := &virtv1.VirtualMachineInstance{}
if err := cli.Get(context.Background(), vmKey, vmi); err != nil {
return false, err
}

vmiNets := vmiSecondaryNetworks(vmi)
networkName, foundNetworkName := vmiNets[nadKey.String()]
if !foundNetworkName {
log.Info(
"network name not found",
"NAD", nadName,
"network", networkName,
)
continue
}

networkSelectionElements[i].IPAMClaimReference = claims.ComposeKey(vmName, networkName)
log.Info(
"requesting claim",
"NAD", nadName,
"network", pluginConfig.Name,
"claim", networkSelectionElement.IPAMClaimReference,
)
hasChangedNetworkSelectionElements = true
continue
}
return hasChangedNetworkSelectionElements, nil
}

func findNewPrimaryNetworkIPAMClaimName(ctx context.Context,
cli client.Client, pod *corev1.Pod, vmName string) (string, error) {
log := logf.FromContext(ctx)
if pod.Annotations[config.OVNPrimaryNetworkIPAMClaimAnnotation] != "" {
return "", nil
}
primaryNetworkNAD, err := udn.FindPrimaryNetwork(ctx, cli, pod.Namespace)
if err != nil {
return "", err
}
if primaryNetworkNAD == nil {
return "", nil
}
qinqon marked this conversation as resolved.
Show resolved Hide resolved
pluginConfig, err := config.NewConfig(primaryNetworkNAD.Spec.Config)
if err != nil {
return "", err
}

if !pluginConfig.AllowPersistentIPs {
return "", nil
}

log.Info(
"will request primary network persistent IPs",
"NAD", client.ObjectKeyFromObject(primaryNetworkNAD),
"network", pluginConfig.Name,
)
vmKey := types.NamespacedName{Namespace: pod.Namespace, Name: vmName}
vmi := &virtv1.VirtualMachineInstance{}
if err := cli.Get(context.Background(), vmKey, vmi); err != nil {
return "", err
}

networkName := vmiPodNetworkName(vmi)
if networkName == "" {
log.Info("vmi has no pod network primary UDN ipam claim will not be created", "vmi", vmKey.String())
return "", nil
}

return claims.ComposeKey(vmi.Name, networkName), nil
}

// returns the name of the kubevirt VM pod network
func vmiPodNetworkName(vmi *virtv1.VirtualMachineInstance) string {
for _, network := range vmi.Spec.Networks {
if network.Pod != nil {
return network.Name
}
}
newPod.Annotations[v1.NetworkAttachmentAnnot] = string(newNets)
return newPod, nil
return ""
}
Loading
Loading