Skip to content

Commit

Permalink
Merge pull request #1099 from rabi/network_val
Browse files Browse the repository at this point in the history 
Remove webhook validation for networks
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored Oct 16, 2024
2 parents b610b77 + 267d61a commit 478de77
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 70 deletions.
27 changes: 0 additions & 27 deletions apis/dataplane/v1beta1/openstackdataplanenodeset_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package v1beta1
import (
"context"
"fmt"
"strings"

"golang.org/x/exp/slices"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -349,29 +348,3 @@ func (r *OpenStackDataPlaneNodeSetSpec) TLSMatch(controlPlane openstackv1.OpenSt
}
return nil
}

// Validate NodeSet networks
func (r *OpenStackDataPlaneNodeSetSpec) ValidateNetworks() (errors field.ErrorList) {
for nodeName, node := range r.Nodes {
if len(node.Networks) > 0 && !strings.EqualFold(string(node.Networks[0].Name), CtlPlaneNetwork) {
errors = append(errors, field.Invalid(
field.NewPath("spec").Child("nodes").Child(nodeName).Child("networks"),
node.Networks,
fmt.Sprintf(
"node %s error: networks should start with %s got %s instead",
node.HostName, CtlPlaneNetwork, node.Networks[0].Name,
)))
}
}
if len(r.NodeTemplate.Networks) > 0 && !strings.EqualFold(string(r.NodeTemplate.Networks[0].Name), CtlPlaneNetwork) {
errors = append(errors, field.Invalid(
field.NewPath("spec").Child("nodeTemplate").Child("networks"),
r.NodeTemplate.Networks,
fmt.Sprintf(
"networks should start with %s got %s instead",
CtlPlaneNetwork, r.NodeTemplate.Networks[0].Name,
)))
}

return errors
}
3 changes: 0 additions & 3 deletions apis/dataplane/v1beta1/openstackdataplanenodeset_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,6 @@ func (r *OpenStackDataPlaneNodeSet) ValidateCreate() (admission.Warnings, error)
if err != nil {
return nil, err
}
errors = append(errors, r.Spec.ValidateNetworks()...)

