Skip to content

CORENET-6130, CORENET-6261, CORENET-6092: Implement PreconfiguredUDNAddresses API changes #2743

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions bindata/network/multus-admission-controller/003-webhook.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ webhooks:
# On updates, only validate if the Spec changes
- name: CreateDeleteOrUpdatedSpec
expression: oldObject == null || object == null || has(object.spec) != has(oldObject.spec) || (has(object.spec) && object.spec != oldObject.spec)
{{if .OVN_PRE_CONF_UDN_ADDR_ENABLE}}
# Ignore default/openshift-ovn-kubernetes NAD to avoid a race between ovn-kubernetes and the multus webhook on install
- name: IgnoreDefaultOVNKubernetesNAD
expression: object == null || object.metadata.namespace != "openshift-ovn-kubernetes" || object.metadata.name != "default"
Copy link
Contributor

Choose a reason for hiding this comment

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

haha in my head i go:

ignore if:
object != null && object.metadata.namespace == "openshift-ovn-kubernetes" && object.metadata.name == "default"

but i see you did exact opp which is the same thing

Copy link
Contributor

Choose a reason for hiding this comment

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

i really hope we can't have a NAD with no namespace and metadata.namespace is never none haha like we hit the hitch with metadata.annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, I will double check that this doesn't happen.

{{- end }}
sideEffects: NoneOnDryRun
admissionReviewVersions:
- v1
Expand Down
211 changes: 211 additions & 0 deletions bindata/network/ovn-kubernetes/common/001-crd.yaml

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
{{if .OVN_PRE_CONF_UDN_ADDR_ENABLE}}
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicy
metadata:
name: default-network-annotation
spec:
matchConstraints:
resourceRules:
- apiGroups: [""]
apiVersions: ["v1"]
operations: ["UPDATE"]
resources: ["pods"]
failurePolicy: Fail
validations:
# Prevent any changes to the default-network annotation after pod creation:
# - If annotation exists in old pod: new pod must have same annotation with identical value
# - If annotation doesn't exist in old pod: new pod must also not have it
- expression: >
!has(object.metadata.annotations) || !has(oldObject.metadata.annotations) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

slight change here that is missing u/s as noted by @kyrtapz already in his comment on the PR. This fix will also go upstream.

(('v1.multus-cni.io/default-network' in oldObject.metadata.annotations)
? ('v1.multus-cni.io/default-network' in object.metadata.annotations) && oldObject.metadata.annotations['v1.multus-cni.io/default-network'] == object.metadata.annotations['v1.multus-cni.io/default-network']
: !('v1.multus-cni.io/default-network' in object.metadata.annotations))
message: "The 'v1.multus-cni.io/default-network' annotation cannot be changed after the pod was created"
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicyBinding
metadata:
name: default-network-annotation-binding
spec:
policyName: default-network-annotation
validationActions: [Deny]
matchResources:
resourceRules:
- apiGroups: [""]
apiVersions: ["v1"]
operations: ["UPDATE"]
resources: ["pods"]
{{end}}
6 changes: 5 additions & 1 deletion pkg/network/multus_admission_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import (
"github.com/openshift/cluster-network-operator/pkg/render"
"github.com/pkg/errors"

apifeatures "github.com/openshift/api/features"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
uns "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -54,7 +56,7 @@ func getOpenshiftNamespaces(client cnoclient.Client) (string, error) {
}

