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

Zarf does not support deploying Flux HelmReleases with OCI Helm chart sources #1974

Open
bburky opened this issue Aug 17, 2023 · 16 comments · May be fixed by #2005
Open

Zarf does not support deploying Flux HelmReleases with OCI Helm chart sources #1974

bburky opened this issue Aug 17, 2023 · 16 comments · May be fixed by #2005
Assignees
Labels
enhancement ✨ New feature or request
Milestone

Comments

@bburky
Copy link
Member

bburky commented Aug 17, 2023

Is your feature request related to a problem? Please describe.

Zarf does not support deploying Flux HelmReleases with OCI Helm chart sources

Related issue

Describe the solution you'd like

  • Given
    • A zarf.yaml with manifests: including a Flux HelmRepository referencing an oci:// chart and a HelmRelease referencing it.
    • And including the OCI helm chart in images: in zarf.yaml.
  • When creating the package
  • Then
    • The package cannot be created because images: does not support arbitrary (non-container) OCI images
      ERROR:  Failed to create package: unable to pull images after 3 attempts: error when trying to save the img                                                                   
              (ghcr.io/stefanprodan/charts/podinfo:6.4.1): mismatched fs layers (1) and diff ids (0)
      
    • Additionally, Zarf's mutating admission webhooks do not support mutating HelmRepository and HelmRelease resources to reference the Zarf registry

Example Flux manifests referencing an OCI Helm chart: https://fluxcd.io/flux/cheatsheets/oci-artifacts/#helm-oci

A potential issue: Zarf uses an insecure registry and Flux source-controller may not support insecure OCI registries with Helm: fluxcd/source-controller#807

  • A little unclear on what's affected and if this issue is still current
  • OCIRepository (consumed by kustomize-controller, not helm-controller) does support .spec.insecure
  • Need to check the current status of HelmRepository and HelmReleases with an insecure localhost registry (sometimes localhost is special cased and allowed)

Describe alternatives you've considered

Zarf's built in Helm support does allow OCI, see demo-helm-oci-chart in this example: https://docs.zarf.dev/examples/helm-charts/

However, this does not support using Flux.

Additional context

The scenario prompting this is a vendor only provides OCI packaged Helm charts. We do not need to modify the upstream vendor charts, and would like to copy them unmodified, and deploy them using Flux.

Additionally, Big Bang now distributes Helm charts in OCI format. If Zarf supported this, the dependency on a Gitea git repository could be dropped in many cases.

Support for generic OCI artifacts may be generally useful for other non-Flux use cases too.

@bburky bburky added the enhancement ✨ New feature or request label Aug 17, 2023
@cmwylie19 cmwylie19 self-assigned this Aug 25, 2023
@joelcomp1
Copy link

+1 this, I would also love to have this exact feature for our Flux Releases

@cmwylie19
Copy link
Contributor

cmwylie19 commented Sep 3, 2023

Just starting to investigate this and wanted to circle back to update the issue:

In terms of the admission controller (Zarf Agent), this development looks like it would touch (for Flux OCIArtifacts):
HelmRepository

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: HelmRepository
metadata:
  name: podinfo
  namespace: flux-system
spec:
  interval: 10m
  type: oci
  url: oci://ghcr.io/stefanprodan/charts # <- HERE

OCIRepository

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: OCIRepository
metadata:
name: podinfo
namespace: flux-system
spec:
interval: 10m
url: oci://ghcr.io/stefanprodan/manifests/podinfo # <- Here 
ref:
  tag: latest

In term's of Zarf's transform library, we should already be capable of transforming the URL as long as we emit the oci:// protocol internally. Example here

The biggest complexity that I am aware based on the issue of seems like it would be around the insecure registry which needs more investigation. I think worst case we would look at securing the Zarf internal registry possibly

@cmwylie19 cmwylie19 linked a pull request Sep 4, 2023 that will close this issue
5 tasks
@bburky
Copy link
Member Author

bburky commented Sep 5, 2023

There are at least 3 tasks:

  1. Admission webhook update to rewrite HelmRepository and OCIRepository with Zarf registry
  2. Add support for packaging arbitrary OCI artifacts, one option is just telling people to add them to images:. However there is an issue that (crane?) throws the mismatched fs layers (1) and diff ids (0) error mentioned above.
  3. Test if HelmRepository supports an insecure localhost registry.

@joelcomp1
Copy link

If at all helpful, for #3 listed above, I had to do this internally for a corporate network with Flux HelmRepository and the only work around I could easily get working was to patch the deployment with the CA that I had into the container like mentioned here: fluxcd/flux2#3417 (comment) i think if its completely insecure (no HTTPS) then I am not sure its at all possible based on what I have tried to do with Flux before and insecure OCI.

@cmwylie19
Copy link
Contributor

cmwylie19 commented Sep 6, 2023

There are at least 3 tasks:

  1. Admission webhook update to rewrite HelmRepository and OCIRepository with Zarf registry
  2. Add support for packaging arbitrary OCI artifacts, one option is just telling people to add them to images:. However there is an issue that (crane?) throws the mismatched fs layers (1) and diff ids (0) error mentioned above.
  3. Test if HelmRepository supports an insecure localhost registry.

FWIW, for #(2), i think we have to assume they are bringing their images ready to go. So they basically make a scratch container with their manifests. However, there are other blockers in terms of our internal registry before getting to point 3. I'll leave a comment about it for the future when this can be fixed.

Actually, going to confirm with the team first. Thanks for the updates

@bburky
Copy link
Member Author

bburky commented Sep 6, 2023

So they basically make a scratch container with their manifests

Not quite. These are OCI artifacts (not quite, that specific term can mean something else), not actual "images". These aren't "Docker images" at all (the layers are not application/vnd.oci.image.layer.v1.tar+gzip tarball layers with a application/vnd.oci.image.index.v1+json config). This is packaging non-Docker images as into "OCI images" something still compatible enough with registries to work, but storing different types of data (Helm charts in this case). You'd use something like helm push to create these OCI images.

@Noxsios would be familiar and probably help explain. It's also how Zarf's own OCI support works (Zarf uses ORAS, Helm does something slightly different, but it's the same concept).

And yes you can assume these images already exist, they're created outsize Zarf. Zarf does need to package and transfer them all the way to the Zarf registry somehow though.

In theory these generic OCI images can be copied around the same way as Docker images, which is why I suggested listing them in images:, but it seems something throws an error while downloading them.

@cmwylie19
Copy link
Contributor

I'm not able to pull from the internal-docker-registry for some reason, may be oci:// url. Needs more investigation

┌─[cmwylie19@Cases-MacBook-Pro] - [~/hello-zarf] - [2023-09-06 02:45:59]
└─[0] <git:(main 0538a87✱) > k explain ocirepo.spec --recursive
GROUP:      source.toolkit.fluxcd.io
KIND:       OCIRepository
VERSION:    v1beta2

FIELD: spec <Object>

DESCRIPTION:
    OCIRepositorySpec defines the desired state of OCIRepository
    
FIELDS:
  certSecretRef <Object>
    name        <string> -required-
  ignore        <string>
  interval      <string> -required-
  layerSelector <Object>
    mediaType   <string>
  provider      <string>
  ref   <Object>
    digest      <string>
    semver      <string>
    tag <string>
  secretRef     <Object>
    name        <string> -required-
  serviceAccountName    <string>
  suspend       <boolean>
  timeout       <string>
  url   <string> -required-

┌─[cmwylie19@Cases-MacBook-Pro] - [~/zarf] - [2023-09-06 02:46:59]
└─[0] <git:(1974 46635184✱✈) > k get ocirepo hello-zarf -n flux-system -oyaml
apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: OCIRepository
metadata:
  annotations:
    meta.helm.sh/release-name: zarf-1ac628beb6600ac961cf019ef1e52045b7fcbb0a
    meta.helm.sh/release-namespace: flux-system
  creationTimestamp: "2023-09-07T11:56:10Z"
  finalizers:
  - finalizers.fluxcd.io
  generation: 1
  labels:
    app.kubernetes.io/managed-by: Helm
  name: hello-zarf
  namespace: flux-system
  resourceVersion: "22934"
  uid: ef430da6-2ba5-4854-938b-067eef2c520b
spec:
  interval: 10m
  provider: generic
  ref:
    tag: latest-zarf-1662488912
  secretRef:
    name: private-registry
  timeout: 60s
  url: oci://127.0.0.1:31999/cmwylie19/oci-manifests-hello-zarf
status:
  conditions:
  - lastTransitionTime: "2023-09-07T11:56:11Z"
    message: no artifact for resource in storage
    observedGeneration: 1
    reason: NoArtifact
    status: "True"
    type: Reconciling
  - lastTransitionTime: "2023-09-07T11:56:11Z"
    message: 'failed to pull artifact from ''oci://127.0.0.1:31999/cmwylie19/oci-manifests-hello-zarf'':
      Get "https://127.0.0.1:31999/v2/": dial tcp 127.0.0.1:31999: connect: connection
      refused; Get "http://127.0.0.1:31999/v2/": dial tcp 127.0.0.1:31999: connect:
      connection refused'
    observedGeneration: 1
    reason: OCIArtifactPullFailed
    status: "False"
    type: Ready
  - lastTransitionTime: "2023-09-07T11:56:11Z"
    message: 'failed to pull artifact from ''oci://127.0.0.1:31999/cmwylie19/oci-manifests-hello-zarf'':
      Get "https://127.0.0.1:31999/v2/": dial tcp 127.0.0.1:31999: connect: connection
      refused; Get "http://127.0.0.1:31999/v2/": dial tcp 127.0.0.1:31999: connect:
      connection refused'
    observedGeneration: 1
    reason: OCIArtifactPullFailed
    status: "True"
    type: FetchFailed
  observedGeneration: -1

┌─[cmwylie19@Cases-MacBook-Pro] - [~/zarf] - [2023-09-06 02:48:37]
└─[0] <git:(1974 46635184✱✈) > oras repo tags localhost:5000/cmwylie19/oci-manifests-hello-zarf
latest
latest-zarf-1662488912

@cmwylie19
Copy link
Contributor

@bburky thank you for the clarity!!

@mjnagel
Copy link
Contributor

mjnagel commented Sep 20, 2023

Just to clarify on flux support - OCIRepository cannot be used as a source for a HelmRelease. So I believe to fully support OCI sourced Flux HelmReleases with Zarf we'll need to solve the insecure registry issue either by Flux adding support or by securing the Zarf registry.

@cmwylie19 cmwylie19 removed their assignment Sep 20, 2023
@cmwylie19
Copy link
Contributor

cmwylie19 commented Sep 20, 2023

Unassigned as this will be likely 2 PRs, one for OCIRepo and another that needs more discussion between Zarf team. OCIRepo should be ready soon but is pending one PR.

The current PR that I am working on is addressed by this issue (Which is specific for OCIRepo)

@jeff-mccoy
Copy link
Member

The challenge with tls on the registry is their is no safe k8s-native way to tell the CRIs to use a given CA like you can with admission controllers. The most generic k8-way is to use a privileged daemonset to modify your CRI config (which is as awful as it sounds). Many platforms will modify the CRI out-of-band to accommodate this during cluster init or node image building.

@cmwylie19
Copy link
Contributor

re-assigning myself after chatting with @Racer159 , we will support HelmRepo as long as the type is OCI

---
apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: HelmRepository
metadata:
  name: podinfo
  namespace: default
spec:
  type: "oci"
  interval: 5m0s
  url: oci://ghcr.io/stefanprodan/charts

@cmwylie19 cmwylie19 self-assigned this Sep 21, 2023
@cmwylie19
Copy link
Contributor

  • In terms of the HelmRepo, I ultimately ended up getting blocked by Flux not supporting insecure registry as @bburky eluded to in the initial message. However, the logic mutate the admission controller is there in this draft PR, so once Flux adds support, we can revisit this and merge it into Zarf.

  • The logic for OCIRepo is done and will close this issue and will live on another branch.

@Racer159 Racer159 added this to the (2023.10.24) milestone Oct 5, 2023
@Racer159 Racer159 modified the milestones: (2023.10.24), (2023.11.21) Nov 1, 2023
@Racer159 Racer159 modified the milestones: (2023.11.21), (2023.12.05) Nov 16, 2023
@bburky
Copy link
Member Author

bburky commented Dec 14, 2023

Flux added a .spec.insecure to type: oci HelmRepository resources, the flux issue linked above was closed. 🎉

In addition to mutating the source of an OCI HelmRepository/OCIRepository, zarf could add the insecure flag automatically if a localhost registry is used.

@koolay
Copy link

koolay commented Jan 1, 2024

For now, git server is required in local cluster if deploy using flux. Just require the local registry if oci is supported.

@shanewhite97
Copy link

I see that this has been unblocked now, what is the latest update on this feature?

AustinAbro321 added a commit that referenced this issue May 21, 2024
## Description

This also changes how we the agent gets secrets from the cluster.
Instead of mounting secrets in the deployment and reading from a file
the agent will instead read the secret directly from the cluster. This
is done for simplicity, and easier testing. Additionally, the agent will
need to read from the cluster in #1974 as well so this helps prepare for
that change.

This only changes and tests pods to keep the scope of the PR slim. This
means temporarily the agent will both read from mounted file secrets and
regular kubernetes secrets depending on the hook, but this will be
changed shortly in future PRs.

## Related Issue

Relates to #2512 

## Checklist before merging

- [ ] Test, docs, adr added or updated as needed
- [ ] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/.github/CONTRIBUTING.md#developer-workflow)
followed

---------

Co-authored-by: Lucas Rodriguez <[email protected]>
schristoff pushed a commit to schristoff/zarf that referenced this issue May 22, 2024
## Description

This also changes how we the agent gets secrets from the cluster.
Instead of mounting secrets in the deployment and reading from a file
the agent will instead read the secret directly from the cluster. This
is done for simplicity, and easier testing. Additionally, the agent will
need to read from the cluster in defenseunicorns#1974 as well so this helps prepare for
that change.

This only changes and tests pods to keep the scope of the PR slim. This
means temporarily the agent will both read from mounted file secrets and
regular kubernetes secrets depending on the hook, but this will be
changed shortly in future PRs.

## Related Issue

Relates to defenseunicorns#2512 

## Checklist before merging

- [ ] Test, docs, adr added or updated as needed
- [ ] [Contributor Guide
Steps](https://github.com/defenseunicorns/zarf/blob/main/.github/CONTRIBUTING.md#developer-workflow)
followed

---------

Co-authored-by: Lucas Rodriguez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
Status: In progress
Status: New
Development

Successfully merging a pull request may close this issue.

8 participants