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

Add primary UDN net-attach-def #1841

Merged
merged 2 commits into from
Aug 8, 2024
Merged

Add primary UDN net-attach-def #1841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
510fb09
pkg/components: Add net-attach-def permissions
RamLavi Aug 6, 2024
d59c852
network/kubevirt-ipam-controller: Add new net-attach-def resource
RamLavi Aug 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
16 changes: 16 additions & 0 deletions data/kubevirt-ipam-controller/004-primary-udn-kubevirt-binding-networkattachdef.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
name: primary-udn-kubevirt-binding
namespace: default
spec:
config: '{
"cniVersion": "1.0.0",
"name": "primary-udn-kubevirt-binding",
"plugins": [
{
"type": "cni-passt-binding-plugin"
}
]
}'
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ require (
github.com/go-git/go-git/v5 v5.11.0
github.com/gobwas/glob v0.2.3
github.com/google/go-github/v32 v32.1.0
github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.3.0
github.com/kubevirt/monitoring/pkg/metrics/parser v0.0.0-20231024120544-6a3ba1a680b4
github.com/machadovilaca/operator-observability v0.0.19-0.20240326121036-9f2e5a31675f
github.com/onsi/ginkgo/v2 v2.11.0
Expand Down Expand Up @@ -144,7 +145,6 @@ require (
github.com/josharian/intern v1.0.0 // indirect
github.com/jpillora/backoff v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/k8snetworkplumbingwg/network-attachment-definition-client v1.3.0 // indirect
github.com/kevinburke/rest v0.0.0-20210506044642-5611499aa33c // indirect
github.com/kevinburke/ssh_config v1.2.0 // indirect
github.com/klauspost/compress v1.16.0 // indirect
Expand Down
2 changes: 1 addition & 1 deletion hack/components/bump-kubevirt-ipam-controller.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ echo 'Adjust kubevirt-ipam-controller to CNAO'

echo 'Copy manifests'
shopt -s extglob
rm -rf data/kubevirt-ipam-controller/!(002-rbac.yaml)
rm -rf data/kubevirt-ipam-controller/!(002-rbac.yaml|004-primary-udn-kubevirt-binding-networkattachdef.yaml)

# CRD
crd_manifest="https://raw.githubusercontent.com/k8snetworkplumbingwg/ipamclaims/${IPAMCLAIMS_CRD_VERSION}/artifacts/k8s.cni.cncf.io_ipamclaims.yaml"
Expand Down
3 changes: 3 additions & 0 deletions pkg/components/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,9 @@ func GetClusterRole(allowMultus bool) *rbacv1.ClusterRole {
"get",
"list",
"watch",
"create",
Copy link
Collaborator

@oshoval oshoval Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

one thing we can consider
limit this to specific NS (i.e default atm)
or even better - use it as Role instead ClusterRole
well for that we need a Role / RoleBinding in the default NS (not that this is risky a bit with backward compatibility once the NS is changed - for this reason we can postpone thinking of it and keep it ClusterRole atm)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thoughts exactly.. we can decide to use more specifc RBAC when we decide what the final resting place of this NAD

"update",
Copy link
Collaborator

@oshoval oshoval Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we need update ? (maybe the reconcile use it when it recreate?)

please consider moving it under the else of allowMultus
otherwise there are duplicate / overlapping rbacs due to multusClusterRoles in U/S, which i tried to refrain from (well indeed on some places like get list watch i did add, because it was just for NAD and needed on both U/S and D/S, so whatever you prefer)

you can move the flag into multusClusterRoles and it will determine if to add * or to add just the diff needed please

Copy link
Collaborator Author

@RamLavi RamLavi Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

do we need update ? (maybe the reconcile use it when it recreate?)

We do.

please consider moving it under the else of allowMultus otherwise there are duplicate / overlapping rbacs due to multusClusterRoles in U/S, which i tried to refrain from

you can move the flag into multusClusterRoles and it will determine if to add * or to add just the diff needed please

OK, DONE

"delete",
},
},
},
Expand Down
45 changes: 45 additions & 0 deletions pkg/network/kubevirt_ipam_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package network

import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

osv1 "github.com/openshift/api/operator/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"

cnao "github.com/kubevirt/cluster-network-addons-operator/pkg/apis/networkaddonsoperator/shared"
)

