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

feat: kustomize image digests #11153

Merged
merged 28 commits into from Sep 10, 2021
Merged

feat: kustomize image digests #11153

merged 28 commits into from Sep 10, 2021

Conversation

@thepwagner
Copy link
Contributor

@thepwagner thepwagner commented Aug 7, 2021

Changes:

Modifies the kustomize manager to:

  • Support updating digests when specified in the digest, newTag or newName field.
  • Skip the pathological case where newTag and digest are both specified - as it's not clear what the user's expectation is here.
  • Drop support for newTag: sha256:... which does not seem to be valid kustomize.

This doesn't support injecting digests where they don't already exist, so it is not fully #8089 .

Context:

I'm using this test repository: https://github.com/thepwagner/renovate-kustomize-image-digests

Kustomize images are complicated, the updated README contains several example possibilities. Renovate's implementation was limited to updating the newTag field, but version and digest information might also be included in newName or digest fields. This PR expands the kustomize manager to integrate those fields.

Edge cases exist when the user has provided additional information that kustomize won't use. Those are skipped, with a warning logged:

- name: quay.io/argoproj/argocd
  newTag: v2.0.5-moot # does not matter because digest is provided
  digest: sha256:b53a2f76f949849cbe7e0b1e343a2076c7f205cfcfe8116e461e6861369fa81f

There's a bias towards formats that provide the version+digest within a single YAML value:

- name: quay.io/argoproj/argocd
  newTag: v2.0.5@sha256:b53a2f76f949849cbe7e0b1e343a2076c7f205cfcfe8116e461e6861369fa81f
- name: quay.io/argoproj/argocd
  newName: custom/argo@v2.0.5@sha256:b53a2f76f949849cbe7e0b1e343a2076c7f205cfcfe8116e461e6861369fa81f
- name: quay.io/argoproj/argocd
  newName: custom/argo
  newTag: v2.0.5@sha256:b53a2f76f949849cbe7e0b1e343a2076c7f205cfcfe8116e461e6861369fa81f

So this is not #8089 , but it is enough for my use case and should be backwards compatible.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository
@thepwagner thepwagner changed the title feature: kustomize image digests feat: kustomize image digests Aug 7, 2021
@rarkins
Copy link
Collaborator

@rarkins rarkins commented Aug 8, 2021

If I understand correctly,

  • before, Renovate would increase the tag, which not only wouldn't match the digest but also would be ignored too?
  • with this PR, Renovate will increase the digest and never update the tag?

What I'm not sure is: is it the current digest for the existing tag (in which case the result is fully correct) or is it selecting the digest of the latest tag?

I'd really like to update both, but ok with a mid step if it improves the situation. I'm a big fan of having both the informative tag plus synced normative digest.

Also, would it be reasonable to support the combined tag@digest approach only and document that, if users want both updated in lockstep?

@thepwagner
Copy link
Contributor Author

@thepwagner thepwagner commented Aug 8, 2021

@rarkins Yep, your assumptions are correct. When digest: is provided, newTag can be treated like a comment.

What I'm not sure is: is it the current digest for the existing tag (in which case the result is fully correct) or is it selecting the digest of the latest tag?

