Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Implement whitelist for usage of TS from labels #2176

Merged
merged 4 commits into from
Jul 10, 2019

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Jun 21, 2019

We did not take the inheritance of labels from base images into account
while developing the timestamps from image labels feature. As a result,
users started experiencing issues with automated image updates, due to
the (moderately old) timestamp from the base image being picked up and
prioritized over the timestamp from the registry.

This commits adds the ability to decorate ImageRepository structures
before they are returned from cache by GetImageRepositoryMetadata.

The sole decorator for now is TimestampLabelWhitelist, which replaces
the CreatedAt field on the image info with the timestamp specified in
one of the labels if the canonical name of the image matches one of
the glob patterns on the whitelist.

As a result, the labels from images will only be used if explicitly
instructed by the user, dealing with the problem described at the top
of this comment.

@azazel75
Copy link

I've tried this with an image compiled on my own ( azazel/flux:timestampfix-9fc7f65e), on a new flux deployment (after having purged the older one). I'm using the following configuration:

additionalArgs:
  - --manifest-generation=true
git:
  email: [email protected]
  path: little
  url: ssh://[email protected]/endianspa/analytics-releases.git
  user: Endian GitOps
helmOperator:
  create: false
  createCRD: false
resources:
  requests: {}

image:
  repository: docker.io/azazel/flux
  tag: timestampfix-9fc7f65e
  pullPolicy: IfNotPresent
  pullSecret:

which produces this output for the list-images command of one of the workloads:

$ fluxctl --k8s-fwd-ns=flux list-images --workload=default:deployment/emqx -l 0
WORKLOAD                 CONTAINER  IMAGE                                         CREATED
default:deployment/emqx  emqx       registry.gitlab.com/endianspa/analytics/emqx  
                                    |   1                                         20 Jun 19 20:31 UTC
                                    |   1.2                                       20 Jun 19 20:31 UTC
                                    |   abfdfbc5                                  20 Jun 19 20:31 UTC
                                    |   kubernetes-abfdfbc5                       20 Jun 19 20:31 UTC
                                    |   latest                                    20 Jun 19 20:31 UTC
                                    |   96d7b9dd                                  20 Jun 19 15:35 UTC
                                    |   af22350e                                  20 Jun 19 15:35 UTC
                                    |   kubernetes-96d7b9dd                       20 Jun 19 15:35 UTC
                                    |   kubernetes-af22350e                       20 Jun 19 15:35 UTC
                                    |   kubernetes-aae1aa44                       20 Jun 19 15:27 UTC
                                    |   26ae0ca5                                  25 Jan 19 07:27 UTC
                                    |   38f13c1b                                  25 Jan 19 07:27 UTC
                                    |   73ebee55                                  25 Jan 19 07:27 UTC
                                    |   9bd775b6                                  25 Jan 19 07:27 UTC
                                    |   a329537d                                  25 Jan 19 07:27 UTC
                                    |   b4ca8f02                                  25 Jan 19 07:27 UTC
                                    |   df123381                                  25 Jan 19 07:27 UTC
                                    |   kubernetes-26ae0ca5                       25 Jan 19 07:27 UTC
                                    '-> kubernetes-38f13c1b                       25 Jan 19 07:27 UTC
                                        kubernetes-73ebee55                       25 Jan 19 07:27 UTC
                                        kubernetes-9bd775b6                       25 Jan 19 07:27 UTC
                                        kubernetes-a329537d                       25 Jan 19 07:27 UTC
                                        kubernetes-b4ca8f02                       25 Jan 19 07:27 UTC
                                        kubernetes-df123381                       25 Jan 19 07:27 UTC

Here, the images showing date of 25 Jan 19 are those with a build timestamp inherithed by their parent image but built actually this week. Instead, in the newer images I've overwritten the parent timestamp with a new one.

And finally, a release for that same workload gives me this error:

