Validate parameters on odo service create from swagger/csv

[feloy]

What type of PR is this?

/kind feature

What does this PR do / why we need it:

• odo catalog describe service human-readable output is similar to kubectl explain output
• during odo service create, github.com/go-openapi/validate is used to validate input parameters against the schema (directly from CRD or built from CSV x-descriptors)

Which issue(s) this PR fixes:

Fixes #4850

PR acceptance criteria:

• Unit test

• Integration test

• Documentation

• I have read the test guidelines

How to test changes / Special notes to the reviewer:

[feloy]

Tests pass locally

/test v4.8-integration-e2e

[odo]  ✗  Failed to start component with name "nodejs-x550532063-xtoh". Error: Failed to create the component: failed to create service(s) associated with the component: Database.postgresql.dev4devs.com "guxdmewudw" is invalid: spec.size: Invalid value: "string": spec.size in body must be of type integer: "string"

[feloy]

/test psi-unit-test-windows

* could not run steps: step unit-test-windows failed: "unit-test-windows" test steps failed: "unit-test-windows" pod "unit-test-windows-unit-test-windows-steps" failed: the pod ci-op-i9rtyxfw/unit-test-windows-unit-test-windows-steps failed after 59s (failed containers: test): ContainerFailed one or more containers exited

[feloy]

/test v4.8-integration-e2e

