search

openshift-helm-charts / development

0 stars 17 forks source link

Incorrect tarball content is not detected #342

Open mgoerens opened 3 months ago

mgoerens commented 3 months ago

Issue occurs for the chart named utcc in the version 1.4.0. Two PRs are corresponding to this chart:

Reason why an update was possible in this particular case is tracked in another issue.

In the first PR, the user submitted a tgz that, when unpacked, results in this directory structure:

uctc-1.4.0/
└── uctc-common
    ├── Chart.yaml
    ├── README.md
    ├── templates
    │   ├── clusterrolebinding.yaml
    │   ├── clusterrole.yaml
    │   ├── daemonset.yaml
    │   ├── deployment.yaml
    │   ├── _helpers.tpl
    │   ├── ingress.yaml
    │   ├── NOTES.txt
    │   ├── serviceaccount.yaml
    │   ├── service.yaml
    │   └── tests
    │       └── uct-cntlr-dns-lookup-test.yaml
    ├── values.schema.json
    └── values.yaml

Note that the tgz itself is correctly named (uctc-1.4.0.tgz, corresponding to our constraint -.tgz). However it contains a directory utcc-common. That causes issue at the very end of the pipeline, when we prepare the index update: we untar the archive, edit the annotations in Chart.yaml, and recreate the archive. Here we expect the directory in the archive to be named after the chart (in this particular case it should be utcc).

In the second PR, the content of the archive is:

uctc-1.4.0/
└── uctc
    ├── Chart.yaml
    ├── README.md
    ├── templates
    │   ├── clusterrolebinding.yaml
    │   ├── clusterrole.yaml
    │   ├── daemonset.yaml
    │   ├── deployment.yaml
    │   ├── _helpers.tpl
    │   ├── ingress.yaml
    │   ├── NOTES.txt
    │   ├── serviceaccount.yaml
    │   ├── service.yaml
    │   └── tests
    │       └── uct-cntlr-dns-lookup-test.yaml
    ├── values.schema.json
    └── values.yaml

which is correct and doesn't cause an issue.

komish commented 3 months ago

@mgoerens Excellent sleuthing here, thank you.

Given the second PR merged and indexed correctly, I think we can leave this partner as is, so long as they are happy. There should be no real side-effects given the original chart was never indexed (and likely have caused issues for people attempting to use it because of the naming mismatch (a problem with which we are too familiar).

I'm rather happy that you found the "chartversion already exists in index" check is working as expected, and that the only reason this was possible is because of the indexing failure of the first PR.

On the other hand, finding a fix for this poses a few challenges as I see it:

(1)

If a partner submits a tarball with invalid contents (e.g. the wrong directory at the root, as we see here), I believe that we should fail them and force them to repackage, which should allow for all later logic that relies on a specific directory structure to work without modification. This would need to happen before we give the :heavy_check_mark: and indicate the submitted asset is good to go. At least, in tarball cases.

This would probably protect our assumptions (that uctc-1.4.0.tar.gz contains uctc/ and nothing else) and allow our logic to continue to function.

(2)

In theory, we're checking the indexes for pre-existing conflicting chart versions on new submissions, which forces a partner to continue to iterate their chart versions. However, if there's any issue with our logic, updating an already-merged chart becomes possible, even if just temporarily.

We can't effectively block certification on a per-partner, per-version basis in the event there's an indexing conflict, so it becomes a race for one of us to fix the indexing for a merged chart to close the "possible to update your chart" window.

Would it make sense to instead look at the modification type for the submitted assets using the GitHub API, and use that to help us inform if the operation is updating existing things, or adding net-new things?

In practice, this would look like this.

This item may be overkill, so I'm curious what you think about this.

As an aside, I strongly dislike modifying the Chart.yaml at all, personally. It's convenient to not have to include certain metadata when you send your chart through the process, but it feels like everyone is on the same page if the partner is required to put the right metadata into place, vs. being surprised when there are new keys in their metadata. Even if it is just the Kube to OCP version mapping (for example).

mgoerens commented 3 months ago

we can leave this partner as is, so long as they are happy

Agreed.

I'm rather happy that you found the "chartversion already exists in index" check is working as expected and that the only reason this was possible is because of the indexing failure of the first PR.

All I can say is that this particular issue didn't occur because of the "chartversion already exists in index" check failing.. Maybe next time :)

(1)

If a partner submits a tarball with invalid contents (e.g. the wrong directory at the root, as we see here), I believe that we should fail them and force them to repackage, which should allow for all later logic that relies on a specific directory structure to work without modification. This would need to happen before we give the ✔️ and indicate the submitted asset is good to go. At least, in tarball cases.

This would probably protect our assumptions (that uctc-1.4.0.tar.gz contains uctc/ and nothing else) and allow our logic to continue to function.

Agreed. Basically, the first PR should have failed before it was merged.

Question is where to implement such a check ? There are maybe places already were we untar the archive and could implement that check, I haven't look for them just yet.

In the near future this could be done as part of the "pre-check" job.

(2) [...] Would it make sense to instead look at the modification type for the submitted assets using the GitHub API, and use that to help us inform if the operation is updating existing things, or adding net-new things?

In practice, this would look like this.

I think that's another clever way to look at it. I'm wondering if this is meant to replace the "chartversion already exists in index" check ?

Ideally of course the index is inline with what's in the repo. So checking for the state of the index and checking for the state of the repo should yield similar results.

BTW I wonder if we could run a check that index and repo are actually coherent. Are there other charts that were merged but never ended up in the index ? Or are there charts in the index that are somehow not in the repo ?

As an aside, I strongly dislike modifying the Chart.yaml at all, personally. It's convenient to not have to include certain metadata when you send your chart through the process, but it feels like everyone is on the same page if the partner is required to put the right metadata into place, vs. being surprised when there are new keys in their metadata. Even if it is just the Kube to OCP version mapping (for example).

I imagine it was decided that it's easier for the partner if they can just submit a Chart as-is vs having to modify it specially for the Red Hat certification. It should be clearly communicated though.

komish commented 1 month ago

Wrapping this conversation up -

BTW I wonder if we could run a check that index and repo are actually coherent. Are there other charts that were merged but never ended up in the index ? Or are there charts in the index that are somehow not in the repo ?

We could definitely do this. I'm sure we'd find inconsistencies (as we did when we did the 1.7.0-related audits).

In the near future this could be done as part of the "pre-check" job.

I think this makes sense. Basically, early on do:

#pseudocode
if submission.is_chart_tar():
    if not submission.tarball().is_expected_structure():
        raise(SubmissionError("Tarball structure does match expectations"))