var _ = Describe("Testing kubevirt ipam controller", func() {
Context("Render KubevirtIpamController", func() {
conf := &cnao.NetworkAddonsConfigSpec{ImagePullPolicy: v1.PullAlways, Multus: &cnao.Multus{}, KubevirtIpamController: &cnao.KubevirtIpamController{}, PlacementConfiguration: &cnao.PlacementConfiguration{Workloads: &cnao.Placement{}}}
manifestDir := "../../data"
openshiftNetworkConf := &osv1.Network{}
clusterInfo := &ClusterInfo{}
expectedGroupVersionKind := schema.GroupVersionKind{
Group: "k8s.cni.cncf.io",
Version: "v1",
RamLavi marked this conversation as resolved.
Show resolved Hide resolved
Kind: "NetworkAttachmentDefinition",
}
const expectedName = "primary-udn-kubevirt-binding"

It("should add the primary-udn network-attach-def obj", func() {
objs, err := Render(conf, manifestDir, openshiftNetworkConf, clusterInfo)
Expect(err).NotTo(HaveOccurred())
Expect(objs).NotTo(BeEmpty())

Expect(objs).To(ContainElement(
SatisfyAll(
WithTransform(func(obj *unstructured.Unstructured) string {
return obj.GetName()
}, Equal(expectedName)),
WithTransform(func(obj *unstructured.Unstructured) schema.GroupVersionKind {
return obj.GetObjectKind().GroupVersionKind()
}, Equal(expectedGroupVersionKind)),
),
))
})
})
})
36 changes: 36 additions & 0 deletions test/check/check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
. "github.com/onsi/gomega"

monitoringv1 "github.com/coreos/prometheus-operator/pkg/apis/monitoring/v1"
k8snetworkplumbingwgv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
testenv "github.com/kubevirt/cluster-network-addons-operator/test/env"
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
securityapi "github.com/openshift/origin/pkg/security/apis/security"
Expand Down Expand Up @@ -346,6 +347,10 @@ func checkForComponent(component *Component) error {
errsAppend(checkForPrometheusRule(component.PrometheusRule))
}

if component.NetworkAttachmentDefinition != "" {
errsAppend(checkForNetworkAttachmentDefinition(component.NetworkAttachmentDefinition))
}

return errsToErr(errs)
}

Expand Down Expand Up @@ -389,6 +394,10 @@ func checkForComponentRemoval(component *Component) error {
errsAppend(checkForPrometheusRuleRemoval(component.PrometheusRule))
}

if component.NetworkAttachmentDefinition != "" {
errsAppend(checkForNetworkAttachmentDefinitionRemoval(component.NetworkAttachmentDefinition))
}

Comment on lines +397 to +400
Copy link
Collaborator

@oshoval oshoval Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The condition can be moved into the function
but we can consider please doing it on a follow-up for all those methods not in this PR

same for the checkForNetworkAttachmentDefinition family

Copy link
Collaborator Author

@RamLavi RamLavi Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

but we can consider doing it on a follow-up for all those methods not in this PR

+1

return errsToErr(errs)
}

Expand Down Expand Up @@ -649,6 +658,21 @@ func checkForPrometheusRule(name string) error {
return nil
}

