Add validations for customresourcedefinitions and apiservicedefinitions #112

darkowlzz · 2019-04-23T15:49:12Z

Adds _csv_crd_validation() and _csv_asd_validation for validating csv.spec.customresourcedefinitions and csv.spec.apiservicedefinitions respectively.
Also validates the descriptors in both CRD and ASD using a common method _csv_descriptors_validation().

openshift-ci-robot · 2019-04-23T15:49:34Z

Hi @darkowlzz. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

SamiSousa

Adding these validations will be a great help, but I think we need to agree to what extent we are validating the CSV

SamiSousa · 2019-04-23T20:11:38Z

operatorcourier/validate.py

+                name = crd["metadata"]["name"]
+                crdList.append(name)
+            except KeyError:
+                pass


Could this log a warning or error informing the user that a CRD is missing the metadata.name field? It's better to be noisy than quiet in this case (unless we previously warn elsewhere)

It's logged here https://github.com/operator-framework/operator-courier/blob/v2.0.2/operatorcourier/validate.py#L107

SamiSousa · 2019-04-23T20:13:11Z

operatorcourier/validate.py

+                pass
+
+        if "owned" not in customresourcedefinitions:
+            self._log_error("spec.customresourcedefinitions.owned"


Not all operators that have the customresourcedefinitions field operate on "owned" CRDs. I believe this should be an early exit rather than a logged error

I agree that having customresourcedefinitions without any subfields should raise an error. But there are other values than "owned" that could exist here (such as required: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/Documentation/design/building-your-csv.md#required-crds)

I agree that having customresourcedefinitions without any subfields should raise an error.

To contradict myself, what's the benefit of enforcing that subfields exist?

Right, those are not required fields. We shouldn't invalidate here.
Enforcement is beneficial only if we know that no subfields result in insall failure. I don't think that's the case here.

SamiSousa · 2019-04-23T20:24:41Z

operatorcourier/validate.py

+    def _csv_asd_validation(self, apiservicedefinitions):
+        valid = True
+
+        if "owned" not in apiservicedefinitions:


An operator may also have "required" apiservicedefinitions: https://github.com/operator-framework/operator-lifecycle-manager/blob/master/Documentation/design/building-your-csv.md#required-apiservices

thanks, and owned and required aren't required fields, so we shouldn't invalidate here.
I'll leave required validation for a separate PR, will validate owned only for now.

SamiSousa · 2019-04-23T20:27:40Z

operatorcourier/validate.py

@@ -312,6 +413,31 @@ def _csv_spec_install_validation(self, install):

        return valid

+    def _csv_descriptors_validation(self, descriptors, crdName):


Could we also check that the x-descriptors (if it exists) field is a list of strings?
Eq: https://github.com/operator-framework/community-operators/blob/master/community-operators/cluster-logging/cluster-logging.v4.1.0.clusterserviceversion.yaml#L275
Not sure if this is a requirement, more like an observation.

Yes, thanks for the idea, that would be useful and I would like to add that in a separate PR. This PR is already huge.

SamiSousa · 2019-04-23T20:30:43Z

...ource_dir/valid_yamls_with_single_crd/dynatrace-monitoring.v0.2.0.clusterserviceversion.yaml

@@ -38,7 +38,8 @@ metadata:
  name: dynatrace-monitoring.v0.2.0
  namespace: "placeholder"
 spec:
-  apiservicedefinitions: {}
+  apiservicedefinitions:
+    owned: {}


Are we requiring that if the apiservicedefinitions or customresourcedefinitions fields exist that they have owned as a sub field (or the presence of any valid sub field)?
(Also this should be a list not an object)

Is our stance that if it exists and is empty that is valid? If so, these both should be valid:

spec: apiservicedefinitions: {}

and

spec: apiservicedefinitions: owned: []

I want to clarify that my stance is based on my observations of the existing community-operators CSVs, many of which set spec.apiservicedefinitions to the empty object {}.
We could enforce that the key owned exists, but should we/does it create any benefit?

Thanks. This was a mistake based on requiring owned. owned is not required, and this change is not needed.

spec.customresourcedefinitions.owned CRDs must have displayName and description and values must not be empty. Validate the attributes of csv owned crd descriptors.

flickerfly · 2019-10-29T16:35:27Z

/test

Because I'm interested in this work and it has been awhile.

openshift-ci-robot · 2021-04-09T12:47:15Z

@darkowlzz: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 23, 2019

openshift-ci-robot requested review from awgreene and MartinBasti April 23, 2019 15:49

SamiSousa suggested changes Apr 23, 2019

View reviewed changes

darkowlzz added 3 commits May 5, 2019 21:52

validate: validate csv spec.crd.owned & descriptors

ce5ce69

spec.customresourcedefinitions.owned CRDs must have displayName and description and values must not be empty. Validate the attributes of csv owned crd descriptors.

validate: add validation for apiservicedefinitions

07c881a

Remove owned requirement

983e781

darkowlzz force-pushed the csv.spec.crd-validation branch from 7952ae6 to 983e781 Compare May 5, 2019 16:22

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validations for customresourcedefinitions and apiservicedefinitions #112

Add validations for customresourcedefinitions and apiservicedefinitions #112

darkowlzz commented Apr 23, 2019

openshift-ci-robot commented Apr 23, 2019

SamiSousa left a comment

SamiSousa Apr 23, 2019

darkowlzz May 5, 2019

SamiSousa Apr 23, 2019

SamiSousa Apr 23, 2019

SamiSousa May 3, 2019

darkowlzz May 5, 2019

SamiSousa Apr 23, 2019

darkowlzz May 5, 2019

SamiSousa Apr 23, 2019

darkowlzz May 5, 2019

SamiSousa Apr 23, 2019

SamiSousa Apr 23, 2019

SamiSousa May 3, 2019

darkowlzz May 5, 2019

flickerfly commented Oct 29, 2019

openshift-ci-robot commented Apr 9, 2021

		@@ -312,6 +413,31 @@ def _csv_spec_install_validation(self, install):

		return valid

		def _csv_descriptors_validation(self, descriptors, crdName):

Add validations for customresourcedefinitions and apiservicedefinitions #112

Are you sure you want to change the base?

Add validations for customresourcedefinitions and apiservicedefinitions #112

Conversation

darkowlzz commented Apr 23, 2019

openshift-ci-robot commented Apr 23, 2019

SamiSousa left a 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

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

flickerfly commented Oct 29, 2019

openshift-ci-robot commented Apr 9, 2021