// Check if OpenStackDataPlaneNodeSet name matches RFC1123 for use in labels
validate := validator.New()
if err := validate.Var(r.Name, "hostname_rfc1123"); err != nil {
Expand Down Expand Up @@ -185,7 +183,6 @@ func (r *OpenStackDataPlaneNodeSet) ValidateUpdate(old runtime.Object) (admissio
return nil, err
}

errors = append(errors, r.Spec.ValidateNetworks()...)
errors = append(errors, r.Spec.ValidateUpdate(&oldNodeSet.Spec)...)

if errors != nil {
Expand Down
11 changes: 11 additions & 0 deletions pkg/dataplane/ipam.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,13 +355,24 @@ func reserveIPs(ctx context.Context, helper *helper.Helper,

// CreateOrPatch IPSets
for nodeName, node := range instance.Spec.Nodes {
foundCtlPlane := false
nets := node.Networks
hostName := node.HostName
if len(nets) == 0 {
nets = instance.Spec.NodeTemplate.Networks
}

if len(nets) > 0 {
for _, net := range nets {
if strings.EqualFold(string(net.Name), dataplanev1.CtlPlaneNetwork) ||
netServiceNetMap[strings.ToLower(string(net.Name))] == dataplanev1.CtlPlaneNetwork {
foundCtlPlane = true
}
}
if !foundCtlPlane {
msg := fmt.Sprintf("ctlplane network should be defined for node %s", nodeName)
return nil, netServiceNetMap, fmt.Errorf(msg)
}
ipSet := &infranetworkv1.IPSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: instance.Namespace,
Expand Down
35 changes: 32 additions & 3 deletions tests/functional/dataplane/base_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ func DefaultDataPlaneNodeSetSpec(nodeSetName string) map[string]interface{} {
fmt.Sprintf("%s-node-1", nodeSetName): map[string]interface{}{
"hostName": "edpm-compute-node-1",
"networks": []infrav1.IPSetNetwork{
{Name: "networkinternal", SubnetName: "subnet1"},
{Name: "ctlplane", SubnetName: "subnet1"},
},
},
Expand Down Expand Up @@ -251,6 +252,7 @@ func DuplicateServiceNodeSetSpec(nodeSetName string) map[string]interface{} {
fmt.Sprintf("%s-node-1", nodeSetName): map[string]interface{}{
"hostName": "edpm-compute-node-1",
"networks": []infrav1.IPSetNetwork{
{Name: "networkinternal", SubnetName: "subnet1"},
{Name: "ctlplane", SubnetName: "subnet1"},
},
},
Expand All @@ -267,6 +269,7 @@ func DefaultDataPlaneNoNodeSetSpec(tlsEnabled bool) map[string]interface{} {
"preProvisioned": true,
"nodeTemplate": map[string]interface{}{
"networks": []infrav1.IPSetNetwork{
{Name: "networkinternal", SubnetName: "subnet1"},
{Name: "ctlplane", SubnetName: "subnet1"},
},
"ansibleSSHPrivateKeySecret": "dataplane-ansible-ssh-private-key-secret",
Expand Down Expand Up @@ -344,9 +347,10 @@ func SingleGlobalServiceDeploymentSpec() map[string]interface{} {
func DefaultNetConfigSpec() map[string]interface{} {
return map[string]interface{}{
"networks": []map[string]interface{}{{
"dnsDomain": "test-domain.test",
"mtu": 1500,
"name": "CtlPLane",
"dnsDomain": "test-domain.test",
"mtu": 1500,
"name": "CtlPlane",
"serviceNet": "ctlplane",
"subnets": []map[string]interface{}{{
"allocationRanges": []map[string]interface{}{{
"end": "172.20.12.120",
Expand All @@ -358,6 +362,22 @@ func DefaultNetConfigSpec() map[string]interface{} {
"gateway": "172.20.12.1",
},
},
}, {
"dnsDomain": "test-domain.test",
"mtu": 1500,
"name": "networkinternal",
"serviceNet": "internalapi",
"subnets": []map[string]interface{}{{
"allocationRanges": []map[string]interface{}{{
"end": "172.20.13.120",
"start": "172.20.13.0",
},
},
"name": "subnet1",
"cidr": "172.20.13.0/16",
"gateway": "172.20.13.1",
},
},
},
},
}
Expand Down Expand Up @@ -412,6 +432,15 @@ func SimulateIPSetComplete(name types.NamespacedName) {
Gateway: &gateway,
ServiceNetwork: "ctlplane",
},
{
Address: "172.20.13.76",
Cidr: "172.20.13.0/16",
MTU: 1500,
Network: "NetworkInternal",
Subnet: "subnet1",
Gateway: &gateway,
ServiceNetwork: "internalapi",
},
}
// This can return conflict so we have the gomega.Eventually block to retry
g.Expect(th.K8sClient.Status().Update(th.Ctx, IPSet)).To(Succeed())
Expand Down
105 changes: 85 additions & 20 deletions tests/functional/dataplane/openstackdataplanenodeset_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,46 @@ var _ = Describe("Dataplane NodeSet Test", func() {
})
})

When("A Dataplane nodeset is created and no ctlplane network in networks", func() {
BeforeEach(func() {
DeferCleanup(th.DeleteInstance,
CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))

DeferCleanup(th.DeleteInstance,
CreateDataplaneNodeSet(dataplaneNodeSetName,
DefaultDataPlaneNoNodeSetSpec(false)))
})

It("Should fail to set NodeSetIPReservationReadyCondition true when ctlplane is not in the networks", func() {
Eventually(func(g Gomega) {
instance := GetDataplaneNodeSet(dataplaneNodeSetName)
instance.Spec.NodeTemplate.Networks[1].Name = "notctlplane"
g.Expect(th.K8sClient.Update(th.Ctx, instance)).Should(Succeed())
th.ExpectCondition(
dataplaneNodeSetName,
ConditionGetterFunc(DataplaneConditionGetter),
condition.ReadyCondition,
corev1.ConditionFalse,
)
th.ExpectCondition(
dataplaneNodeSetName,
ConditionGetterFunc(DataplaneConditionGetter),
condition.InputReadyCondition,
corev1.ConditionUnknown,
)
th.ExpectCondition(
dataplaneNodeSetName,
ConditionGetterFunc(DataplaneConditionGetter),
dataplanev1.NodeSetIPReservationReadyCondition,
corev1.ConditionFalse,
)
conditions := DataplaneConditionGetter(dataplaneNodeSetName)
message := &conditions.Get(dataplanev1.NodeSetIPReservationReadyCondition).Message
g.Expect(*message).Should(ContainSubstring("ctlplane network should be defined for node"))
}, timeout, interval).Should(Succeed())
})
})

When("A Dataplane nodeset is created and no dnsmasq", func() {
BeforeEach(func() {
DeferCleanup(th.DeleteInstance,
Expand Down Expand Up @@ -292,10 +332,15 @@ var _ = Describe("Dataplane NodeSet Test", func() {
AnsibleVars: nil,
},
ExtraMounts: nil,
Networks: []infrav1.IPSetNetwork{{
Name: "ctlplane",
SubnetName: "subnet1",
},
Networks: []infrav1.IPSetNetwork{
{
Name: "networkinternal",
SubnetName: "subnet1",
},
{
Name: "ctlplane",
SubnetName: "subnet1",
},
},
},
Env: nil,
Expand Down Expand Up @@ -418,10 +463,15 @@ var _ = Describe("Dataplane NodeSet Test", func() {
},
NodeTemplate: dataplanev1.NodeTemplate{
AnsibleSSHPrivateKeySecret: "dataplane-ansible-ssh-private-key-secret",
Networks: []infrav1.IPSetNetwork{{
Name: "ctlplane",
SubnetName: "subnet1",
},
Networks: []infrav1.IPSetNetwork{
{
Name: "networkinternal",
SubnetName: "subnet1",
},
{
Name: "ctlplane",
SubnetName: "subnet1",
},
},
ManagementNetwork: "ctlplane",
Ansible: dataplanev1.AnsibleOpts{
Expand Down Expand Up @@ -724,10 +774,15 @@ var _ = Describe("Dataplane NodeSet Test", func() {
DeferCleanup(th.DeleteInstance, CreateNetConfig(dataplaneNetConfigName, DefaultNetConfigSpec()))
nodeOverrideSpec := dataplanev1.NodeSection{
HostName: dataplaneNodeName.Name,
Networks: []infrav1.IPSetNetwork{{
Name: "ctlplane",
SubnetName: "subnet1",
},
Networks: []infrav1.IPSetNetwork{
{
Name: "networkinternal",
SubnetName: "subnet1",
},
{
Name: "ctlplane",
SubnetName: "subnet1",
},
},
Ansible: dataplanev1.AnsibleOpts{
AnsibleUser: "test-user",
Expand Down Expand Up @@ -860,10 +915,15 @@ var _ = Describe("Dataplane NodeSet Test", func() {
},
NodeTemplate: dataplanev1.NodeTemplate{
AnsibleSSHPrivateKeySecret: "dataplane-ansible-ssh-private-key-secret",
Networks: []infrav1.IPSetNetwork{{
Name: "ctlplane",
SubnetName: "subnet1",
},
Networks: []infrav1.IPSetNetwork{
{
Name: "networkinternal",
SubnetName: "subnet1",
},
{
Name: "ctlplane",
SubnetName: "subnet1",
},
},
ManagementNetwork: "ctlplane",
Ansible: dataplanev1.AnsibleOpts{
Expand Down Expand Up @@ -1164,10 +1224,15 @@ var _ = Describe("Dataplane NodeSet Test", func() {
DeferCleanup(th.DeleteInstance, CreateDNSMasq(dnsMasqName, DefaultDNSMasqSpec()))
nodeOverrideSpec := dataplanev1.NodeSection{
HostName: dataplaneNodeName.Name,
Networks: []infrav1.IPSetNetwork{{
Name: "ctlplane",
SubnetName: "subnet1",
},
Networks: []infrav1.IPSetNetwork{
{
Name: "networkinternal",
SubnetName: "subnet1",
},
{
Name: "ctlplane",
SubnetName: "subnet1",
},
},
Ansible: dataplanev1.AnsibleOpts{
AnsibleUser: "test-user",
Expand Down
17 changes: 0 additions & 17 deletions tests/functional/dataplane/openstackdataplanenodeset_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,21 +243,4 @@ var _ = Describe("DataplaneNodeSet Webhook", func() {
dataplaneDeploymentName.Name, string(v1beta1.NodeSetDeploymentReadyCondition))))
})
})
When("networks are out of order", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNoNodeSetSpec(false)
dataplaneNodeSetName.Name = "unordered"
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
})

It("Should fail when ctlplane is not the first network", func() {
Eventually(func(_ Gomega) string {
dataplaneNodeSetName.Name = "unordered"
instance := GetDataplaneNodeSet(dataplaneNodeSetName)
instance.Spec.NodeTemplate.Networks[0].Name = "wrong"
err := th.K8sClient.Update(th.Ctx, instance)
return fmt.Sprintf("%s", err)
}).Should(ContainSubstring("networks should start with ctlplane got wrong instead"))
})
})
})

0 comments on commit 478de77

Please sign in to comment.