-
Notifications
You must be signed in to change notification settings - Fork 162
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
ADR: Practical Component Composition #2690
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for zarf-docs canceled.
|
|
||
## Context | ||
|
||
Presently "composition" within Zarf is only possible at the package level. This can only be done with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can also be done locally with path: ../ but agree the skeleton is the more obtuse use case (and has been acknowledged in the past)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(skeleton is only used in the OCI case in Zarf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought both local path and OCI imports were considered "skeleton packages". They are the same in all respects, correct?
namespace: mattermost | ||
condition: "'{.status.phase}'=Ready" | ||
|
||
- name: mattermost-registry1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would you build one component vs another?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. Can you rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I answered this below in https://github.com/defenseunicorns/zarf/pull/2690/files/27b8b7d9640e31c00b6f51904e2aba58fe682120#r1679778222
tl;dr: zarf package create . --include [...<optional component>]
# - registry1.dso.mil/ironbank/opensource/mattermost/mattermost:9.9.0 | ||
images: | ||
- name: appropriate/curl | ||
newName: registry1.dso.mil/ironbank/redhat/ubi/ubi9-minimal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels verbose / complex to me (you'd have to match strings in a Helm post render which might not be reliable - these references could be anywhere including a command arg passed into an operator)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verbose relative to what? The proposed API allows you to avoid specifying Helm chart value overrides entirely while still declaring which images to include in the package.
you'd have to match strings in a Helm post render which might not be reliable
This should be performed as structural edits on the YAML, not by string matching. Here are the field specs that kustomize
looks at by default: https://github.com/kubernetes-sigs/kustomize/blob/c1de0301f5e71ffbecf77f5bc8687e7693878eb5/api/internal/konfig/builtinpluginconsts/images.go#L9-L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Racer159 @AustinAbro321 it just occurred to me today that this proposed images
API could be used to greatly improve the performance of our CI jobs for building/testing/publishing packages.
In any case where one component directly extends
another, we can of course build a new package from source (as we do with only.flavor
today), but we should be able to perform that composition directly on a previously built package that includes the component being extended.
This could result in significantly faster package builds, especially as we introduce the third unicorn
flavor. In cases where we are currently (re-)building entire packages from source for each flavor, we could manipulate a pre-built tarball to add or replace specified images
directly.
Using zarf package create
to compose components into packages:
# build package from source (with only required components by default)
zarf package create . > mattermost-slim.tar.zst
# build package from source including optional components
zarf package create . \
--include mattermost-registry1 \
--include mattermost-plugins
# build a new package _by adding optional components to a pre-built package_
zarf package create . \
--include mattermost-registry1 \
--include mattermost-plugins \
--from mattermost-slim.tar.zst
Using zarf package strip
to create minimal packages:
You could also imagine doing the opposite (i.e. build "full" package for testing and strip it down for published artifacts) with a new sub-command:
# build "full" package from source (including all optional components)
zarf package create . --include-all > mattermost-full.tar.zst
# strip optional component(s) from full package
zarf package strip mattermost-full.tar.zst \
--exclude mattermost-registry1 > mattermost-upstream.tar.zst
zarf package strip mattermost-full.tar.zst \
--exclude mattermost > mattermost-registry1.tar.zst
# by default `strip` excludes all non-required components to create a "slim" package
zarf package strip mattermost-full.tar.zst > mattermost-slim.tar.zst
before: | ||
- dir: plugins | ||
cmd: | | ||
docker buildx . -t uds-package-mattermost/mattermost-extra-plugins:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way to solve this could be at the UDS CLI level to allow an additional image to be included alongside a Helm override (may still need an API change to the Zarf SDK to support but may be cleaner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we could probably also get away from a docker build here ourselves and publish this image instead (and require others to do the same for theirs))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purposes of this proposal, it doesn't matter whether we are doing a local docker build or not. This was just a self-contained way to demonstrate the need for an optional component to bundle both an additional image and related Helm chart value overrides.
|
||
1. `components[].extends` (`string`): delcares this component as an extension (overlay) of another component. | ||
Similar to `flavor`, the resulting component is considered the "deployable unit" and cannot be deployed alongside the component it extends. | ||
2. `components[].requires` (`[]string`): similar to `extends`, declares this component as an extension (overlay) of another component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zarf has discussed boing from required
-> optional
but this may conflict with required
semantics currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does conflict, I think any components that use components[].requires
to declare dependencies on other named components within the package MUST be considered optional. Same with components[].extends
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How far are we planning on taking this? This basically means that we need to implement a DAG for the components to make sure that we don't end up with cyclical references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phillebaba is that bad? What prevents cyclical path imports
right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing, but that does not mean that we should add to the existing complexity. My feeling is that we are trying to partially implement Kustomize features in Zarf. Before we do that we should probably look at Kustomize and the challenges it has faced. Mixing declarative YAML with imperative functions is difficult and tends to become messy in complex situations. Which is why Helm is so popular even with all of its flaws. I am no fan of mixing Go templating with YAML but there is a reason people prefer that over Kustomize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stand corrected, was not aware that we were already doing this. I still do not think this changes my worries about implementing Kustomize like features into Zarf with all the benefits and challenges that entails.
charts: | ||
- name: mattermost-enterprise-edition | ||
valuesFiles: | ||
- values/extra-plugins-values.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part could be interesting and is probably the main practical issue - optional components can't influence other components when they are selected - if a component could set a chartOverride
map to be applied to later components that could be interesting - you would also need component name here as well to map those in correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The merge strategies for "named primitive arrays" (like charts
in this example) is already well-defined. Why would further extending the API with something like chartOverride
be preferable?
you would also need component name here as well to map those in correctly
The component name that you are extending in this example is defined by requires: [ mattermost ]
.
Description
Composition within Zarf packages feels clunky and difficult right now. Specifically, there is no way to declare a self-contained "optional" component that overrides Helm chart values (or otherwise modifies the configuration of component(s) it is intended to be used with).
Checklist before merging