[DNM] (review-only) WIP: OLM v1 API review #2067
Conversation
Signed-off-by: everettraven <everettraven@gmail.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| type CatalogURLs struct { | ||
| // base is a required cluster-internal URL from which on-cluster components can access the API endpoint for this catalog | ||
| Base string `json:"base"` | ||
| } |
There was a problem hiding this comment.
This particular type is still a work in progress. For now we intend to only provide a base URL in which a user can build on top of to perform requests against the supported REST API for accessing this ClusterCatalog's content.
For more information, see https://docs.google.com/document/d/1XvQPwk-Ws1rVcBpNsAaDoYKlmlws-hiQ19Ex9D4zAok/edit?usp=sharing
There was a problem hiding this comment.
Relevant types changes here merged in operator-framework/catalogd#429 and released in v0.34.0 of catalogd.
There was a problem hiding this comment.
That is a shame, as you haven't added much validation to the field.
You would have benefited from adding CEL to validate that it was a valid URL, and imposing a maximum length to the field. I suppose we can add that for v1, though conversion might be tricky
There was a problem hiding this comment.
IIUC our current thinking is that, even though it isn't following best practices for API versioning, we can do a hard cut over to the v1 versions of the API when we release v1.0.0 of the upstream projects and GA in OpenShift 4.18 (although @joelanford please correct me if this is an incorrect understanding).
With that in mind, we can still add some validations without having to worry about conversion complexity.
There was a problem hiding this comment.
This section has been updated to reflect the most recent changes to the types that were merged for this API upstream. Leaving this thread unresolved in case there is more discussion required re: conversion/API versioning
There was a problem hiding this comment.
What are you expecting the transitional path to be for users of the existing v1alpha1 APIs? A hard cut does not sound like there is necessarily a migration path?
There was a problem hiding this comment.
That's a great question :) - my current understanding is that there is none. My general understanding is that doing a hard cut:
- Has no impact to supported OpenShift customers since these APIs only exist when
TechPreviewNoUpgradeis enabled meaning there is nothing to migrate when going GA in 4.18 - May have impact to users of the upstream project, but we seem to be OK with not providing a migration path there because, following SemVer, users should expect a breaking change going from a v0.Y.Z release to a v1.Y.Z release.
I don't think this approach is set in stone, but I'll have another chat with @joelanford later this afternoon and see if there is any expectations that we provide that transitional path and that we are OK with the consequences of not providing that path.
There was a problem hiding this comment.
May have impact to users of the upstream project, but we seem to be OK with not providing a migration path there because, following SemVer, users should expect a breaking change going from a v0.Y.Z release to a v1.Y.Z release.
If the cluster tries to upgrade to a new OLM, that is installing the v1 CRD, without the v1alpha1 version included, this would in theory mean that the data stored in etcd already, now has no representation in the v1 API, so, I think you'd lose all data there.
Also, any controllers running during this change, would break until they are upgraded to the new version of the API.
I'm not even sure if Kube will allow you to make that change to a CRD once its installed
I think you'll need to try this out if you want to say that v0 users aren't going to be provided with a migration path, it could mean they have to completely uninstall the operator, rather than upgrade, which in turn, means perhaps uninstalling the other operators installed by OLM right? The UX of that could be pretty bad
| } | ||
|
|
||
| // ClusterCatalogSpec defines the desired state of ClusterCatalog | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.source.image.pollInterval) || (self.source.image.ref.find('@sha256:') == \"\")",message="cannot specify PollInterval while using digest-based image" |
There was a problem hiding this comment.
Why is this here rather than at the image level?
There was a problem hiding this comment.
No good reason as far as I can tell. We should move it.
| // CatalogSource is a discriminated union of possible sources for a Catalog. | ||
| // CatalogSource contains the sourcing information for a Catalog | ||
| // +union | ||
| // +kubebuilder:validation:XValidation:rule="self.type == 'Image' && has(self.image)",message="source type 'Image' requires image field" |
There was a problem hiding this comment.
This forces the value of type to required, which is fine, but, would need to be changed if you ever added a new value
We typically use
| // +kubebuilder:validation:XValidation:rule="self.type == 'Image' && has(self.image)",message="source type 'Image' requires image field" | |
| // +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Image' ?has(self.image) : !has(self.image)",message="image is required when type is Image, and forbidden otherwise" |
You can then copy/paste this for new fields in the union without changing this one
There was a problem hiding this comment.
Nice! Thanks for this suggestion. That will be helpful for having a reasonable "message" when we add new types.
| // ref: quay.io/operatorhubio/catalog:latest # image reference | ||
| // ref: quay.io/operatorhubio/catalog@sha256:c7392b4be033da629f9d665fec30f6901de51ce3adebeff0af579f311ee5cf1b # image reference with sha256 digest |
There was a problem hiding this comment.
Somewhere in o/api there is an api that validates valid references, we should find that and copy their validation.
There was a problem hiding this comment.
The one in o/api is part of the catalog mirroring effort, it validates something slightly different
There are some constraints we can validate on here though right?
There was a problem hiding this comment.
Did some further investigation and the containers/image library that we use for parsing these references uses this regex:
^((?:(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?/)?[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?)(?::([\w][\w.-]{0,127}))?(?:@([A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*[:][[:xdigit:]]{32,}))?$
It's quite complex, but I'll see if I can break it down into some CEL expressions similar to how you mentioned we could attempt to break down some of our other pattern validations
There was a problem hiding this comment.
If it cannot be broken down, we could still use it as is, but it would be preferable to be broken down as mentioned elsewhere
| // pollInterval: 1h30m # poll the image source every 1 hour and 30 minutes | ||
| // | ||
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration |
There was a problem hiding this comment.
Have you tested that this actually works? And that non-duration like fields are rejected at admission time
There was a problem hiding this comment.
We are going to change this field to pollIntervalMinutes to avoid any pitfalls of using a duration.
| // For more information on semver, please see https://semver.org/ | ||
| // | ||
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$` |
There was a problem hiding this comment.
Using Pattern will print this regex to the end user, and to me this is write once, read never...
If you were to use CEL, you can provide a custom error message that might be a bit more user friendly
There was a problem hiding this comment.
Oh good call out.
For context, CEL has regex capabilities. See https://github.com/google/cel-spec/blob/master/doc/langdef.md#string-functions
We can transplant this pattern into a CEL expression using the CEL matches function.
There was a problem hiding this comment.
When moving this to CEL, you might also want to break it down into multiple validations rather than one huge regex
There was a problem hiding this comment.
Since this validation is of a grammar with position-sensitive elements, we really couldn't break this down into multiple validations.
Joe also talked a bit on the subject here
everettraven
changed the title
Signed-off-by: everettraven <everettraven@gmail.com>
openshift-ci
bot
added
size/XXL
Signed-off-by: everettraven <everettraven@gmail.com>
| // When set to "Enabled", the CRD Upgrade Safety pre-flight check will be run when | ||
| // performing an upgrade operation. | ||
| // | ||
| //+kubebuilder:validation:Enum:="Enabled";"Disabled" |
There was a problem hiding this comment.
Enabled/Disabled should be replaced with descriptive levels like
- InformationalOnly
- Strict
- Could later add: CriticalOnly.
There was a problem hiding this comment.
We talked about this for awhile yesterday and got hung up on InformationalOnly because we didn't think a user would actually be able see the information on the API.
We ended up settling on: CRDUpgradeSafetyEnforcement with None and Strict as options, and we could later add Permissive, CriticalOnly, etc. WDYT?
For InformationalOnly is there recommended way to provide that information to a user? Since this is an edge-based check, we couldn't immediately think of a way to put this in the API in a way that it would "stick". We mentioned recording an Event, but those are ephemeral.
For now, we landed on "None", which means "don't even run the checks", and in our controller we will log what the enforcement enum was set to when we reconcile so that we have more information to help reconstruct why a cluster might be borked when a customer reports an issue with a ClusterExtension.
|
|
||
| MetadataNameLabel = "olm.operatorframework.io/metadata.name" | ||
|
|
||
| AvailabilityEnabled = "Enabled" |
There was a problem hiding this comment.
Enabled/Disabled needs more dimension, and communicate better to those with less op-con context.
Proposed were something like "AvailableToBeUsed" and its reverse.
| // +optional | ||
| Priority int32 `json:"priority"` | ||
|
|
||
| // Availability is an optional field that allows users to define whether the ClusterCatalog is utilized by the operator-controller. |
There was a problem hiding this comment.
explain without references to a specific client and speak more to the general use-case.
| // Availability is an optional field that allows users to define whether the ClusterCatalog is utilized by the operator-controller. | ||
| // | ||
| // Allowed values are : ["Enabled", "Disabled"]. | ||
| // If set to "Enabled", the catalog will be used for updates, serving contents, and package installations. | ||
| // | ||
| // If set to "Disabled", catalogd will stop serving the catalog and the cached data will be removed. | ||
| // | ||
| // If unspecified, the default value is "Enabled" | ||
| // | ||
| // +kubebuilder:validation:Enum="Disabled";"Enabled" | ||
| // +kubebuilder:default="Enabled" | ||
| // +optional | ||
| Availability string `json:"availability,omitempty"` |
There was a problem hiding this comment.
Available should probably be a value, not a field name. A possibility
Available: Operators in this catalog are useable on the cluster.
Removed: Operators in this catalog are not usable on the cluster.
| // ref contains the resolved sha256 image ref containing Catalog contents. | ||
| Ref string `json:"ref"` | ||
| // lastSuccessfulPollAttempt is the time when the resolved source was last successfully polled for new content. | ||
| LastSuccessfulPollAttempt metav1.Time `json:"lastSuccessfulPollAttempt"` |
There was a problem hiding this comment.
Fields like this end up at as standing write load. Suggest using conditions instead with a threshold for number of failures before declaring a thing stale.
There was a problem hiding this comment.
remove this. Later, we want possibly to add conditions to capture 'staleness' of catalog contents.
There was a problem hiding this comment.
Dev note: I realize that we currently use this field from this struct to compute a next poll time. We don't actually read it from the status of the object that we get from etcd though, so it is safe to remove from the API.
We just re-use this struct in an in-memory map that tracks state. We'll have to refactor that map to keep track of the last poll time separately.
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration | ||
| // +optional | ||
| PollInterval *metav1.Duration `json:"pollInterval,omitempty"` |
There was a problem hiding this comment.
lower bound on the poll interval
There was a problem hiding this comment.
1 minute as the lower bound sounds good as discussed.
There was a problem hiding this comment.
1 minute means we have an e2e that will take ~1 minute to run, instead of the current configuration, which sets a poll interval as 1 second, and the test finishes in a couple of seconds.
That's not a good motivation for the design of an API, but just something for us to consider on the e2e side of things. Maybe we could run this test in parallel with others?
There was a problem hiding this comment.
Testing at a different poll interval than you'd expect real life users to leverage is likely to either expose bugs or mask them, so I'd encourage testing at a real-ish poll interval where possible
Out of interest, what do you expect the "default" or average to be? Every hour?
There was a problem hiding this comment.
The "default" that we set on the ClusterCatalog resources that ship as part of the OCP payload will be 10 minutes, which is the same as what the existing OLM v0 CatalogSource resources that ship as part of the OCP payload.
| // pollInterval: 1h30m # poll the image source every 1 hour and 30 minutes | ||
| // | ||
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration |
There was a problem hiding this comment.
consider adding a lower-bound to this, or adding CEL validation, or ... better yet, change to pollIntervalMinutes which sidesteps the problem.
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration | ||
| // +optional | ||
| PollInterval *metav1.Duration `json:"pollInterval,omitempty"` |
There was a problem hiding this comment.
What is the resync period for the informers in the controller that will be processing this? Does a full resync of the informer cause this to poll?
I'm wondering if the resync of the informer (which might be 10m, 10h, 24h...) actually sets an upper bound for the poll interval
There was a problem hiding this comment.
Unless we have a bug, resync does not cause a poll to occur.
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration | ||
| // +optional | ||
| PollInterval *metav1.Duration `json:"pollInterval,omitempty"` |
| // pollInterval: 1h30m # poll the image source every 1 hour and 30 minutes | ||
| // | ||
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration |
There was a problem hiding this comment.
Consider a force-refresh field, where if user changes the value from the spec we will take an action, which fulfills the user need to trigger an update w/o forcing a polling interval change
There was a problem hiding this comment.
There are a lot of places where my comments would/are duplicated, I didn't capture them all as I didn't want to constantly repeat myself.
For the most part I think the structure looks good, though I've left some comments where we might need more thought about structure.
The majority of what I've left is suggestions on the godocs and what I would consider missing validations.
| Availability is an optional field that allows users to define whether the ClusterCatalog is utilized by the operator-controller. | ||
|
|
||
| Allowed values are : ["Enabled", "Disabled"]. | ||
| If set to "Enabled", the catalog will be used for updates, serving contents, and package installations. | ||
|
|
||
| If set to "Disabled", catalogd will stop serving the catalog and the cached data will be removed. | ||
|
|
||
| If unspecified, the default value is "Enabled" |
There was a problem hiding this comment.
Out of interest, have you tried a kubectl explain on fields like this? I'm wondering if the output does render like this, or if it mucks with the whitespace.
Would be good to check that when a user explains some of your fields, that they are rendering as you expect
| ) | ||
|
|
||
| // SourceType defines the type of source used for catalogs. | ||
| // +enum |
There was a problem hiding this comment.
I'm not sure if this tag actually does anything, it may help the openapi generator perhaps
If you are intending to have this be an enum in a CRD, you need to have
// +kubebuilder:validation:Enum:=Image;Foo;Bar;...
There was a problem hiding this comment.
I'll plan to remove the tag. Where this is used in the types we do have the appropriate enum tag.
There was a problem hiding this comment.
Turns out this is what controls the openapi.json, so feel free to leave it.
Will see if we can get the tooling all to use a common tag in the future
| AvailabilityEnabled = "Enabled" | ||
| AvailabilityDisabled = "Disabled" |
There was a problem hiding this comment.
If these are for enums, use a string type alias as you have done with SourceType
| Spec ClusterCatalogSpec `json:"spec"` | ||
| Status ClusterCatalogStatus `json:"status,omitempty"` |
There was a problem hiding this comment.
Both of these should have godoc
Godoc should start with the json tag version of the name
Both should also have either // +kubeubilder:validation:Required or // +optional. We add an explicit optional vs required as there are ways to accidentally change this if it isn't specified explicitly (you can change the default at the package level, and there are other inferences that decide as well)
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ListMeta `json:"metadata"` | ||
|
|
||
| Items []ClusterCatalog `json:"items"` |
| // to be used for installation and management of the content for the package | ||
| // specified in the packageName field. | ||
| // | ||
| // This ServiceAccount is expected to exist in the installNamespace. |
There was a problem hiding this comment.
Be more direct?
| // This ServiceAccount is expected to exist in the installNamespace. | |
| // This ServiceAccount must exist in the installNamespace. |
| // ref: quay.io/operatorhubio/catalog:latest | ||
| // | ||
| // For more information on FBC, see https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs | ||
| Source CatalogSource `json:"source"` |
There was a problem hiding this comment.
Would there ever be a use case for having multiple sources, like mirrors of the same catalog?
There was a problem hiding this comment.
We don't envision a use case for this right now, but if they do arise, we think that there would be a reasonable way to implement something like this on a per source level without it being a breaking change. That being said, we haven't done a deep dive into how or what that design would look like. The existing OLM v0 CatalogSource API only supports a single image source per CatalogSource and we have not had any RFEs for being able to specify multiple.
For the mirroring use case in particular, we already respect the standard mirroring configurations when using things like ITMS, IDMS, etc. in OpenShift.
| ) | ||
|
|
||
| // ClusterExtensionSpec defines the desired state of ClusterExtension | ||
| type ClusterExtensionSpec struct { |
There was a problem hiding this comment.
Source and Install could well be interpreted to be quite similar, eg, why is the source not part of the installation configuration, I wonder if changing the names might make this more obvious, or perhaps, source should be within install? Or the fields from install should just be promoted to the spec?
There was a problem hiding this comment.
The loose idea is that there are stages:
- source the bundle from $somewhere (for now, from a catalog. in the future, perhaps from a direct reference or from a helm repo)
- (not implemented yet) template/render the bundle using cluster-admin defined configuration
- install it (i.e. change the cluster to reflect the result of (2))
So the idea was to have logical groupings of fields in the API so that fields from different concepts would not be interspersed.
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$` | ||
| //+optional | ||
| Version string `json:"version,omitempty"` |
There was a problem hiding this comment.
Would this be easier to reason about, if you created a structure to capture the version constraints, rather than trying to regex match and explain this massive validation?
For example, just splitting this into a list of strings, and saying that a match of any item in the list is considered a match, would remove the need to account for the OR in this.
Thinking about the ~ and ^ usage, is that not effectively a form of pinning to either the major, minor or patch versions of the version?
Could a field that was an enum represent that choice? upgradePinning: Major | Minor represent something equivalent? (I don't think you need patch)
One of my concerns about the validation here at the moment, is that there are many ways to express the same thing
What if this was something like
version:
- base: 1.2.0 # This would be ">=1.2.0, < 1.3.0" or "1.2.x" or "~1.2.0" in the current form
operator: GreaterOrEqual
pinnedAt: Minor
There was a problem hiding this comment.
Jordan, Lala, Bryce, and I chatted about this for a while yesterday. Our conclusion is that we believe masterminds implements a standard version range syntax used in other well-known parts of the Kubernetes ecosystem (e.g. helm install --version) and handles edge cases well (e.g. pre-release versions are only included in the range if the range itself specifies versions with pre-release values), so we were all agreed that we desired for the implementation to remain.
With that settled, the next questions we discussed were:
- Can we confidently translate from a syntax like the one you are proposing back into masterminds syntax?
- Are we sure that our pre-translation API would be expressive enough to cover all of the possible masterminds constraint strings? If it was, would that API still be more intuitive than a version range string?
For (1), we thought that we probably could, but it would require an extensive test suite that we would need to maintain. For (2), we were not confident.
Lastly, we talked about whether we thought there was a significant enough UX gain by going with a structured syntax, and we didn't think there was. We believe that the GoDoc on the field, the alignment with a well-known library, and the simplicity of a single string might actually be overall more intuitive than a verbose structure would be.
| // # OR Comparisons | ||
| // You can use the "||" character to represent an OR operation in the version | ||
| // range. Some examples: | ||
| // - ">=1.2.3, <2.0.0 || >3.0.0" |
There was a problem hiding this comment.
If I wanted to exclude a specific version from being valid, do I then just have to create ORs on a range around the version I want to exclude?
There was a problem hiding this comment.
You could do something like this:
version: "2.x, !=2.1.2"
|
here is example output for describing the ClusterExtension CRD |
The Operator Framework group has been working on a re-vamp of the Operator Lifecycle Manager project that we have dubbed "OLM v1". Our upstream project has adopted the OpenShift API conventions and since we intend for it to go GA in OpenShift 4.18 we wanted to get the APIs reviewed before GA.
This PR adds a new
olmdirectory containing the Go types for the two APIs that would be introduced:ClusterExtensionClusterCatalogEach API is under a separate sub-directory in this PR because while they are fairly coupled, the APIs live in separate upstream projects:
ClusterExtensionAPI lives in the upstream project https://github.com/operator-framework/operator-controller (which has a downstream equivalent at https://github.com/openshift/operator-framework-operator-controller) and as such I named the subdirectoryoperator-controllerClusterCatalogAPI lives in the upstream project https://github.com/operator-framework/catalogd (which has a downstream equivalent at https://github.com/openshift/operator-framework-catalogd) and as such I named the subdirectorycatalogdAlso of note for reviewers, there is a portion of theClusterCatalogAPI that is still a work-in-progress, but was included in this PR with what we anticipate it to look like. I will leave PR comments on the exact locations with references to the design document we have in place for the API change and functionality intended.