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

Merge existing image credentials into a fresh set #1702

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
3 participants
@hiddeco
Copy link
Member

hiddeco commented Feb 1, 2019

Fixes #1485

With the previous merging strategy the set of credentials which got
mutated could be assigned to other images, resulting in overwrites
where we would (and did) not expect them.

This new approach is the most direct fix to work around mutating the
credentials of other images. It creates a fresh set of (empty)
credentials and merges the others into it before assigning it to the
image.

Details

@hiddeco hiddeco requested review from squaremo and 2opremio Feb 1, 2019

Show resolved Hide resolved cluster/kubernetes/images.go Outdated

@hiddeco hiddeco added the blocked label Feb 1, 2019

@hiddeco

This comment has been minimized.

Copy link
Member Author

hiddeco commented Feb 1, 2019

After deploying this to one of our clusters it seems to create a new issue where Flux is unable to pull images tied to HelmRelease manifests because it fails to extract credentials from this resource kind.

ts=2019-02-01T14:30:10.59171206Z caller=images.go:17 component=sync-loop msg="polling images"
ts=2019-02-01T14:32:24.797392145Z caller=warming.go:192 component=warmer canonical_name=eu.gcr.io/<image> auth={map[]} err="requesting tags: denied: Failed to read tags for host 'eu.gcr.io', repository '/v2/<image>/tags/list'"
ts=2019-02-01T14:32:26.38038089Z caller=warming.go:192 component=warmer canonical_name=eu.gcr.io/<image> auth={map[]} err="requesting tags: denied: Failed to read tags for host 'eu.gcr.io', repository '/v2/<image>/tags/list'"
ts=2019-02-01T14:33:31.017196418Z caller=warming.go:192 component=warmer canonical_name=eu.gcr.io/<image> auth={map[]} err="requesting tags: denied: Failed to read tags for host 'eu.gcr.io', repository '/v2/<image>/tags/list'"
@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 1, 2019

Flux is unable to pull images tied to HelmRelease manifests because it fails to extract credentials from this resource kind.

fluxd doesn't try to extract secrets from HelmRelease resources (they don't have any); it relies on scanning images from resources created as part of the release. So, what is happening is fluxd doesn't pick up credentials for those.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 4, 2019

So, what is happening is fluxd doesn't pick up credentials for those.

No, I'm being silly -- what's happening is that the credentials extracted from HelmRelease (i.e., none) pre-empting the credentials from the other workloads.

This is a bug, but it's not clear what the correct solution is. Going back to merging credentials will solve it, but is not obviously correct either; excluding HelmRelease objects when scanning for images will give the right result, but seems like an arbitrary special case.

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 4, 2019

Logically, I'm not sure how this changes the situation: before, credentials were merged meaning an entry for a particular registry can be overwritten (last entry wins); now, whichever workload is scanned first will supply the credentials, and no others can (first entry wins). Either way, if you have two distinct credentials against workloads using the same image, one of them will non-deterministically "win".

@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 4, 2019

Logically, I'm not sure how this changes the situation: before, credentials were merged meaning an entry for a particular registry can be overwritten (last entry wins); now, whichever workload is scanned first will supply the credentials, and no others can (first entry wins). Either way, if you have two distinct credentials against workloads using the same image, one of them will non-deterministically "win".

.. all of which doesn't go anywhere in countering your actual experience of it not working!

We found the problem with the existing code: https://github.com/weaveworks/flux/blob/master/cluster/kubernetes/images.go#L155 mutates a set of credentials which can be assigned to other images. The most direct fix would be to create a fresh set of credentials for each image, and merge the others into it; i.e.,

existingCreds, ok := allImageCreds[imageID]
if ok {
    mergedCreds := registry.NoCredentials()
    mergedCreds.Merge(existingCreds)
    mergedCreds.Merge(creds)
    allImageCreds[imageID] = mergedCreds
} else {
    allImageCreds[imageID] = creds
}
Merge existing image credentials into a fresh set
With the previous merging strategy the set of credentials which got
mutated could be assigned to other images, resulting in overwrites
where we would (and did) not expect them.

This new approach is the most direct fix to work around mutating the
credentials of other images. It creates a fresh set of (empty)
credentials and merges the others into it before assigning it to the
image.

@hiddeco hiddeco force-pushed the 1485-image-pull-secrets branch from 52505f2 to 284b2a1 Feb 4, 2019

@hiddeco

This comment has been minimized.

Copy link
Member Author

hiddeco commented Feb 4, 2019

Force-pushed the fix mentioned above to solve the issue without leaving a trail of my original approach, which skipped the merge if there were already credentials present for an image.

@hiddeco hiddeco changed the title Do not merge credentials for already found images Merge existing image credentials into a fresh set Feb 4, 2019

@hiddeco hiddeco removed the blocked label Feb 4, 2019

@squaremo
Copy link
Member

squaremo left a comment

Looks right to me 🍎

@hiddeco hiddeco merged commit 8bcf060 into master Feb 4, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@hiddeco hiddeco deleted the 1485-image-pull-secrets branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.