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.

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

Jump to bottom

ADR: Practical Component Composition #2690

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

ADR: Practical Component Composition #2690

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4eddd18
ADR for component-level composition
marshall007 Jul 2, 2024
1a9a3e2
update context summary
marshall007 Jul 2, 2024
27b8b7d
minor fixes to example YAML
marshall007 Jul 2, 2024
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
282 changes: 282 additions & 0 deletions adr/0025-composition.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,282 @@
# 25. Practical Component Composition

Date: 2024-07-02

## Status

Draft

## Context

Presently "composition" within Zarf is only possible at the package level. This can only be done with a
Copy link
Contributor

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)

Copy link
Contributor

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)

Copy link
Author

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?

special kind of package, a "skeleton package". The actual "composition" of these "skeleton packages" into proper
packages is then supported by the `import` and `flavor` APIs.

We use "composition" (in quotations) here because this is not true composition. 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).

According to the
[Wikipeida article on _Composability_](https://en.wikipedia.org/wiki/Composability):

> A highly composable system provides components that can be selected and assembled in various combinations to satisfy
> specific user requirements. In information systems, the essential features that make a component composable are that it be:
>
> - self-contained (modular): it can be deployed independently – note that it may cooperate with other components,
> but dependent components are replaceable
> - stateless: it treats each request as an independent transaction, unrelated to any previous request

Neither of these criteria are met in the context of Zarf components and packages. Here is a practical example from
[`defenseunicorns/uds-package-mattermost`](https://github.com/defenseunicorns/uds-package-mattermost/blob/5e02c2ceb7b0e097b7e6eb356b19eaff4c913613/zarf.yaml):

1. The `mattermost-(upstream|registry1)` component flavors depend on a `common` "skeleton package".
The `common` package cannot be deployed independantly, which violates the "self-contained" principle.
2. The `mattermost-plugins` component is not "stateless". It must be declared first because it
builds a container image during `onCreate` that is expected by the other components.
3. `mattermost-plugins` is not "self-contained" because, in order to use it,
you must override Helm chart values declared by other components.

```yaml
kind: ZarfPackageConfig
metadata:
name: mattermost
description: "UDS Mattermost Package"
version: "9.9.0-uds.0"

variables:
- name: SUBDOMAIN
description: "Subdomain for Mattermost"
default: "chat"
- name: DOMAIN
default: "uds.dev"
- name: ACCESS_KEY
description: "Access Key for S3 compatible storage"
- name: SECRET_KEY
description: "Secret Key for S3 compatible storage"
- name: DB_PASSWORD
description: "Database Password for Mattermost"

components:
- name: mattermost-plugins
required: true
images:
- uds-package-mattermost/mattermost-extra-plugins:latest
actions:
onCreate:
before:
- dir: plugins
cmd: |
docker build . -t uds-package-mattermost/mattermost-extra-plugins:latest

- name: mattermost
required: true
description: "Deploy Mattermost"
import:
path: common
only:
flavor: upstream
charts:
- name: mattermost-enterprise-edition
valuesFiles:
- values/upstream-values.yaml
images:
- appropriate/curl:latest
- mattermost/mattermost-enterprise-edition:9.9.0

- name: mattermost
required: true
description: "Deploy Mattermost"
import:
path: common
only:
flavor: registry1
cluster:
architecture: amd64
charts:
- name: mattermost-enterprise-edition
valuesFiles:
- values/registry1-values.yaml
images:
- registry1.dso.mil/ironbank/redhat/ubi/ubi9-minimal:9.4
- registry1.dso.mil/ironbank/opensource/mattermost/mattermost:9.9.0
```

### Proposed Solutions

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.
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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.

Copy link
Contributor

@Noxsios Noxsios Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/defenseunicorns/zarf/blob/014d1d22544c1b85f2444b9cce7584eac6a70ab2/src/pkg/packager/composer/list.go#L163-L174

Copy link
Member

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.

However, unlike `extends`, the required component(s) are not replaced. Instead, the resulting component is considered an optional
extention (overlay) to the required components. It can only be included when all required components are also included.
3. `images` (~~`[]string`~~ `[]{ name, newName?, newTag? }`): adopt
[`ImageTagTransformer` semantics from Kustomize](https://kubectl.docs.kubernetes.io/references/kustomize/builtins/#_imagetagtransformer_)
- `name`: the image name present in the component manifests
- `newName` (optional): the new image name you wish to use
(useful for changing registry locations)
- `newTag` (optional): the new image tag you wish to reference
(useful for updating version tags without modifying or relying on Helm chart values)

```yaml
kind: ZarfPackageConfig
metadata:
name: mattermost
description: "UDS Mattermost Package"
version: "9.9.0-uds.0"

variables:
- name: SUBDOMAIN
description: "Subdomain for Mattermost"
default: "chat"
- name: DOMAIN
default: "uds.dev"
- name: ACCESS_KEY
description: "Access Key for S3 compatible storage"
- name: SECRET_KEY
description: "Secret Key for S3 compatible storage"
- name: DB_PASSWORD
description: "Database Password for Mattermost"

components:
- name: mattermost
description: "Deploy Mattermost"
required: true

## The `mattermost` component is now a self-contained, deployable unit.
## Thus, all the configuration from `common/zarf.yaml` has been inlined
## into this example. We no longer import a `common` "skeleton package".
#
# import:
# path: common
# only:
# flavor: upstream
charts:
- name: uds-mattermost-config
namespace: mattermost
version: 0.1.0
localPath: ./chart
valuesFiles:
- values/config-values.yaml
- name: mattermost-enterprise-edition
namespace: mattermost
url: https://helm.mattermost.com
gitPath: chart
version: 2.6.55
valuesFiles:
- values/common-values.yaml

## No longer needed because this was only used for replacing
## image tags, see new `images` API usage below.
#
# - values/upstream-values.yaml

## Kustomize-style image replacements facilitate proper composition.
## This makes it easy for downstream components to override image tags
## without knowing anything about the Helm chart(s) being referenced
## nor their chart-specific supported `values`.
#
# images:
# - appropriate/curl:latest
# - mattermost/mattermost-enterprise-edition:9.9.0
images:
- name: appropriate/curl
newTag: latest
- name: mattermost/mattermost-enterprise-edition
newTag: 9.9.0
actions:
onDeploy:
after:
- description: Validate Mattermost Package
maxTotalSeconds: 300
wait:
cluster:
kind: Packages
name: mattermost
namespace: mattermost
condition: "'{.status.phase}'=Ready"

- name: mattermost-registry1
Copy link
Contributor

@Racer159 Racer159 Jul 2, 2024 •
edited
Loading

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?

Copy link
Author

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?

Copy link
Author

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>]


## Previously a `required` component with the "registry1" `flavor`,
## `mattermost-registry1` is now defined simply as a component that directly
## extends the default `mattermost` component above.
#
# required: true
# import:
# path: common
# only:
# flavor: registry1
# cluster:
# architecture: amd64
extends: mattermost
required: false

## In this case (and as is the case with most `registry1` component flavors)
## the Helm chart values were only used to override image tags. We can now
## take advantage of a more robust and declarative `images` replacements API.
#
# charts:
# - name: mattermost-enterprise-edition
# valuesFiles:
# - values/registry1-values.yaml
# ## sample contents of `values/registry1-values.yaml`
# # mattermostApp:
# # image:
# # repository: registry1.dso.mil/ironbank/opensource/mattermost/mattermost
# # tag: 9.9.0
# # initContainerImage:
# # repository: registry1.dso.mil/ironbank/redhat/ubi/ubi9-minimal
# # tag: 9.4
# images:
# - registry1.dso.mil/ironbank/redhat/ubi/ubi9-minimal:9.4
# - registry1.dso.mil/ironbank/opensource/mattermost/mattermost:9.9.0
images:
- name: appropriate/curl
newName: registry1.dso.mil/ironbank/redhat/ubi/ubi9-minimal
Copy link
Contributor

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)

Copy link
Author

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

Copy link
Author

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

newTag: 9.4
- name: mattermost/mattermost-enterprise-edition
newName: registry1.dso.mil/ironbank/opensource/mattermost/mattermost
newTag: 9.9.0

## Finally, the most interesting example of *optionally* enabling injection of
## Mattermost plugins. This component is now "stateless" because it does nothing
## by default and it is "self-contained" because it both builds the required image
## and includes the necessary Helm chart values overrides
## (i.e. `mattermostApp.extraInitContainers`).
- name: mattermost-plugins
required: false

## Note that the semantics of `requires` is slightly different from `extends`.
## The idea is that `extends` signals intent to _replace_ the original component
## whereas `requires` signals that the originaly component(s) are being overlaid
## with additional configuration.
requires: [ mattermost ]
charts:
- name: mattermost-enterprise-edition
valuesFiles:
- values/extra-plugins-values.yaml
Comment on lines +253 to +256
Copy link
Contributor

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

Copy link
Author

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 ].

## sample contents of `values/extra-plugins-values.yaml`:
#
# mattermostApp:
# extraInitContainers:
# - name: mattermost-extra-plugins
# image: uds-package-mattermost/mattermost-extra-plugins:latest
# imagePullPolicy: Always
# volumeMounts:
# - name: mattermost-plugins
# mountPath: /mattermost/plugins/
images:
- name: uds-package-mattermost/mattermost-extra-plugins
newTag: latest
actions:
onCreate:
before:
- dir: plugins
cmd: |
docker buildx . -t uds-package-mattermost/mattermost-extra-plugins:latest
Copy link
Contributor

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)

Copy link
Contributor

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))

Copy link
Author

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.

```

## Decision


## Consequences

Loading