Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Commit

Permalink
Block changes to baremetalSetTemplate
Browse files Browse the repository at this point in the history
This will block changes to the baremetalSetTemplate to ensure that users
don't try to change attributes about the baremetal nodes that would
require re-provisioning the baremetal host via metal3.

Signed-off-by: Brendan Shephard <[email protected]>
  • Loading branch information
bshephar committed Oct 26, 2023
1 parent bc9eca4 commit 5832504
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 10 deletions.
55 changes: 54 additions & 1 deletion api/v1beta1/openstackdataplanenodeset_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,15 @@ limitations under the License.
package v1beta1

import (
"fmt"
"reflect"

baremetalv1 "github.com/openstack-k8s-operators/openstack-baremetal-operator/api/v1beta1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
ctrl "sigs.k8s.io/controller-runtime"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -87,8 +94,54 @@ func (r *OpenStackDataPlaneNodeSet) ValidateCreate() error {
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
func (r *OpenStackDataPlaneNodeSet) ValidateUpdate(old runtime.Object) error {
openstackdataplanenodesetlog.Info("validate update", "name", r.Name)
oldNodeSet, ok := old.(*OpenStackDataPlaneNodeSet)
if !ok {
return apierrors.NewInternalError(
fmt.Errorf("Expected a OpenStackDataPlaneNodeSet object, but got %T", oldNodeSet))
}

var errors field.ErrorList
// Some changes to the baremetalSetTemplate after the initial deployment would necessitate
// a redeploy of the node. Thus we should block these changes and require the user to
// delete and redeploy should they wish to make such changes after the initial deploy.
// If the BaremetalSetTemplate is changed, we will offload the parsing of these details
// to the openstack-baremetal-operator webhook to avoid duplicating logic.
if !reflect.DeepEqual(r.Spec.BaremetalSetTemplate, oldNodeSet.Spec.BaremetalSetTemplate) {
// Initialize OpenStackBaremetalSet with old spec details
oldBaremetalSetObject := &baremetalv1.OpenStackBaremetalSet{
ObjectMeta: metav1.ObjectMeta{
Name: r.Name,
Namespace: r.Namespace,
},
}
oldNodeSet.Spec.BaremetalSetTemplate.DeepCopyInto(&oldBaremetalSetObject.Spec)

// Initialize OpenStackBaremetalSet with new spec details
baremetalSetObject := &baremetalv1.OpenStackBaremetalSet{
ObjectMeta: metav1.ObjectMeta{
Name: r.Name,
Namespace: r.Namespace,
},
}
r.Spec.BaremetalSetTemplate.DeepCopyInto(&baremetalSetObject.Spec)

// Call openstack-baremetal-operator ValidateUpdate() webhook to parse changes
err := baremetalSetObject.ValidateUpdate(oldBaremetalSetObject)
if err != nil {
errors = append(errors, field.Forbidden(
field.NewPath("spec.baremetalSetTemplate"),
fmt.Sprintf("%s", err)))
}
}

if errors != nil {
return apierrors.NewInvalid(
schema.GroupKind{Group: "dataplane.openstack.org", Kind: "OpenStackDataPlaneNodeSet"},
r.Name,
errors,
)
}

// TODO(user): fill in your validation logic upon object update.
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion api/v1beta1/openstackdataplaneservice_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ package v1beta1
import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
infranetworkv1 "github.com/openstack-k8s-operators/infra-operator/apis/network/v1beta1"
"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
)

// KubeService represents a Kubernetes Service. It is called like this to avoid the extreme overloading of the
Expand Down
8 changes: 0 additions & 8 deletions controllers/openstackdataplanenodeset_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,6 @@ func (r *OpenStackDataPlaneNodeSetReconciler) Reconcile(ctx context.Context, req
}
}

// TODO: if the input hash changes or the nodes in the role is updated we should
// detect that and redeploy the role that may also require deleting/recreating
// the dataplane service AEE CRs based on the updated input/inventory.
// for now we just check if the role is already deployed and not being deleted
// and leave the triggering to a human to initiate.
// This can be done by deleting the dataplane service AEE CRs and then
// patching the role to set dataplane service condition ready to "Unknown"
// then patching the Deployed flag to false.
if instance.Status.Deployed && instance.DeletionTimestamp.IsZero() {
// The role is already deployed and not being deleted, so reconciliation
// is already complete.
Expand Down
96 changes: 96 additions & 0 deletions tests/functional/openstackdataplanenodeset_webhook_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package functional

import (
"fmt"
"os"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/types"

baremetalv1 "github.com/openstack-k8s-operators/openstack-baremetal-operator/api/v1beta1"
)

var _ = Describe("DataplaneNodeSet Webhook", func() {

var dataplaneNodeSetName types.NamespacedName

BeforeEach(func() {
dataplaneNodeSetName = types.NamespacedName{
Name: "edpm-compute-nodeset",
Namespace: namespace,
}
err := os.Setenv("OPERATOR_SERVICES", "../../config/services")
Expect(err).NotTo(HaveOccurred())
})

When("User tries to change forbidden items in the baremetalSetTemplate", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNoNodeSetSpec()
nodeSetSpec["preProvisioned"] = false
nodeSetSpec["baremetalSetTemplate"] = baremetalv1.OpenStackBaremetalSetSpec{
CloudUserName: "test-user",
BmhLabelSelector: map[string]string{
"app": "test-openstack",
},
BaremetalHosts: map[string]baremetalv1.InstanceSpec{
"compute-0": {
CtlPlaneIP: "192.168.1.12",
},
},
}
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
})

It("Should block changes to the BmhLabelSelector object in baremetalSetTemplate spec", func() {
Eventually(func(g Gomega) string {
instance := GetDataplaneNodeSet(dataplaneNodeSetName)
instance.Spec.BaremetalSetTemplate = baremetalv1.OpenStackBaremetalSetSpec{
CloudUserName: "new-user",
BmhLabelSelector: map[string]string{
"app": "openstack1",
},
}
err := th.K8sClient.Update(th.Ctx, instance)
return fmt.Sprintf("%s", err)
}).Should(ContainSubstring("Forbidden: cannot change"))
})
})

When("A user changes an allowed field in the baremetalSetTemplate", func() {
BeforeEach(func() {
nodeSetSpec := DefaultDataPlaneNoNodeSetSpec()
nodeSetSpec["preProvisioned"] = false
nodeSetSpec["baremetalSetTemplate"] = baremetalv1.OpenStackBaremetalSetSpec{
CloudUserName: "test-user",
BmhLabelSelector: map[string]string{
"app": "test-openstack",
},
BaremetalHosts: map[string]baremetalv1.InstanceSpec{
"compute-0": {
CtlPlaneIP: "192.168.1.12",
},
},
}
DeferCleanup(th.DeleteInstance, CreateDataplaneNodeSet(dataplaneNodeSetName, nodeSetSpec))
})

It("Should allow changes to the CloudUserName", func() {
Eventually(func(g Gomega) error {
instance := GetDataplaneNodeSet(dataplaneNodeSetName)
instance.Spec.BaremetalSetTemplate = baremetalv1.OpenStackBaremetalSetSpec{
CloudUserName: "new-user",
BmhLabelSelector: map[string]string{
"app": "test-openstack",
},
BaremetalHosts: map[string]baremetalv1.InstanceSpec{
"compute-0": {
CtlPlaneIP: "192.168.1.12",
},
},
}
return th.K8sClient.Update(th.Ctx, instance)
}).Should(Succeed())
})
})
})

0 comments on commit 5832504

Please sign in to comment.