kube:admin
++++ scripts/configure-cluster/common/kubeconfigreset.sh
./scripts/configure-cluster/common/kubeconfigandadmin.sh: line 41: scripts/configure-cluster/common/kubeconfigreset.sh: Permission denied
+++ echo 'Call  separately if you want to reset the kubeconfig'
Call  separately if you want to reset the kubeconfig
++ . ./scripts/configure-cluster/common/setup-postgres-operator.sh
++ sh ./scripts/configure-cluster/common/setup-operators.sh
+ export SBO_CATALOG_SOURCE=redhat-operators
+ SBO_CATALOG_SOURCE=redhat-operators
+ export SBO_SUBSCRIPTION_NAME=rh-service-binding-operator
+ SBO_SUBSCRIPTION_NAME=rh-service-binding-operator
+ '[' == true ']'
./scripts/configure-cluster/common/setup-operators.sh: line 56: [: ==: unary operator expected
+ '[' == true ']'
./scripts/configure-cluster/common/setup-operators.sh: line 60: [: ==: unary operator expected
+ install_redis_operator oc openshift-operators community-operators openshift-marketplace

[netlify]

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 76b63e7

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/61486d321e2fe500070dbfc0

😎 Browse the preview: https://deploy-preview-5074--odo-docusaurus-preview.netlify.app

[feloy]

/test v4.8-integration-e2e

[odo]  ✗  Failed to pull stack nodejs from registry.stage.devfile.io/devfile-catalog/nodejs:latest with allowed media types [application/vnd.devfileio.devfile.layer.v1 image/png image/svg+xml application/vnd.devfileio.vsx.layer.v1.tar application/x-tar]: failed to do request: Head "https://registry.stage.devfile.io/v2/devfile-catalog/nodejs/manifests/latest": connection error: PROTOCOL_ERROR

[feloy]

/test psi-kubernetes-integration-e2e

Is the test using previous source?

Test started today at 8:56 AM failed after 3m31s. (more info)

[feloy]

/test ci/prow/v4.8-integration-e2e

level=error
level=error msg=Error: Provider produced inconsistent result after apply
level=error
level=error msg=When applying changes to module.vpc.aws_route_table.default[0], provider
level=error msg="registry.terraform.io/-/aws" produced an unexpected new value for was
level=error msg=present, but now absent.
level=error
level=error msg=This is a bug in the provider, which should be reported in the provider's own
level=error msg=issue tracker.
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change

[openshift-ci]

@feloy: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test psi-kubernetes-integration-e2e
• /test unit
• /test v4.6-images
• /test v4.7-images
• /test v4.8-images
• /test v4.8-integration-e2e
• /test v4.9-images

The following commands are available to trigger optional jobs:

• /test psi-k8s-ibmc-integration-e2e
• /test psi-unit-test-mac
• /test psi-unit-test-windows

Use /test all to run the following jobs that were automatically triggered:

• pull-ci-openshift-odo-main-psi-k8s-ibmc-integration-e2e
• pull-ci-openshift-odo-main-psi-kubernetes-integration-e2e
• pull-ci-openshift-odo-main-psi-unit-test-mac
• pull-ci-openshift-odo-main-psi-unit-test-windows
• pull-ci-openshift-odo-main-unit
• pull-ci-openshift-odo-main-v4.8-integration-e2e

In response to this:

/test ci/prow/v4.8-integration-e2e

level=error
level=error msg=Error: Provider produced inconsistent result after apply
level=error
level=error msg=When applying changes to module.vpc.aws_route_table.default[0], provider
level=error msg="registry.terraform.io/-/aws" produced an unexpected new value for was
level=error msg=present, but now absent.
level=error
level=error msg=This is a bug in the provider, which should be reported in the provider's own
level=error msg=issue tracker.
level=error
level=fatal msg=failed to fetch Cluster: failed to generate asset "Cluster": failed to create cluster: failed to apply Terraform: failed to complete the change

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.

[feloy]

/test v4.8-integration-e2e

[dharmit]

$ odo service create redis-operator.v0.8.0/Redis -p kubernetesConfig.imagePullPolicy=Always -p kubernetesConfig.image=quay.io/opstree/redis:v6.2.5 -p kubernetesConfig.serviceType=ClusterIP -p noSuchParam=junkValue --dry-run
apiVersion: redis.redis.opstreelabs.in/v1beta1
kind: Redis
metadata:
  name: redis
spec:
  kubernetesConfig:
    image: quay.io/opstree/redis:v6.2.5
    imagePullPolicy: Always
    serviceType: ClusterIP
  noSuchParam: junkValue

I purposefully added -p noSuchParam=junkValue to above command. I think the way we have things now, it doesn't allow setting a value for an invalid parameter. Although it's based on x-descriptors and not swagger. But I think we should still do that in validation.

[feloy]

$ odo service create redis-operator.v0.8.0/Redis -p kubernetesConfig.imagePullPolicy=Always -p kubernetesConfig.image=quay.io/opstree/redis:v6.2.5 -p kubernetesConfig.serviceType=ClusterIP -p noSuchParam=junkValue --dry-run
apiVersion: redis.redis.opstreelabs.in/v1beta1
kind: Redis
metadata:
  name: redis
spec:
  kubernetesConfig:
    image: quay.io/opstree/redis:v6.2.5
    imagePullPolicy: Always
    serviceType: ClusterIP
  noSuchParam: junkValue

I purposefully added -p noSuchParam=junkValue to above command. I think the way we have things now, it doesn't allow setting a value for an invalid parameter. Although it's based on x-descriptors and not swagger. But I think we should still do that in validation.

I agree that this is a change in the behaviour, but now we use the swagger spec to validate the params, we should rely on the swagger spec. And the Redis swagger spec allows additional parameters (there is an additionalProperties field in the swagger spec, that indicates if an object can accept additional properties, and the Spec object of the Redis CRD does not specify it, so additional properties are accepted by the Redis swagger)

[kadel]

I agree that this is a change in the behaviour, but now we use the swagger spec to validate the params, we should rely on the swagger spec. And the Redis swagger spec allows additional parameters (there is an additionalProperties field in the swagger spec, that indicates if an object can accept additional properties, and the Spec object of the Redis CRD does not specify it, so additional properties are accepted by the Redis swagger)

+1 this works as I would expect. If there is an "additionalProperties": true it allows additional parameters. If "additionalProperties": false it correctly errors out.
I've just tested this with postgresql-operator.v0.1.1/Database which doesn't allow additional parameters.

▶ odo service create postgresql-operator.v0.1.1/Database ffff -p asdfasdf=ff

 ✗  validation failure list:
.asdfasdf in body is a forbidden property

[kadel]

The new output is great. Much better than before.
The output can still get a little bit overvelming with CRs with a lot of options, like in case of strimzi-cluster-operator.v0.25.0/Kafka which has hundrets of options. But this will probably require something else than plain terminal output, like interactive mode, to make sense of this much of parameters.

@feloy 👍 great work!

/approve
/lgtm

[anandrkskd]

/test psi-kubernetes-integration-e2e

[dharmit]

/lgtm

[openshift-bot]

/retest-required

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest-required

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest-required

Please review the full test history for this PR and help us cut down flakes.

[dharmit]

/hold

@feloy can you PTAL at the strfmt and validate imports in pkg/odo/cli/service/operator_backend.go?

[dharmit]

/lgtm cancel

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
2.0% Duplication

[feloy]

/test v4.8-integration-e2e

Failure [2.853 seconds]
odo devfile url command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_url_test.go:16
  Creating urls
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_url_test.go:114
    should not allow using tls secret if url is not secure [It]

[openshift-ci]

@feloy: 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
ci/prow/psi-k8s-ibmc-integration-e2e	fd7ba20	link	false	/test psi-k8s-ibmc-integration-e2e

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

[feloy]

/test v4.8-integration-e2e

• Failure [1.629 seconds]
odo devfile storage command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_storage_test.go:14
  When ephemeral is not set in preference.yaml
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_storage_test.go:380
    should not create a pvc to store source code  (default is ephemeral=true) [It]
skipped 361 lines unfold_more
[Fail] odo devfile delete command tests when a component is created [JustBeforeEach] should throw an error on an invalid delete command 
/go/src/github.com/openshift/odo/tests/helper/helper_cmd_wrapper.go:99

[Fail] odo link command tests for OperatorHub Operators are installed in the cluster when a component and a service are deployed [BeforeEach] when a link with between the component and the service is created with --bind-as-files and --inlined when odo push is executed should display the link in odo describe 

[dharmit]

/hold cancel

[dharmit]

/lgtm