Skip to content

OCPBUGS-45295: Make plugins name array items unique #2117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-45295: Make plugins name array items unique #2117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27015,7 +27015,8 @@
"items": {
"type": "string",
"default": ""
}
},
"x-kubernetes-list-type": "atomic"
},
"providers": {
"description": "providers contains configuration for using specific service providers.",
Expand Down
26 changes: 26 additions & 0 deletions operator/v1/tests/consoles.operator.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,3 +296,29 @@ tests:
visibility:
state: Enables
expectedError: "spec.customization.capabilities[0].visibility.state: Unsupported value: \"Enables\": supported values: \"Enabled\", \"Disabled\""
- name: Should be able to create a Console with multiple plugins
initial: |
apiVersion: operator.openshift.io/v1
kind: Console
spec:
plugins:
- test-plugin-1
- test-plugin-2
expected: |
apiVersion: operator.openshift.io/v1
kind: Console
spec:
logLevel: Normal
operatorLogLevel: Normal
plugins:
- test-plugin-1
- test-plugin-2
- name: Should throw an error for duplicate plugin name
initial: |
apiVersion: operator.openshift.io/v1
kind: Console
spec:
plugins:
- test-plugin-1
- test-plugin-1
expectedError: "spec.plugins: Invalid value: \"array\": each plugin name must be unique"
9 changes: 8 additions & 1 deletion operator/v1/types_console.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,22 @@ type ConsoleSpec struct {
// +optional
Route ConsoleConfigRoute `json:"route"`
// plugins defines a list of enabled console plugin names.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While updating the constraints for this field, it would be good to update the godoc as well to explain in prose what validations exist for this field. Users can't see the markers when using something like oc/kubectl explain so it is helpful to spell it out for them in the godoc.

https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc is a good place to start when thinking about what to include in the godoc

// +listType=atomic
// +kubebuilder:validation:MaxItems=64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the number 64 chosen?

Is it possible for there to be existing instances of this resource with more than 64 items?

// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="each plugin name must be unique"
// +optional
Plugins []string `json:"plugins,omitempty"`
Plugins []PluginName `json:"plugins,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When did this API ship?

Is there a finite, enumerable list of plugins, that we as RH control?

What happens if the list is not already completely unique? Have you tested this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no its not an enum cause it would be hard to keep track of plugins since they could be provided by 3rd party.

What happens if the list is not already completely unique? Have you tested this?

so console-operator is already taking care of it programatically and makes the set of enabled plugins unique. The issue is mainly in this array, which can cause confusion, when adding plugins with the same name.

// ingress allows to configure the alternative ingress for the console.
// This field is intended for clusters without ingress capability,
// where access to routes is not possible.
// +optional
Ingress Ingress `json:"ingress"`
}

// +kubebuilder:validation:MaxLength=128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was a max length of 128 chosen?

Are there any plugins we are aware of that might be cutting it close to that 128 character limit?

Where are plugin names derived from?

// +kubebuilder:validation:Pattern=^[a-zA-Z0-9-]+$
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple things:

  • There didn't appear to be any pattern validation on the value previously. Why was this pattern chosen? Would introducing this pattern result in plugins that are no longer able to be used?
  • Conventions are shifting away from using the kubebuilder:validation:Pattern marker in favor of writing a CEL validation, using the kubebuilder:validation:XValidation marker, to provide users a more clear message when their input is invalid. If this pattern stays, update this to use a CEL validation instead?

type PluginName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs godoc with some more information about the type and the validations on it. Good guideline for writing human readable go doc can be found here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure that the detailed documentation ends up on the field itself, rather than the type alias.

The CRD schema will pick up godoc from the field, but sadly not the type alias.


// ConsoleConfigRoute holds information on external route access to console.
// DEPRECATED
type ConsoleConfigRoute struct {
Expand Down
7 changes: 7 additions & 0 deletions operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -779,8 +779,15 @@ spec:
plugins:
description: plugins defines a list of enabled console plugin names.
items:
maxLength: 128
pattern: ^[a-zA-Z0-9-]+$
type: string
maxItems: 64
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: each plugin name must be unique
rule: self.all(x, self.exists_one(y, x == y))
providers:
description: providers contains configuration for using specific service
providers.
Expand Down
2 changes: 1 addition & 1 deletion operator/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions ...1/zz_generated.featuregated-crd-manifests/consoles.operator.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,8 +780,15 @@ spec:
plugins:
description: plugins defines a list of enabled console plugin names.
items:
maxLength: 128
pattern: ^[a-zA-Z0-9-]+$
type: string
maxItems: 64
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: each plugin name must be unique
rule: self.all(x, self.exists_one(y, x == y))
providers:
description: providers contains configuration for using specific service
providers.
Expand Down