Skip to content

Update dependencies #1805

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

Closed
wants to merge 4 commits into from
Closed

Update dependencies #1805

wants to merge 4 commits into from

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented May 23, 2025

Updates:

  • helm.sh/helm/v3 v3.18.0
  • sigs.k8s.io/controller-runtime v0.21.0
  • cloud.google.com/go/storage v1.54.0
  • github.com/sigstore/cosign/v2 v2.5.0

Part of: fluxcd/flux2#5363

@stefanprodan stefanprodan requested a review from matheuscscp May 23, 2025 12:26
@stefanprodan stefanprodan added the dependencies Pull requests that update a dependency label May 23, 2025
@stefanprodan stefanprodan force-pushed the helm-v3.18.0 branch 2 times, most recently from 0576287 to 41abf96 Compare May 23, 2025 13:40
@stefanprodan
Copy link
Member Author

Lots of issues with Helm 3.18 due to the upgrade of ORAS from v1 to v2. Login to container registries is broken:

failed to construct Helm client: failed to decode config file at /tmp/credentials3039474744: invalid config format: EOF

We'll postpone to upgrade to Helm 3.18 for Flux 2.7

@TerryHowe
Copy link

Does the error indicate that since the login failed, there is nothing in the creds file (or it wasn't created) causing a EOF.

@matheuscscp matheuscscp reopened this May 23, 2025
@matheuscscp
Copy link
Member

Reopening just to test some fixes in CI

@matheuscscp matheuscscp force-pushed the helm-v3.18.0 branch 3 times, most recently from 3d5b499 to e81e6cd Compare May 23, 2025 20:44
@TerryHowe
Copy link

That change related to the floats may not be required because there was a change similar to that that was reverted for 3.18.1

@TerryHowe
Copy link

Ideally, you wouldn't need to make these changes with 3.18.1, but I don't yet understand all the problems

@@ -112,7 +113,7 @@ func TestLocalBuilder_Build(t *testing.T) {
name: "default values",
reference: LocalReference{Path: "../testdata/charts/helmchart"},
wantValues: chartutil.Values{
"replicaCount": float64(1),
"replicaCount": json.Number("1"),

Choose a reason for hiding this comment

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

I'd be concerned that 3.18.1 will be broken with this change.

@matheuscscp
Copy link
Member

Tests are passing now, but I still prefer not to ship 3.18.0

@matheuscscp
Copy link
Member

I'll keep this branch around though, it's documenting a few things we need to do:

  • Remove old assertion library from tests
  • Remove a flaky .Update() in the ObjectLevelWorkloadIdentity feature gate test outside an Eventually block
  • Remove the oras v1 workaround passing an empty tmp file for not storing docker creds at ~/.docker/config.json (the big culprit of tests not passing)

@TerryHowe
Copy link

I'd still like to understand the file issue

@matheuscscp
Copy link
Member

matheuscscp commented May 24, 2025

I'd still like to understand the file issue

It's actually simple: oras v1 was writing to ~/.docker/config.json. In order to avoid that in a Flux controller, which cannot keep such state across reconciliations of different objects, we were creating an empty tmp file and passing its name down to oras v1 so it could have somewhere else to write that was not ~/.docker/config.json. Then we delete the file at the end of the reconciliation of the object.

The first thing that oras v2 does with this empty file is try to parse a JSON from it, which obviously fails with EOF as the file has 0 bytes of content.

The fix for our CI to pass here is just not creating and passing down this empty file to helm/oras anymore.

We were never reusing this file anywhere else in the reconciliation code except for deleting it at the end, so I think in our case just removing the code that creates it, passes it down to helm/oras and deletes it at the end is enough.

@stefanprodan
Copy link
Member Author

We were never reusing this file anywhere else in the reconciliation code except for deleting it at the end, so I think in our case just removing the code that creates it, passes it down to helm/oras and deletes it at the end is enough.

I hope ORAS v2 can take the auth data directly without having to write files on disk. If that's not the case, then we should look into replacing ORAS with fluxcd/pkg/oci and fluxcd/pkg/auth.

@matheuscscp
Copy link
Member

We were never reusing this file anywhere else in the reconciliation code except for deleting it at the end, so I think in our case just removing the code that creates it, passes it down to helm/oras and deletes it at the end is enough.

I hope ORAS v2 can take the auth data directly without having to write files on disk. If that's not the case, then we should look into replacing ORAS with fluxcd/pkg/oci and fluxcd/pkg/auth.

I ran this test step by step on the debugger to investigate:

{
name: "HTTP with basic auth secret",
want: sreconcile.ResultSuccess,
insecure: true,
registryOpts: registryOptions{
withBasicAuth: true,
},
secretOpts: secretOptions{
username: testRegistryUsername,
password: testRegistryPassword,
},
secret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secretref",
},
Type: corev1.SecretTypeDockerConfigJson,
Data: map[string][]byte{},
},
assertConditions: []metav1.Condition{
*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: pulled 'helmchart' chart with version '0.1.0'"),
},
},

Looks like helm and oras v2 are keeping the username/password entirely in memory with this:

https://github.com/helm/helm/blob/cc58e3f5a3aa615c6a86275e6f4444b5fdd3cc4e/pkg/registry/client.go#L269

https://github.com/oras-project/oras-go/blob/05a2b09cbf2eab1df691411884dc4df741ec56ab/registry/remote/auth/client.go#L68-L82

So I don't think oras v2 is writing this to disk, @TerryHowe can probably confirm?

Maybe we should use fluxcd/pkg/oci to have the same OCI code everywhere and not three different codes like right now, but it doesn't look like writing the creds to disk is going to be an issue. Btw this helm OCI code already uses fluxcd/pkg/auth

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants