Don't try to resolve pinned sha's to versions when updating docker images

[deivid-rodriguez]

This may seem too aggressive, and I'm probably missing something, and I haven't yet run full tests BUT, I think this is incorrect and not necessary.

When a base image is pinned to a specific sha, Dependabot does a very heavy operation of requesting the list of tags from the registry, and then asking for the sha of each of them until one tag that matches the current sha is found.

However, this is not necessary, because Dependabot will end up updating to the latest sha anyways, since it defaults to always keeping the current style/precision.

Also, I believe this is incorrect, because docker tags are mutable, so the registry will only return the latest sha associated to each tag. So this loop is likely to run forever to end up not finding any matches if the current sha present in the Dockerfile has been overwritten.

Finally, this should be a big performance boost because this operation is very very slow.

Fixes #2997.
Fixes #4419.

Just opening a WIP PR to check CI for now.

[Nishnha]

However, this is not necessary, because Dependabot will end up updating to the latest sha anyways, since it defaults to always keeping the current style/precision.

I think the reason we fetch versions and try to match them against the SHA is to determine the version precision. Otherwise, we can't know what version the image needs to be upgraded to

Not sure if I'm misunderstanding something here though

[Nishnha]

Oh I think I was missing something.... Docker doesn't support any other versioning strategies other than default https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#versioning-strategy

So we really might just be updating all Docker SHAs to latest. The feature seems to be redundant!

Can you confirm that matches what your reasoning for removing this feature? The only exception I can think of is security updates, but GitHub doesn't publish any Docker advisories 🤔

[deivid-rodriguez]

Let me try explain what happens when Dependabot tries to update a Dockerfile using a sha256 reference before and after this PR through an example.

Let's take for example the declaration FROM ghcr.io/dependabot/dependabot-updater@sha256: 654abc3436d0c99896df14d3d6bb450774fd5639a97e8a2bf88bc813151ef7bd.

This is what happens without this PR:

• Dependabot tries to find a tag with sha matching sha256: 654abc3436d0c99896df14d3d6bb450774fd5639a97e8a2bf88bc813151ef7bd.
• This sometimes works and something don't because while today a specific SHA may be associated with a tag, docker tags are usually moved and previous SHAs pointed to the tag get "orphaned". Also, when this works, it's very slow because it needs to make an API request for every tag checked, and some images have a lot of tags.
• Assuming the previous point works, Dependabot finds a tag associated to the SHA. In this case, it's v2.0.20221207081814.
• Now it uses the that tag to find another tag to update. In this case, since the tag uses patch level precision, it will find an up to date patch level tag. In this case, it would figure out it needs to update to the latest tag: v2.0.20221213172033.
• Finally it finds the SHA of the updated tag: sha256:e95a9f539fa58bc6a2e57fe6333cdcb08d4da9774bddc7d7b66e07bd748a89dd.
• The final result is Dependabot would create a PR with title "Update ghcr.io/dependabot/dependabot-updater from v2.0.20221207081814 to v2.0.20221213172033" and that updates the Dockerfile from FROM ghcr.io/dependabot/dependabot-updater@sha256: 654abc3436d0c99896df14d3d6bb450774fd5639a97e8a2bf88bc813151ef7bd to FROM ghcr.io/dependabot/dependabot-updater@sha256:e95a9f539fa58bc6a2e57fe6333cdcb08d4da9774bddc7d7b66e07bd748a89dd.

This is the logic with this PR:

• Dependabot sees that the Dockerfile is locked to a sha256 reference.
• Dependabot finds the sha256 reference of the latest tag.
• Dependabot creates a PR with title "Update ghcr.io/dependabot/dependabot-updater from 654abc3436d0c99896df14d3d6bb450774fd5639a97e8a2bf88bc813151ef7bd to e95a9f539fa58bc6a2e57fe6333cdcb08d4da9774bddc7d7b66e07bd748a89dd" and the same diff as with the previous logic.

[deivid-rodriguez]

So we really might just be updating all Docker SHAs to latest. The feature seems to be redundant!

Can you confirm that matches what your reasoning for removing this feature? The only exception I can think of is security updates, but GitHub doesn't publish any Docker advisories 🤔

Yes, that's exactly right by the way! Indeed, if we wanted a "lowest fixed version" logic for Security Updates, we wouldn't have that, but that does not seem a concern now, and we don't have that logic in place at the moment anyways.

[Nishnha]

Thanks for walking me through the logic on this ❤️

Awesome change. I had one minor nit but this LGTM

[stevehipwell]

@deivid-rodriguez do you have a status on this?

[deivid-rodriguez]

Hei! There's no ETA but the PR is already approved and just needs a rebase and a final look to get shipped. It should not take too long.

[deivid-rodriguez]

This should be now ready for another review unless CI proves me wrong. It's a big PR, but mostly deletes a lot of code!

[Nishnha]

I notice this can match tags with different prefixes. It seems like these are both valid?

• sha265:18305429afa14ea462f810146ba44d4363ae76e4c8dfc38288cf73aa07485005
• :sha265:18305429afa14ea462f810146ba44d4363ae76e4c8dfc38288cf73aa07485005

But most tags will be in the form of FROM ghcr.io/dependabot/dependabot-updater@sha256: 654abc3436d0c99896df14d3d6bb450774fd5639a97e8a2bf88bc813151ef7bd, right?

Is there a reason you support the other prefix with a leading :? Do they also appear in Dockerfiles?

[deivid-rodriguez]

@Nishnha Regarding your comment about leading :, is it because the regex is now

IMAGE_SPEC = %r{^(#{REGISTRY}/)?#{IMAGE}#{TAG}?(?:@sha256:#{DIGEST})?#{NAME}?}x

If that's it, the ?: part before sha256 is meant to mean "match this group, but don't include it as a named match". So in other words, that leading : is not supposed to be matched, it's just a special character when preceded by ?.

See this: https://rubular.com/r/FXdZeMjcFcMH03.

[Nishnha]

@Nishnha Regarding your comment about leading :, is it because the regex is now

IMAGE_SPEC = %r{^(#{REGISTRY}/)?#{IMAGE}#{TAG}?(?:@sha256:#{DIGEST})?#{NAME}?}x

If that's it, the ?: part before sha256 is meant to mean "match this group, but don't include it as a named match". So in other words, that leading : is not supposed to be matched, it's just a special character when preceded by ?.

See this: https://rubular.com/r/FXdZeMjcFcMH03.

TIL! 😮

The only other question I have is why remove the private registry tests in 666c100 ?

[deivid-rodriguez]

Sorry @Nishnha, not sure what happened there, I was sure I had answered the question about the removed tests 😅.

I removed those because the logic they were testing has been removed.

Previously, if we found an image reference with only a SHA-version (no numeric version), we would talk to the registry to try figure out whether there's a numeric tag associated to that SHA. Those tests were testing that the registry requests to do this happen correctly.

With the new approach, we don't mess with trying to find a numeric version for a given SHA, and rely exclusively to the information present in the Dockerfile itself. So we don't talk to the registry anymore when parsing Dockerfile dependencies.

This is why those tests were removed, I hope it makes sense!