Repository: metal3-io/baremetal-operator
Pull request #991 (2021-10-07 11:59:05)

Add validations for bmo resource name
Add validations to check if there are any invalid character(s) in the BMO resource name.

hroyrh: fixes #855 
ardaguclu: /test-integration
ardaguclu: /test-v1b1-integration
dtantsur: /lgtm
andfasano: Given that the validation rules are fine, my biggest concern here is that they are not applied in the operator (ie, in the create event filter), thus leading to accept invalid BMH when webhook is not enabled / not available. Thoughts?
ardaguclu: > Given that the validation rules are fine, my biggest concern here is that they are not applied in the operator (ie, in the create event filter), thus leading to accept invalid BMH when webhook is not enabled / not available. Thoughts?

@andfasano baremetal operator webhook failure policy(both upstream and downstream) is set to fail. That means if webhook has problems, operator can not accept any requests. Or you mean somewhere else that did not come to my mind? 
andfasano: > > Given that the validation rules are fine, my biggest concern here is that they are not applied in the operator (ie, in the create event filter), thus leading to accept invalid BMH when webhook is not enabled / not available. Thoughts?
> 
> @andfasano baremetal operator webhook failure policy(both upstream and downstream) is set to fail. That means if webhook has problems, operator can not accept any requests. Or you mean somewhere else that did not come to my mind?

I'm referring to the _"Validating a resource in the controller"_ chapter of the ["Kubernetes operator best practices" blog article](https://cloud.redhat.com/blog/kubernetes-operators-best-practices) where essentially the recommendation it's to organize the code to share the same validation rule between the controller and the webhook, so that in those cases where the webhook is not available (maybe because not enabled or not yet started due some racy conditions) the check will not be missed. In addition, I'd say that sharing the code in such case will ensure that the validation logic will not diverge between the controller and the webhook.
metal3-io-bot: @hroyrh: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gofmt/1463567574506672128) | true | `/test gofmt`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=991). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @hroyrh: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gofmt/1463567574506672128) | true | `/test gofmt`
gomod | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gomod/1463567577040031744) | true | `/test gomod`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=991). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @hroyrh: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gofmt/1463567574506672128) | true | `/test gofmt`
gomod | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gomod/1463567577040031744) | true | `/test gomod`
manifestlint | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/manifestlint/1463567585395085312) | true | `/test manifestlint`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=991). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @hroyrh: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gofmt/1463567574506672128) | true | `/test gofmt`
gomod | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gomod/1463567577040031744) | true | `/test gomod`
manifestlint | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/manifestlint/1463567585395085312) | true | `/test manifestlint`
unit | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/unit/1463567580840071168) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=991). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @hroyrh: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
gofmt | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gofmt/1463567574506672128) | true | `/test gofmt`
gomod | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gomod/1463567577040031744) | true | `/test gomod`
manifestlint | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/manifestlint/1463567585395085312) | true | `/test manifestlint`
unit | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/unit/1463567580840071168) | true | `/test unit`
gosec | a4fdef4b34fa8a6f2c6d00e1e87d82b3d96facf9 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/991/gosec/1463567576381526016) | true | `/test gosec`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=991). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:hroyrh).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
ardaguclu: Maybe we can discuss this again after bmc module created.
/lgtm
/test all
ardaguclu: /test-v1b1-centos-integration
andfasano: > Maybe we can discuss this again after bmc module created. /lgtm /test all

Despite the technical difficulties, my point was to decide a common and shared approach for this specific point at design level, given that also in other BMO controllers we are currently going to add webhooks. 
metal3-io-bot: @hroyrh: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/apis/go.mod b/apis/go.mod
index 9f5e4667..b201ad7c 100644
--- a/apis/go.mod
+++ b/apis/go.mod
@@ -4,6 +4,7 @@ go 1.17
 
 require (
 	github.com/metal3-io/baremetal-operator/pkg/hardwareutils v0.0.0
+	github.com/google/uuid v1.1.2
 	github.com/stretchr/testify v1.7.0
 	k8s.io/api v0.21.4
 	k8s.io/apimachinery v0.21.4
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index d8eda142..f9843e39 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -2,7 +2,9 @@ package v1alpha1
 
 import (
 	"fmt"
+	"regexp"
 
+	"github.com/google/uuid"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	_ "github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
@@ -20,6 +22,10 @@ func (host *BareMetalHost) validateHost() []error {
 		errs = append(errs, err)
 	}
 
+	if err := validateBMHName(host.Name); err != nil {
+		errs = append(errs, err)
+	}
+
 	return errs
 }
 
@@ -55,3 +61,18 @@ func validateRAID(r *RAIDConfig) error {
 
 	return nil
 }
+
+func validateBMHName(bmhname string) error {
+
+	invalidname, _ := regexp.MatchString(`[^A-Za-z0-9\.\-\_]`, bmhname)
+	if invalidname {
+		return fmt.Errorf("BareMetalHost resource name cannot contain characters other than [A-Za-z0-9._-]")
+	}
+
+	_, err := uuid.Parse(bmhname)
+	if err == nil {
+		return fmt.Errorf("BareMetalHost resource name cannot be a UUID")
+	}
+
+	return nil
+}


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 707575bc..e2d2aef5 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -32,6 +32,16 @@ func TestValidateCreate(t *testing.T) {
 		Namespace: "test-namespace",
 	}
 
+	inom := metav1.ObjectMeta{
+		Name:      "test~",
+		Namespace: "test-namespace",
+	}
+
+	inom2 := metav1.ObjectMeta{
+		Name:      "07564256-96ae-4315-ab03-8d34ece60fbb",
+		Namespace: "test-namespace",
+	}
+
 	tests := []struct {
 		name      string
 		newBMH    *BareMetalHost
@@ -44,6 +54,18 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "",
 		},
+		{
+			name:      "invalidName",
+			newBMH:    &BareMetalHost{TypeMeta: tm, ObjectMeta: inom, Spec: BareMetalHostSpec{}},
+			oldBMH:    nil,
+			wantedErr: "BareMetalHost resource name cannot contain characters other than [A-Za-z0-9._-]",
+		},
+		{
+			name:      "invalidName2",
+			newBMH:    &BareMetalHost{TypeMeta: tm, ObjectMeta: inom2, Spec: BareMetalHostSpec{}},
+			oldBMH:    nil,
+			wantedErr: "BareMetalHost resource name cannot be a UUID",
+		},
 		{
 			name: "invalidRAID",
 			newBMH: &BareMetalHost{