Skip to content
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.

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

Addon dependencies version range #5861

Closed
merusso opened this issue Apr 7, 2023 · 6 comments · Fixed by #6002
Closed

Addon dependencies version range #5861

merusso opened this issue Apr 7, 2023 · 6 comments · Fixed by #6002
Labels
area/addon dependencies Pull requests that update a dependency file

Comments

@merusso
Copy link
Contributor

merusso commented Apr 7, 2023

As a KubeVela addon developer, when defining dependencies for an addon in metadata.yaml, I would like to set a version range for these dependencies, so that my addon will be restricted from being enabled in a cluster where the dependency version doesn't match the specified range.

I see in the docs that there is a dependencies[].version property, but KubeVela doesn't seem to use this value to enforce any version requirements. Ideally this property would work similarly to the existing system property.

For example, in "metadata.yaml":

name: my-addon
version: 0.1.0

dependencies:
  - name: fluxcd
    version: ">= 2.3.3, < 3.0.0"
  - name: foo
    version: ">= 0.1.0, < 1.0.0"

system:
  vela: ">= v1.7.0"
  kubernetes: ">= 1.24.0"

Specific behavior:

  • When enabling addon, check whether dependencies are enabled within existing cluster.
  • For any dependency that is already enabled, verify that version matches provided version range.
    • If version is outside range, fail. Do not update dependency version.
  • For any dependency that is not enabled, attempt to enable while considering version range.
    • Fetch all versions of the addon, and enable the latest version that matches the version range.
    • If no addon version is available that matches the version range, fail.
@merusso
Copy link
Contributor Author

merusso commented Apr 11, 2023

As discussed in our most recent community meeting, this would introduce a breaking change, since existing usages of dependencies.version do not currently enforce any constraint.

We can avoid this issue by using a new property, but this leads to other questions:

  • What's a good name for this new property?
  • Should this new property deprecate (and eventually replace) the old property?
  • Are these properties mutually exclusive? If not, what's the behavior when both are used?

Alternatively, we could add a feature flag to enable the version constraint behavior. Only if enabled, KubeVela checks whether the enabled version matches the version constraint and fails if the addon doesn't meet the constraint. The flag would be disabled by default to preserve backward compatibility.

@wonderflow
Copy link
Collaborator

Hi @merusso

I walked through all our addons in the catalog, almost all of the addons use dependency without specifying the version, like:

dependencies:
  - name: fluxcd

In this case, we can keep the compatiblity for this schematic: 1) check the existence of the dependency, 2) install the latest one if not exists.

When specify the version, we can make the version range feature (which is also a constraint) work.

@merusso
Copy link
Contributor Author

merusso commented Apr 15, 2023

Should this issue be moved to https://github.com/kubevela/kubevela?

@wonderflow
Copy link
Collaborator

@merusso Yes, moving

@wonderflow wonderflow transferred this issue from kubevela/community Apr 16, 2023
@wonderflow wonderflow added dependencies Pull requests that update a dependency file area/addon labels Apr 16, 2023
@Hanmengnan
Copy link
Member

I agree with @wonderflow's point of view. For addons that dependents other addons, we can specify the version range of other dependent addons in the dependencies field. The system field is used to specify the current addon's dependent system environment. It is not appropriate to place the version range of dependent addons here.

@merusso
Copy link
Contributor Author

merusso commented Apr 16, 2023

Sorry if I was unclear, but this is what I'm proposing above. I'm proposing that dependencies[].version act as a semver constraint, similar to the system properties.

wangyikewxgm pushed a commit that referenced this issue Jun 7, 2023
* Feat(#5861): Support addon dependencies version ranges

This change enables addon maintainers to define version ranges for
dependencies in an addon's metadata.yaml file.

This behavior is similar to the version range allowed in the `system`
section of the metadata file. The version range expression for
`dependencies` follows the same format as for `system`.

Example:

```yaml
dependencies:
  - name: addon1
    version: ">= 2.3.3, < 3.0.0"
  - name: addon2
    version: ">= 0.1.0, < 1.0.0"
```

When installing an addon, the behavior varies depending on whether the
dependency is already installed.

If a dependency is already installed, the installed version will be
validated against the version range, and installation will fail with an
error if there's a mismatch.
If a dependency is not installed, the version range will be used to
select the addon version to be installed. If no addon version matching
the range exists, the installation will fail with an error.

Fixes #5861

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): remove unused ctx parameter

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): Add comment for IsLocalRegistry

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): unexport AddonInfoMap

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): unexport addonInfo

Signed-off-by: Michael Russo <merusso@gmail.com>

* chore: replace map[string]addonInfo with addonInfoMap for consistency

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix: add short circuit when dependency version is not specified

Signed-off-by: Michael Russo <merusso@gmail.com>

* feat: Add test for multiple validation errors

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix: Run go mod tidy

Signed-off-by: Michael Russo <merusso@gmail.com>

* feat: add tests for ToVersionedRegistry

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix: simplify listInstalledAddons loop

Signed-off-by: Michael Russo <merusso@gmail.com>

* feat: listAvailableAddons returns addons from multiple sources

Changes:
* implement ListAddonInfo in Registry
* add interface to aid testing of listAvailableAddons
* add tests for listAvailableAddons

Signed-off-by: Michael Russo <merusso@gmail.com>

* refactor: simplify validateAddonDependencies

move logic from validateAddonDependencies to
calculateDependencyVersionToInstall.

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): Implicit memory aliasing in for loop.

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): non-wrapping format verb for fmt.Errorf

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): indent-error-flow: (revive)

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): unexported-return

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): exported type comment format (revive)

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): refactor AddonInfo to ItemInfo, avoid "stutter" (revive)

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): add comment to exported method Registry.ListAddonInfo

Signed-off-by: Michael Russo <merusso@gmail.com>

* fix(lint): fix stutter, rename AddonInfoLister to ItemInfoLister

Signed-off-by: Michael Russo <merusso@gmail.com>

* chore: Add suite tests for Registry.ListAddonInfo()

Signed-off-by: Michael Russo <merusso@gmail.com>

* Test: add test cases for addon.sortVersionsDescending

Signed-off-by: Michael Russo <merusso@gmail.com>

---------

Signed-off-by: Michael Russo <merusso@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants