Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Cron job to check for and update images #249

Merged
merged 24 commits into from
Aug 18, 2023
Merged

Cron job to check for and update images #249

merged 24 commits into from
Aug 18, 2023

Conversation

kfox1111
Copy link
Contributor

@kfox1111 kfox1111 commented Apr 29, 2023

This patch adds a cron job to check images for updates and propose a PR with updated versions when one is found and a new version is available.

Running it, it already discovers 3 updates at the time of this writing.

It does have an issue with reformatting comments and removing white space. We may want to just go along with the reformatting the first time.

fixes: #221

@kfox1111
Copy link
Contributor Author

@marcofranssen Its failing shellcheck with:
./.github/scripts/update-tags.sh:38:85: note: Double quote to prevent globbing and word splitting. [SC2086]

But the non double quoted variable substitution is actually critical to its proper functioning.

@kfox1111
Copy link
Contributor Author

kfox1111 commented May 1, 2023

Fixed

@kfox1111 kfox1111 requested a review from edwbuck as a code owner May 17, 2023 16:09
@kfox1111
Copy link
Contributor Author

Ok. This version doesn't touch whitespace and comments. Can see an example of what it produced here:
#290

@faisal-memon faisal-memon added this to the 0.9.0 milestone May 23, 2023
@kfox1111 kfox1111 marked this pull request as draft June 7, 2023 15:00
@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 7, 2023

That introduced a biug... fixing.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 7, 2023

Fixed.

@kfox1111 kfox1111 marked this pull request as ready for review June 7, 2023 15:08
@marcofranssen
Copy link
Contributor

marcofranssen commented Jun 8, 2023

Thinking again on this, isn't it better if the upstream project checks for vulnerabilities in the images? Maybe contribute it there on the images we are using.

In the end it doesn't help us to detect the vulnerability if the upstream doesn't make a release. I think we should just check for new version like dependabot does. Checking for vulnerabilities seems to be something that shouldn't be part of this helm-charts repo, but should be handled in the upstream projects we depend on (hence why I advised to use chainguard images), then we can have a job that checks for new releases.

IMHO we should close this PR as image scanning shouldn't be our responsibility as in the end it doesn't give us value in the sense of having patches for those things we detect, those patches will only arrive if the upstream detects them, hence why I advise to contribute this over there in the upstream projects.

@faisal-memon
Copy link
Contributor

Thinking again on this, isn't it better if the upstream project checks for vulnerabilities in the images? Maybe contribute it there on the images we are using.

In the end it doesn't help us to detect the vulnerability if the upstream doesn't make a release. I think we should just check for new version like dependabot does. Checking for vulnerabilities seems to be something that shouldn't be part of this helm-charts repo, but should be handled in the upstream projects we depend on (hence why I advised to use chainguard images), then we can have a job that checks for new releases.

IMHO we should close this PR as image scanning shouldn't be our responsibility as in the end it doesn't give us value in the sense of having patches for those things we detect.

I agree with the above. In the end if we are just staying up to date on the latest versions then we are just as good. The code to do that should be much simpler. I spent a good chunk of time reviewing but could not wrap my head around the scripts. I wish I had your scripting skills @kfox1111.

With so many other pr's to review i'm not sure how much more time i can spend on this one.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 8, 2023

Thinking again on this, isn't it better if the upstream project checks for vulnerabilities in the images? Maybe contribute it there on the images we are using.

In the end it doesn't help us to detect the vulnerability if the upstream doesn't make a release. I think we should just check for new version like dependabot does. Checking for vulnerabilities seems to be something that shouldn't be part of this helm-charts repo, but should be handled in the upstream projects we depend on (hence why I advised to use chainguard images), then we can have a job that checks for new releases.

IMHO we should close this PR as image scanning shouldn't be our responsibility as in the end it doesn't give us value in the sense of having patches for those things we detect, those patches will only arrive if the upstream detects them, hence why I advise to contribute this over there in the upstream projects.

Chainguard releases daily. So if we do what you suggest, we will be patching our charts, daily. The container scanner part of the pr does just one thing. Squash patching version when no cve's are fixed so we're not having to cut releases constantly to deal with the fact that chainguard releases daily and we can't easily tell which ones are important to update (actual cve's fixed) vs just a new daily image was built. We could consider not doing the trivy stuff if we stopped using chainguard as they are the only images doing such constant releases.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 8, 2023

Are we trying to throw the baby out with the bathwater? When I joined up, I think the only one that knew how the .github tests worked was Marco. It was ok. Things were better with the tests then without them.

Perhaps this is in that category. We are only better off with this in place then without it. That not everyone knows all the gory details of how it works in its entirety, is not great, but we're still better off with it IMO.

Based on how complex it became in bash, its probably better to rewrite this in something more well known such as python. I propose though that we can merge this, as is, and when other, more pressing issues are out of the way, we can do that then. I'm afraid we're letting the perfect stand in the way of the good enough here.

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 8, 2023

Ok. With Edwin's help, we got it converted over to read into a bash array. This fixes the shellcheck issue so we don't have to squash it. Thanks Edwin!

Copy link
Contributor

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

Worked with Kevin to get all BASH passing shellcheck.

Led to actually documenting the different flags in the input YAML as different flags (previously there were not an array, but a string).

Led to changes to pull each array item into a bash array,

Led to the ability to properly quote that array on expansion.

.github/scripts/update-tags.sh Outdated Show resolved Hide resolved
@edwbuck
Copy link
Contributor

edwbuck commented Jun 9, 2023

@faisal-memon @marcofranssen @mrsabath Can we get a second review here? This is nearly ready to merge.

@faisal-memon
Copy link
Contributor

@faisal-memon @marcofranssen @mrsabath Can we get a second review here? This is nearly ready to merge.

@edwbuck Any thoughts on the concerns posted earlier and the alternatives presented?

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@marcofranssen marcofranssen enabled auto-merge (squash) August 16, 2023 16:09
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@faisal-memon faisal-memon modified the milestones: 0.12.0, 0.12.1 Aug 17, 2023
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@marcofranssen marcofranssen merged commit 77fe43f into main Aug 18, 2023
44 checks passed
@marcofranssen marcofranssen deleted the cve-update branch August 18, 2023 07:49
marcofranssen added a commit that referenced this pull request Aug 18, 2023
* 5e2e8a9 Adds AWS KMS KeyManager support (#435)
* 77fe43f Cron job to check for and update images (#249)
* b7e1525 Allow job hooks to be disabled (#434)
* 5e4cf6f Clarify project issues identified with nesting document (#450)
* 7289351 Update spire bits to 1.7.2 (#452)
* dc8a454 Array spacing in values is incorrect in a file. (#451)
* 94326d9 Fixup Helm docs
* ae8941c Support Nested Spire with External Agent (#117)
* f40743d Improve Tornjak documentation (#439)
* 0124f63 Bypass example-test for docs only changes (#449)
* 48a2898 Fix chainguard image references as per issue 442 (#443)
* bd393e9 Bump test chart dependencies (#445)
* a52818a Add a FAQ and switch rare issue from README to it (#437)
* e60f528 option to set KeyManager memory in spire server (#444)
* a167ce6 Bump actions/setup-go from 4.0.1 to 4.1.0
* e774584 Bump test chart dependencies (#426)
* bfec27e Fix jwtIssuer to allow for Uris including scheme (#425)
* 7a6e4f8 Change Tornjak backend default port (#436)
* 1e3039c Bump spire Helm Chart version from 0.11.0 to 0.11.1 (#419)
* d2e1606 issuer naming should respect issuer_name override (#378)
* a2e5c36 Bump test chart dependencies (#416)
* a09e054 support annotations so oidc can be annotated (#391)
* 7d94b10 Update spire to 1.7.1 (#412)
* 9f4d4ac Add aws_pca to the spire-server (#404)
* af13f1f Bump test chart dependencies (#401)
* 9a6768b Add support for disabling container selectors (#399)
* 4687e20 Merge pull request #315 from spiffe/persistence-type
* e16210c Merge branch 'main' into persistence-type
* 624ca9c Remove misadded lockfile (#400)
* 7ce67c6 Bump actions/checkout from 3.5.2 to 3.5.3 (#395)
* b85ba64 Bump helm/kind-action from 1.7.0 to 1.8.0 (#396)
* a6bdb4d Add persistence type flag

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
faisal-memon pushed a commit that referenced this pull request Aug 21, 2023
Please review the below changelog to ensure this matches up with the
semantic version being applied.

> **Note**: **Maintainers** ensure to run following after merging this
PR to trigger the release workflow:
>
> ```shell
> git checkout main
> git pull
> git checkout release
> git pull
> git merge main
> git push
> ```

**Changes in this release**

* 5e2e8a9 Adds AWS KMS KeyManager support (#435)
* 77fe43f Cron job to check for and update images (#249)
* b7e1525 Allow job hooks to be disabled (#434)
* 5e4cf6f Clarify project issues identified with nesting document
(#450)
* 7289351 Update spire bits to 1.7.2 (#452)
* dc8a454 Array spacing in values is incorrect in a file. (#451)
* 94326d9 Fixup Helm docs
* ae8941c Support Nested Spire with External Agent (#117)
* f40743d Improve Tornjak documentation (#439)
* 0124f63 Bypass example-test for docs only changes (#449)
* 48a2898 Fix chainguard image references as per issue 442 (#443)
* bd393e9 Bump test chart dependencies (#445)
* a52818a Add a FAQ and switch rare issue from README to it (#437)
* e60f528 option to set KeyManager memory in spire server (#444)
* a167ce6 Bump actions/setup-go from 4.0.1 to 4.1.0
* e774584 Bump test chart dependencies (#426)
* bfec27e Fix jwtIssuer to allow for Uris including scheme (#425)
* 7a6e4f8 Change Tornjak backend default port (#436)
* 1e3039c Bump spire Helm Chart version from 0.11.0 to 0.11.1 (#419)
* d2e1606 issuer naming should respect issuer_name override (#378)
* a2e5c36 Bump test chart dependencies (#416)
* a09e054 support annotations so oidc can be annotated (#391)
* 7d94b10 Update spire to 1.7.1 (#412)
* 9f4d4ac Add aws_pca to the spire-server (#404)
* af13f1f Bump test chart dependencies (#401)
* 9a6768b Add support for disabling container selectors (#399)
* 4687e20 Merge pull request #315 from spiffe/persistence-type
* e16210c Merge branch 'main' into persistence-type
* 624ca9c Remove misadded lockfile (#400)
* 7ce67c6 Bump actions/checkout from 3.5.2 to 3.5.3 (#395)
* b85ba64 Bump helm/kind-action from 1.7.0 to 1.8.0 (#396)
* a6bdb4d Add persistence type flag

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cron job to look for out of date images
4 participants