$ fluxctl --k8s-fwd-ns=flux release --workload=default:deployment/emqx --update-all-images
Submitting release ...
Error: verifying changes: failed to verify changes: the image for container "emqx" in resource "default:deployment/emqx" should be "registry.gitlab.com/endianspa/analytics/emqx:kubernetes-abfdfbc5", but is "registry.gitlab.com/endianspa/analytics/emqx:kubernetes-38f13c1b"
Run 'fluxctl release --help' for usage.

If I can help you more with this please ask me here or in chat, thanks for your work

@hiddeco
Copy link
Member Author

hiddeco commented Jun 21, 2019

@azazel75 this is probably due to the fact that your cache is already filled, I need to bump the version somewhere in the source so it creates new entries.

You can confirm this by emptying your memcached instance (deleting the pod should be sufficient).

@azazel75
Copy link

@hiddeco But I have completely erased the flux ns where the service was running, where does it keeps such cache?

@hiddeco
Copy link
Member Author

hiddeco commented Jun 21, 2019

@azazel75 please try it with a fluxctl version build with the source from this PR.

@azazel75
Copy link

@hiddeco you were right, I wasn't using the fluxctl compiled from these sources.. With that, I correctly get:

$ fluxctl --k8s-fwd-ns=flux list-images --workload=default:deployment/emqx -l 0
WORKLOAD                 CONTAINER  IMAGE                                         CREATED
default:deployment/emqx  emqx       registry.gitlab.com/endianspa/analytics/emqx  
                                    |   1                                         20 Jun 19 20:31 UTC
                                    |   1.2                                       20 Jun 19 20:31 UTC
                                    |   abfdfbc5                                  20 Jun 19 20:31 UTC
                                    |   kubernetes-abfdfbc5                       20 Jun 19 20:31 UTC
                                    |   latest                                    20 Jun 19 20:31 UTC
                                    |   96d7b9dd                                  20 Jun 19 15:35 UTC
                                    |   af22350e                                  20 Jun 19 15:35 UTC
                                    |   kubernetes-96d7b9dd                       20 Jun 19 15:35 UTC
                                    |   kubernetes-af22350e                       20 Jun 19 15:35 UTC
                                    |   kubernetes-aae1aa44                       20 Jun 19 15:27 UTC
                                    |   26ae0ca5                                  17 Jun 19 16:56 UTC
                                    |   38f13c1b                                  17 Jun 19 16:56 UTC
                                    |   73ebee55                                  17 Jun 19 16:56 UTC
                                    |   9bd775b6                                  17 Jun 19 16:56 UTC
                                    |   a329537d                                  17 Jun 19 16:56 UTC
                                    |   b4ca8f02                                  17 Jun 19 16:56 UTC
                                    |   df123381                                  17 Jun 19 16:56 UTC
                                    |   kubernetes-26ae0ca5                       17 Jun 19 16:56 UTC
                                    '-> kubernetes-38f13c1b                       17 Jun 19 16:56 UTC
                                        kubernetes-73ebee55                       17 Jun 19 16:56 UTC
                                        kubernetes-9bd775b6                       17 Jun 19 16:56 UTC
                                        kubernetes-a329537d                       17 Jun 19 16:56 UTC
                                        kubernetes-b4ca8f02                       17 Jun 19 16:56 UTC
                                        kubernetes-df123381                       17 Jun 19 16:56 UTC

but a release still gives me an error:

$ fluxctl --k8s-fwd-ns=flux release --workload=default:deployment/emqx --update-all-images
Submitting release ...
Error: verifying changes: failed to verify changes: the image for container "emqx" in resource "default:deployment/emqx" should be "registry.gitlab.com/endianspa/analytics/emqx:kubernetes-abfdfbc5", but is "registry.gitlab.com/endianspa/analytics/emqx:kubernetes-38f13c1b"
Run 'fluxctl release --help' for usage.

@hiddeco hiddeco requested a review from squaremo June 24, 2019 09:29
image/image.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the enhancement/label-ts-whitelist branch 3 times, most recently from 6234b8f to 29b618b Compare July 8, 2019 17:54
@hiddeco hiddeco added this to the 1.13.2 milestone Jul 9, 2019
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

Looks correct to me 👍 A test specifically of the decorated Registry would be good.