With this proposed implementation+example, I think the latter: since the currentValue parsed from the dependency is undefined any image with a digest: set will be treated as :latest.
That could be improved if newTag was parsed as currentValue (like it is when digest: isn't set), but until the fields are maintained lockstep all options produce ambiguous output.

I'd really like to update both, but ok with a mid step if it improves the situation

This is definitely a mid-step; I was working on a solution that maintained both, but it awkwardly parsed the images: block twice (to deserialize as YAML, and to break into big replaceString tokens) and used a autoReplaceStringTemplate so it might reorder keys - I figured this would be easier to land.

Also, would it be reasonable to support the combined tag@digest approach only and document that, if users want both updated in lockstep?

I think that's the most reasonable middle ground, it's the compromise I was willing to make! I can take this PR in that direction.

Thanks for the feedback!

@rarkins
Copy link
Collaborator

@rarkins rarkins commented Aug 9, 2021

With this proposed implementation+example, I think the latter: since the currentValue parsed from the dependency is undefined any image with a digest: set will be treated as :latest.

I would prefer to avoid this behavior, because I think it's downside is far worse than what we have today. Today, we can mislead people that they updated from version X to version X+1 but actually nothing changed. In other words, they stay on a version which hopefully works.

With this PR's change, we could be updating them to a completely non-working version for them, because sometimes latest tags can be semi-canary builds. And to make it worse: it would look like a simple digest update (low risk) when it could be potentially be breaking.

Have I understood correctly and do you agree?

That could be improved if newTag was parsed as currentValue (like it is when digest: isn't set), but until the fields are maintained lockstep all options produce ambiguous output.

I think this is definitely best. I would only like to do a digest update if it matches newTag. It's a pity though we have no way to "only update digests if newTag is defined and skip any newTag updates", leaving only:

  • Digest updates matching existing newTag
  • newTag updates only when digest is missing

What if we intentionally skipped any extracted dependency which has both newTag and digest in separate fields and logged a warning that it's unsupported? i.e. support only one field at a time.

This is definitely a mid-step; I was working on a solution that maintained both, but it awkwardly parsed the images: block twice (to deserialize as YAML, and to break into big replaceString tokens) and used a autoReplaceStringTemplate so it might reorder keys - I figured this would be easier to land.

Considering how small these YAML files should be, we shouldn't be concerned with "optimization" of two parse-throughs. But if you think the logic itself is too convoluted then it's another matter.

I think that's the most reasonable middle ground, it's the compromise I was willing to make! I can take this PR in that direction.

Would that align with what I suggested earlier about intentionally skipping extracted deps with both fields defined?

(And let's open a new issue for the advanced case of both fields present, so that one day someone may work out how to elegantly achieve it)

  • Would it be appropriate to info/warning a message if digest: is provided? or should it continue to be ignored by Renovate?

I think warning is best.

I think that's the only one we need.

Thanks!

@viceice
Copy link
Collaborator

@viceice viceice commented Aug 9, 2021

#8089 (comment)

I've already written some weird samples where it's not clear what should happen.

@thepwagner thepwagner marked this pull request as draft Aug 10, 2021
@thepwagner
Copy link
Contributor Author

@thepwagner thepwagner commented Aug 11, 2021

This PR is still in progress - I'll edit the OP and mark RFR when it is.

But an update: I've ported most of _viceice's cases to https://github.com/thepwagner/renovate-kustomize-image-digests.
The most interesting result was that newTag: sha256:abcabcabc doesn't seem to actually work with kustomize-4.2.0. I didn't dig into earlier versions of kustomize that might have supported that syntax, I just updated to return SkipReason.InvalidValue.
I wanted to flag that diff ASAP as it contradicts #7987 - I'm missing some context that might make newTag: sha256:... valid.

@viceice
Copy link
Collaborator

@viceice viceice commented Aug 12, 2021

@thepwagner did you saw my previous work here: #8110 ?

@thepwagner
Copy link
Contributor Author

@thepwagner thepwagner commented Aug 12, 2021

@thepwagner did you saw my previous work here: #8110 ?

Yep! Sorry, I should have mentioned that. I liked the pattern of a regex to match multiple YAML keys. I figured that pattern would need to include newName as well, and didn't like the prospect of abc|acb|bac|bca|cab|cba (since YAML will allow the values in any order).

That's what sent me look at pre-splitting the images: block as a string. Key order wouldn't matter, as long as there was a name: in there somewhere the replaceString contain the fully indented .images[] entry.

I abandoned the latter approach as autoReplaceStringTemplate felt necessary to support injecting newDigest if it didn't already exist (which would afford pinDigests: true and fully close #8089). Doing this while maintaining the user's original key order did not seem like fun. I may have been to focused on maintaining key order. While it reads as weird to me, the following should be supported, right?

  - newTag: 3.12.2
    name: alpine
    digest: sha256:25f5332d060da2c7ea2c8a85d2eac623bd0b5f97d508b165f846c7d172897438
  - digest: sha256:e1488cb900233d035575f0a7787448cb1fa93bed0ccc0d4efc1963d7d72a8f17
    newTag: 1.32.1
    name: busybox:1.30.0
  - newName: amd64/busybox:1.30.1
    name: busybox
    digest: sha256:e1488cb900233d035575f0a7787448cb1fa93bed0ccc0d4efc1963d7d72a8f17

So when I realized newTag: tag@digest worked for my use case, I gave up on multiple keys and started on this priority-driven approach: digest > newTag > newName. It seems correct, but admittedly punishes users that provide too much information that kustomize will ignore. Per the discussion with _rarkins, I think nudging those users towards newTag: tag@digest is a better course.

@thepwagner thepwagner marked this pull request as ready for review Aug 12, 2021
@rarkins rarkins requested a review from viceice Aug 20, 2021
lib/manager/kustomize/extract.ts Show resolved Hide resolved
lib/manager/kustomize/extract.ts Show resolved Hide resolved
lib/manager/kustomize/extract.ts Outdated Show resolved Hide resolved
lib/manager/kustomize/extract.ts Outdated Show resolved Hide resolved
@viceice viceice requested a review from HonkingGoose Aug 24, 2021
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

I don't understand enough about these changes to confirm/deny that the docs changes are good.

I added a review comment with some things I noticed.

lib/manager/kustomize/readme.md Outdated Show resolved Hide resolved
thepwagner and others added 2 commits Aug 26, 2021
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@thepwagner thepwagner requested a review from viceice Aug 31, 2021
@thepwagner
Copy link
Contributor Author

@thepwagner thepwagner commented Sep 5, 2021

@viceice I've addressed comments, please take a look.

lib/manager/kustomize/extract.spec.ts Outdated Show resolved Hide resolved
lib/manager/kustomize/extract.spec.ts Show resolved Hide resolved
lib/manager/kustomize/extract.spec.ts Show resolved Hide resolved
lib/manager/kustomize/extract.ts Show resolved Hide resolved
Copy link
Collaborator

@viceice viceice left a comment

ups, some small fix

@HonkingGoose

This comment has been hidden.

@rarkins rarkins requested review from viceice and HonkingGoose Sep 10, 2021
lib/manager/kustomize/readme.md Outdated Show resolved Hide resolved
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
@viceice viceice enabled auto-merge (squash) Sep 10, 2021
@viceice viceice merged commit dc15dfd into renovatebot:main Sep 10, 2021
7 checks passed
@renovate-release
Copy link
Collaborator

@renovate-release renovate-release commented Sep 10, 2021

🎉 This PR is included in version 27.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@thepwagner thepwagner deleted the renovate-8089 branch Sep 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants