core: validate crd name size from admission controller #11233

parth-gr · 2022-10-27T15:23:50Z

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves ##11212

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn · 2022-10-27T17:13:54Z

pkg/operator/ceph/cr_manager.go

+	for _, crd := range o.resources {
+		var crdNameLength int = len(crd.Name)
+		if crdNameLength > 38 {
+			return errors.New("CRD name cannot be bigger than 38 characters")


As discussed, having a single limit for all CRs may break some existing CRs if it's not actually a limit already hit with some K8s error. We may want to just address the original bug for the object store and only add a check for that CR length to start with.

travisn · 2022-10-27T17:17:16Z

pkg/operator/ceph/cr_manager.go

 func (o *Operator) startCRDManager(context context.Context, mgrErrorCh chan error) {
+	logger.Info("check if CRDs name are valid")


We need to check the CR name lengths in the admission controller. For example, in the object store CR this would be in the ValidateObjectSpec() method.

Checking at operator startup time won't prevent the CRs from being created incorrectly.

right and actually we need to update this method

rook/pkg/apis/ceph.rook.io/v1/object.go

Lines 63 to 68 in ef3f554

func (o *CephObjectStore) ValidateCreate() error {

logger.Infof("validate create cephobjectstore %v", o)

if err := ValidateObjectSpec(o); err != nil {

return err

}

return nil

since ValidateObjectSpec() is called during validate updatealso which will be problem for upgrade.

@subhamkrai why it would be a problem during upgrade

@travisn >Checking at operator startup time won't prevent the CRs from being created incorrectly.
That right,
I thought this process will reconcile later, but if it is been created at very first then there is no need to add a check,
So updated the validate method
Thanks.

@subhamkrai why it would be a problem during upgrade

see here #11233 (comment)

travisn · 2022-10-28T14:27:28Z

pkg/apis/ceph.rook.io/v1/object.go

@@ -76,6 +76,9 @@ func ValidateObjectSpec(gs *CephObjectStore) error {
 	if gs.Namespace == "" {
 		return errors.New("missing namespace")
 	}
+	if len(gs.Name) > 38 {
+		return errors.New("object store name cannot be bigger than 38 characters")


Suggested change

return errors.New("object store name cannot be bigger than 38 characters")

return errors.New("object store name cannot be longer than 38 characters")

travisn · 2022-10-28T14:29:17Z

pkg/apis/ceph.rook.io/v1/object.go

@@ -76,6 +76,9 @@ func ValidateObjectSpec(gs *CephObjectStore) error {
 	if gs.Namespace == "" {
 		return errors.New("missing namespace")
 	}
+	if len(gs.Name) > 38 {


Please add a comment here that explains why 38 is the critical number, and maybe show a formula for how it was decided.

subhamkrai · 2022-10-28T14:52:50Z

pkg/apis/ceph.rook.io/v1/object.go

@@ -76,6 +76,9 @@ func ValidateObjectSpec(gs *CephObjectStore) error {
 	if gs.Namespace == "" {
 		return errors.New("missing namespace")
 	}
+	if len(gs.Name) > 38 {


Let's make 38 a const variable on the top

Added the reason in the comment

I'd vote for the const as well, still helpful for code readability.

subhamkrai · 2022-10-28T14:55:25Z

pkg/apis/ceph.rook.io/v1/object.go

+	if len(gs.Name) > 38 {
+		return errors.New("object store name cannot be bigger than 38 characters")
+	}


Suggested change

if len(gs.Name) > 38 {

return errors.New("object store name cannot be bigger than 38 characters")

}

if len(gs.Name) > 38 {

return errors.New("object store name cannot be bigger than 38 characters")

}

Also, as I mentioned let's move this block to L63 inside ValidateCluster as this method will trigger during the update also and will cause errors because earlier it was >38 and now webhook will validate with <38 and it will through an error.

@travisn right?

This method is called by ValidateCreate() on L65, so why do we need to move it? I didn't follow.

It will not create a problem for upgrade cluster, because a deployment is not possible if the store name exceeded this length

let's discuss this

travisn

Actually we should be able to this with a schema length check on the types.go property such as:

+kubebuilder:validation:MaxLength=38

Then we don't need code in the admission controller.

parth-gr · 2022-11-02T14:36:58Z

Actually we should be able to this with a schema length check on the types.go property such as:
+kubebuilder:validation:MaxLength=38
Then we don't need code in the admission controller.

That was very first thing I wanted to do but, objectstore is define as

type CephObjectStore struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata"`

metav1.ObjectMeta json:"metadata"`` contains the name definition of the objectstore and Kubernetes define, which should not be edited, right?

travisn

That was very first thing I wanted to do but, objectstore is define as
type CephObjectStore struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata"`
metav1.ObjectMeta json:"metadata"`` contains the name definition of the objectstore and Kubernetes define, which should not be edited, right?

Oh of course, that won't work!

travisn · 2022-11-02T14:47:49Z

pkg/apis/ceph.rook.io/v1/object.go

@@ -76,6 +76,9 @@ func ValidateObjectSpec(gs *CephObjectStore) error {
 	if gs.Namespace == "" {
 		return errors.New("missing namespace")
 	}
+	if len(gs.Name) > 38 {


I'd vote for the const as well, still helpful for code readability.

subhamkrai

build is failing, can you restart and confirm? @parth-gr

Closes: rook#11212 Signed-off-by: parth-gr <paarora@redhat.com>

parth-gr · 2022-11-03T14:44:59Z

build is failing, can you restart and confirm? @parth-gr

The build passed thanks @sp98 for pointing out the problem in the comment using an extra ` symbol

core: validate crd name size from admission controller (backport #11233)

parth-gr changed the title ~~Crd length validation~~ core: validate crd name size from admission controller Oct 27, 2022

parth-gr force-pushed the crd-length-validation branch from 7e40dcf to da4b95e Compare October 27, 2022 16:08

travisn requested changes Oct 27, 2022

View reviewed changes

parth-gr force-pushed the crd-length-validation branch from da4b95e to 563be1a Compare October 28, 2022 12:45

parth-gr requested a review from travisn October 28, 2022 12:45

travisn requested changes Oct 28, 2022

View reviewed changes

subhamkrai requested changes Oct 28, 2022

View reviewed changes

parth-gr force-pushed the crd-length-validation branch from 563be1a to 43788df Compare October 31, 2022 14:51

parth-gr requested review from subhamkrai and travisn October 31, 2022 14:52

parth-gr force-pushed the crd-length-validation branch from 43788df to 9582465 Compare October 31, 2022 15:41

travisn requested changes Oct 31, 2022

View reviewed changes

parth-gr requested a review from travisn November 2, 2022 14:37

travisn reviewed Nov 2, 2022

View reviewed changes

parth-gr force-pushed the crd-length-validation branch from 9582465 to 877c506 Compare November 2, 2022 14:56

travisn approved these changes Nov 2, 2022

View reviewed changes

subhamkrai reviewed Nov 2, 2022

View reviewed changes

parth-gr force-pushed the crd-length-validation branch 2 times, most recently from 8cc0233 to 468747c Compare November 3, 2022 14:27

core: validate crd name size from admission controller

7bd863d

Closes: rook#11212 Signed-off-by: parth-gr <paarora@redhat.com>

parth-gr force-pushed the crd-length-validation branch from 468747c to 7bd863d Compare November 3, 2022 14:29

parth-gr requested a review from subhamkrai November 3, 2022 14:43

subhamkrai approved these changes Nov 3, 2022

View reviewed changes

parth-gr added the backport-release-1.10 label Nov 3, 2022

travisn merged commit 9c7a004 into rook:master Nov 3, 2022

mergify bot mentioned this pull request Nov 3, 2022

core: validate crd name size from admission controller (backport #11233) #11264

Merged

travisn added a commit that referenced this pull request Nov 3, 2022

Merge pull request #11264 from rook/mergify/bp/release-1.10/pr-11233

c9506ed

core: validate crd name size from admission controller (backport #11233)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: validate crd name size from admission controller #11233

core: validate crd name size from admission controller #11233

parth-gr commented Oct 27, 2022 •

edited

travisn Oct 27, 2022

travisn Oct 27, 2022

subhamkrai Oct 28, 2022

parth-gr Oct 28, 2022

parth-gr Oct 28, 2022 •

edited

subhamkrai Oct 28, 2022

travisn Oct 28, 2022

travisn Oct 28, 2022

parth-gr Oct 31, 2022

subhamkrai Oct 28, 2022

parth-gr Oct 31, 2022

travisn Nov 2, 2022

subhamkrai Oct 28, 2022

travisn Oct 28, 2022

parth-gr Oct 31, 2022 •

edited

subhamkrai Nov 2, 2022

travisn left a comment

parth-gr commented Nov 2, 2022

travisn left a comment

travisn Nov 2, 2022

subhamkrai left a comment

parth-gr commented Nov 3, 2022

		func (o *Operator) startCRDManager(context context.Context, mgrErrorCh chan error) {
		logger.Info("check if CRDs name are valid")

	func (o *CephObjectStore) ValidateCreate() error {
	logger.Infof("validate create cephobjectstore %v", o)
	if err := ValidateObjectSpec(o); err != nil {
	return err
	}
	return nil

	return errors.New("object store name cannot be bigger than 38 characters")
	return errors.New("object store name cannot be longer than 38 characters")

core: validate crd name size from admission controller #11233

core: validate crd name size from admission controller #11233

Conversation

parth-gr commented Oct 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Oct 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

parth-gr commented Nov 2, 2022

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkrai left a comment

Choose a reason for hiding this comment

parth-gr commented Nov 3, 2022

parth-gr commented Oct 27, 2022 •

edited

parth-gr Oct 28, 2022 •

edited

parth-gr Oct 31, 2022 •

edited