Could the glob pattern be tested once for the whole repo, or is it possible to have e.g., --registry-use-labels=fluxcd/flux:master-*?

@hiddeco
Copy link
Member Author

hiddeco commented Jul 9, 2019

Could the glob pattern be tested once for the whole repo, or is it possible to have e.g., --registry-use-labels=fluxcd/flux:master-*?

Not possible, filtering happens on the canonical reference, e.g. index.docker.io/fluxcd/flux, so that it matches the behaviour of --registry-exclude-images.

@hiddeco hiddeco force-pushed the enhancement/label-ts-whitelist branch 5 times, most recently from 3159c11 to 3ba00ae Compare July 10, 2019 08:10
@squaremo
Copy link
Member

Could the glob pattern be tested once for the whole repo, or is it possible to have e.g., --registry-use-labels=fluxcd/flux:master-*?

Not possible, filtering happens on the canonical reference, e.g. index.docker.io/fluxcd/flux, so that it matches the behaviour of --registry-exclude-images.

OK so you can test outside the loop -- all the images in an ImageRepository will have the same canonical image name.

@hiddeco hiddeco force-pushed the enhancement/label-ts-whitelist branch from 1dfe6c6 to 35226d3 Compare July 10, 2019 09:54
hiddeco added 2 commits July 10, 2019 11:55
We did not take the inheritance of labels from base images into account
while developing the timestamps from image labels feature. As a result,
users started experiencing issues with automated image updates, due to
the (moderately old) timestamp from the base image being picked up and
prioritized over the timestamp from the registry.

This commits adds the ability to decorate `ImageRepository` structures
before they are returned from cache by `GetImageRepositoryMetadata`.

The sole decorator for now is `TimestampLabelWhitelist`, which replaces
the `CreatedAt` field on the image info with the timestamp specified in
one of the labels if the canonical name of the image matches one of
the glob patterns on the whitelist.

As a result, the labels from images will only be used if explicitly
instructed by the user, dealing with the problem described at the top
of this comment.
@hiddeco hiddeco force-pushed the enhancement/label-ts-whitelist branch from 35226d3 to 6a3835a Compare July 10, 2019 09:55
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

LGTM again 🍍

@hiddeco hiddeco merged commit 636f849 into master Jul 10, 2019
@hiddeco hiddeco deleted the enhancement/label-ts-whitelist branch July 10, 2019 10:25
squaremo pushed a commit that referenced this pull request Jul 10, 2019
Implement whitelist for usage of TS from labels
hiddeco added a commit that referenced this pull request Nov 8, 2019
In #2176 a whitelist option for the usage of image timestamps was
implemented. This PR did however not remove the `CreatedTS` method,
with as a result that some elements of Flux like `fluxctl` would
still consult the timestamp parsed from labels, rather than relying
on the cache decorated `info.CreatedAt`.

This commit removes the method, and replaces any calls made to it
with `info.CreatedAt`.
hiddeco added a commit that referenced this pull request Nov 8, 2019
In #2176 a whitelist option for the usage of image timestamps from
labels was implemented. This PR did however not remove the
`CreatedTS` method, with as a result that some elements of Flux like
`fluxctl` would still consult the timestamp parsed from labels, rather
than relying on the cache decorated `info.CreatedAt`.

This commit removes the method, and replaces any calls made to it
with `info.CreatedAt`, so that a timestamp from a label is not
mistakenly used while the image has not been whitelisted.
hiddeco added a commit that referenced this pull request Nov 19, 2019
hiddeco added a commit that referenced this pull request Nov 19, 2019
In #2176 a whitelist option for the usage of image timestamps from
labels was implemented. This PR did however not remove the
`CreatedTS` method, with as a result that some elements of Flux like
`fluxctl` would still consult the timestamp parsed from labels, rather
than relying on the cache decorated `info.CreatedAt`.

This commit removes the method, and replaces any calls made to it
with `info.CreatedAt`, so that a timestamp from a label is not
mistakenly used while the image has not been whitelisted.
hiddeco added a commit that referenced this pull request Nov 19, 2019
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.

3 participants