func checkForNetworkAttachmentDefinition(name string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

but why we need all this logic if we just create the nad and if it doesnt exists we have the error that nad doesnt exists without any additional changes?

is it a must because it was added here ?
https://github.com/kubevirt/cluster-network-addons-operator/pull/1841/files#diff-dd3d0d54317e754fd95de914de01a73b5578626b42e755fdad4998186d552600R90

Copy link
Collaborator Author

@RamLavi RamLavi Aug 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

but why we need all this logic if we just create the nad

This code is for CNAO CI to make sure the NAD is deployed. It's now CNAO's responsibility, so CI should check it's deployed, and also removed.

and if it doesnt exists we have the error that nad doesnt exists without any additional changes?

small correction - only if the NAD's CRD doesn't exists then CNAO will fail to deploy with the error you mention.

is it a must because it was added here ? https://github.com/kubevirt/cluster-network-addons-operator/pull/1841/files#diff-dd3d0d54317e754fd95de914de01a73b5578626b42e755fdad4998186d552600R90

technically yeah, but it's intentional. As I mentioned above - it's CNAO's job to make sure the resources it is in charge of deploying are deployed.

networkAttachmentDefinition := k8snetworkplumbingwgv1.NetworkAttachmentDefinition{}
err := testenv.Client.Get(context.Background(), types.NamespacedName{Name: name, Namespace: corev1.NamespaceDefault}, &networkAttachmentDefinition)
if err != nil {
return err
}
Comment on lines +663 to +666
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use
if ...; err != nil {
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a cosmetic change, that should be done to all of the checkForX functions above. Let's do it in a separate refactor PR that does not change logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well this is a new function, but i wont insist

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 will make the change in a separate PR after this one is merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure thx


err = checkRelationshipLabels(networkAttachmentDefinition.GetLabels(), "NetworkAttachmentDefinition", name)
if err != nil {
return err
}

return nil
Comment on lines +669 to +673
Copy link
Collaborator

@oshoval oshoval Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

here you can just return err basically, but one can consider it less readable (imho it is fine fwiw)
so either this please or short writing

if ...; err != nil {
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same

}

func checkRelationshipLabels(labels map[string]string, kind, name string) error {
expectedValues := map[string]string{
names.COMPONENT_LABEL_KEY: names.COMPONENT_LABEL_DEFAULT_VALUE,
Expand Down Expand Up @@ -708,6 +732,14 @@ func checkForPrometheusRuleRemoval(name string) error {
return isNotFound("PrometheusRule", name, err)
}

func checkForNetworkAttachmentDefinitionRemoval(name string) error {
err := testenv.Client.Get(context.Background(), types.NamespacedName{Name: name, Namespace: corev1.NamespaceDefault}, &k8snetworkplumbingwgv1.NetworkAttachmentDefinition{})
if isKindNotFound(err) {
return nil
}
return isNotFound("NetworkAttachmentDefinition", name, err)
}

func getMonitoringEndpoint() (*corev1.Endpoints, error) {
By("Finding CNAO prometheus endpoint")
endpoint := &corev1.Endpoints{}
Expand Down Expand Up @@ -812,6 +844,10 @@ func isNotSupportedKind(err error) bool {
return strings.Contains(err.Error(), "no kind is registered for the type")
}

func isKindNotFound(err error) bool {
return strings.Contains(err.Error(), "no matches for kind")
}

func configToYaml(gvk schema.GroupVersionKind) string {
config := GetConfig(gvk)
manifest, err := yaml.Marshal(config)
Expand Down
12 changes: 7 additions & 5 deletions test/check/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type Component struct {
Service string
ServiceMonitor string
PrometheusRule string
NetworkAttachmentDefinition string
}

var (
Expand Down Expand Up @@ -87,11 +88,12 @@ var (
Deployments: []string{"secondary-dns"},
}
KubevirtIpamController = Component{
ComponentName: "KubevirtIpamController",
ClusterRole: "kubevirt-ipam-controller-manager-role",
ClusterRoleBinding: "kubevirt-ipam-controller-manager-rolebinding",
Deployments: []string{"kubevirt-ipam-controller-manager"},
DaemonSets: []string{"passt-binding-cni"},
ComponentName: "KubevirtIpamController",
ClusterRole: "kubevirt-ipam-controller-manager-role",
ClusterRoleBinding: "kubevirt-ipam-controller-manager-rolebinding",
Deployments: []string{"kubevirt-ipam-controller-manager"},
DaemonSets: []string{"passt-binding-cni"},
NetworkAttachmentDefinition: "primary-udn-kubevirt-binding",
}
AllComponents = []Component{
KubeMacPoolComponent,
Expand Down
3 changes: 2 additions & 1 deletion test/e2e/workflow/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ var _ = Describe("NetworkAddonsConfig", func() {
Entry(
KubevirtIpamController.ComponentName,
cnao.NetworkAddonsConfigSpec{
Multus: &cnao.Multus{},
KubevirtIpamController: &cnao.KubevirtIpamController{},
},
[]Component{KubevirtIpamController},
[]Component{MultusComponent, KubevirtIpamController},
),
)
It("should deploy prometheus if NetworkAddonsConfigSpec is not empty", func() {
Expand Down
3 changes: 3 additions & 0 deletions test/env/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package env

import (
k8snetworkplumbingwgv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
. "github.com/onsi/gomega"

monitoringv1 "github.com/coreos/prometheus-operator/pkg/apis/monitoring/v1"
Expand Down Expand Up @@ -39,6 +40,8 @@ func Start() {
ExpectWithOffset(1, err).NotTo(HaveOccurred())
err = monitoringv1.AddToScheme(scheme.Scheme)
ExpectWithOffset(1, err).NotTo(HaveOccurred())
err = k8snetworkplumbingwgv1.AddToScheme(scheme.Scheme)
ExpectWithOffset(1, err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme

Expand Down
Loading