// renderMultusAdmissonControllerConfig returns the manifests of Multus Admisson Controller
func renderMultusAdmissonControllerConfig(manifestDir string, externalControlPlane bool, bootstrapResult *bootstrap.BootstrapResult, client cnoclient.Client, hsc *hypershift.HyperShiftConfig, clientName string) ([]*uns.Unstructured, error) {
func renderMultusAdmissonControllerConfig(manifestDir string, externalControlPlane bool, bootstrapResult *bootstrap.BootstrapResult, client cnoclient.Client, hsc *hypershift.HyperShiftConfig, clientName string, featureGates featuregates.FeatureGate) ([]*uns.Unstructured, error) {
objs := []*uns.Unstructured{}
var err error

Expand Down Expand Up @@ -83,6 +85,8 @@ func renderMultusAdmissonControllerConfig(manifestDir string, externalControlPla
data.Data["ResourceRequestCPU"] = nil
data.Data["ResourceRequestMemory"] = nil
data.Data["PriorityClass"] = nil
data.Data["OVN_PRE_CONF_UDN_ADDR_ENABLE"] = featureGates.Enabled(apifeatures.FeatureGatePreconfiguredUDNAddresses)

if hsc.Enabled {
data.Data["AdmissionControllerNamespace"] = hsc.Namespace
data.Data["KubernetesServiceHost"] = bootstrapResult.Infra.APIServers[bootstrap.APIServerDefaultLocal].Host
Expand Down
6 changes: 3 additions & 3 deletions pkg/network/multus_admission_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ func TestRenderMultusAdmissionController(t *testing.T) {
bootstrap := fakeBootstrapResult()

// disable MultusAdmissionController
objs, err := renderMultusAdmissionController(config, manifestDir, false, bootstrap, fakeClient)
objs, err := renderMultusAdmissionController(config, manifestDir, false, bootstrap, fakeClient, getDefaultFeatureGates())
g.Expect(err).NotTo(HaveOccurred())
g.Expect(objs).NotTo(ContainElement(HaveKubernetesID("Deployment", "openshift-multus", "multus-admission-controller")))

// enable MultusAdmissionController
enabled := false
config.DisableMultiNetwork = &enabled
objs, err = renderMultusAdmissionController(config, manifestDir, false, bootstrap, fakeClient)
objs, err = renderMultusAdmissionController(config, manifestDir, false, bootstrap, fakeClient, getDefaultFeatureGates())
g.Expect(err).NotTo(HaveOccurred())
g.Expect(objs).To(ContainElement(HaveKubernetesID("Deployment", "openshift-multus", "multus-admission-controller")))

Expand Down Expand Up @@ -143,7 +143,7 @@ func TestRenderMultusAdmissonControllerConfigForHyperShift(t *testing.T) {
hsc.ReleaseImage = "MyImage"
hsc.ControlPlaneImage = "MyCPOImage"

objs, err := renderMultusAdmissonControllerConfig(manifestDir, false, bootstrap, fakeClient, hsc, "")
objs, err := renderMultusAdmissonControllerConfig(manifestDir, false, bootstrap, fakeClient, hsc, "", getDefaultFeatureGates())
g.Expect(err).NotTo(HaveOccurred())

// Check rendered object
Expand Down
2 changes: 1 addition & 1 deletion pkg/network/ovn_kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4191,7 +4191,7 @@ func Test_renderOVNKubernetes(t *testing.T) {
client: cnofake.NewFakeClient(),
featureGates: preDefUDNFeatureGates,
},
expectNumObjs: 45,
expectNumObjs: 47,
Copy link
Contributor

Choose a reason for hiding this comment

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

ah the VAP and the role binding?

},
}
for _, tt := range tests {
Expand Down
6 changes: 3 additions & 3 deletions pkg/network/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func Render(operConf *operv1.NetworkSpec, clusterConf *configv1.NetworkSpec, man

// render MultusAdmissionController
o, err = renderMultusAdmissionController(operConf, manifestDir,
bootstrapResult.Infra.ControlPlaneTopology == configv1.ExternalTopologyMode, bootstrapResult, client)
bootstrapResult.Infra.ControlPlaneTopology == configv1.ExternalTopologyMode, bootstrapResult, client, featureGates)
if err != nil {
return nil, progressing, err
}
Expand Down Expand Up @@ -805,7 +805,7 @@ func getMultusAdmissionControllerReplicas(bootstrapResult *bootstrap.BootstrapRe
}

// renderMultusAdmissionController generates the manifests of Multus Admission Controller
func renderMultusAdmissionController(conf *operv1.NetworkSpec, manifestDir string, externalControlPlane bool, bootstrapResult *bootstrap.BootstrapResult, client cnoclient.Client) ([]*uns.Unstructured, error) {
func renderMultusAdmissionController(conf *operv1.NetworkSpec, manifestDir string, externalControlPlane bool, bootstrapResult *bootstrap.BootstrapResult, client cnoclient.Client, featureGates featuregates.FeatureGate) ([]*uns.Unstructured, error) {
if *conf.DisableMultiNetwork {
return nil, nil
}
Expand All @@ -815,7 +815,7 @@ func renderMultusAdmissionController(conf *operv1.NetworkSpec, manifestDir strin

hsc := hypershift.NewHyperShiftConfig()
objs, err := renderMultusAdmissonControllerConfig(manifestDir, externalControlPlane,
bootstrapResult, client, hsc, names.ManagementClusterName)
bootstrapResult, client, hsc, names.ManagementClusterName, featureGates)
if err != nil {
return nil, err
}
Expand Down
5 changes: 1 addition & 4 deletions pkg/network/render_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
. "github.com/onsi/gomega"
"github.com/openshift/cluster-network-operator/pkg/client/fake"
"github.com/openshift/cluster-network-operator/pkg/hypershift"
"github.com/openshift/library-go/pkg/operator/configobserver/featuregates"
"github.com/stretchr/testify/assert"
"k8s.io/client-go/kubernetes/scheme"

Expand Down Expand Up @@ -401,9 +400,7 @@ func TestRenderUnknownNetwork(t *testing.T) {
bootstrapResult, err := Bootstrap(&config, client)
g.Expect(err).NotTo(HaveOccurred())

featureGatesCNO := featuregates.NewFeatureGate([]configv1.FeatureGateName{}, []configv1.FeatureGateName{})

objs, _, err := Render(prev, &configv1.NetworkSpec{}, manifestDir, client, featureGatesCNO, bootstrapResult)
objs, _, err := Render(prev, &configv1.NetworkSpec{}, manifestDir, client, getDefaultFeatureGates(), bootstrapResult)
g.Expect(err).NotTo(HaveOccurred())

// Validate that openshift-sdn isn't rendered
Expand Down