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

AKS credentials for ACR #1694

Merged
merged 9 commits into from Feb 12, 2019
Merged

Conversation

@alanjcastonguay
Copy link
Contributor

alanjcastonguay commented Jan 29, 2019

Azure Kubernetes Service (AKS) nodes' kubelet can use the cluster's
service principal to fetch images from Azure Container Registry (ACR).

If azure.json is projected into the Flux pod, consume the service
principal credentials when authenticating to azure container registry.

Note that this method may eventually be deprecated, possibly replaced
by Managed Identity + OAuth, similar to the GCP implementation.
See kubernetes/kubernetes#58034

Prompted by discussion with @squaremo on slack

Azure Kubernetes Service (AKS) nodes' kubelet can use the cluster's
service principal to fetch images from Azure Container Registry (ACR).

If azure.json is projected into the Flux pod, consume the service
principal credentials when authenticating to azure container registry.

Note that this method may eventually be deprecated, possibly replaced
by Managed Identity + OAuth, similar to the GCP implementation.
See kubernetes/kubernetes#58034
registry/azure.go Outdated Show resolved Hide resolved
Copy link
Member

squaremo left a comment

Thanks for this! A point of order: please gofmt files you touch (I noticed this because the added lines have spaces rather than tabs). You can probably set this up as a hook in your favoured editor.

I like the idea that you can just do a hostPath mount to get the credentials, that seems nice and simple. Perhaps consider doing a small explanation in one of the docs files (there's not really a perfect place; site/faq.md would do for now).

@4c74356b41

This comment has been minimized.

Copy link

4c74356b41 commented Feb 7, 2019

I can confirm this is working by adding these changes to the deploy/flux:

        volumeMounts:
        xxx
        - mountPath: /etc/kubernetes/azure.json
          name: acr-credentials
        xxx
      volumes:
      xxx
      - hostPath:
          path: /etc/kubernetes/azure.json
          type: ""
        name: acr-credentials
@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 7, 2019

I can confirm this is working

Splendid, thank you very much for trying it out @4c74356b41

@alanjcastonguay

This comment has been minimized.

Copy link
Contributor Author

alanjcastonguay commented Feb 8, 2019

Awesome!

Let me fix the formatting and docs....

@timwebster9

This comment has been minimized.

Copy link

timwebster9 commented Feb 8, 2019

ACR user here...

Just so I understand, does this PR remove the need to provide imagePullSecrets along with your k8s resources in order to enable the registry scanning to work?

Oddly enough, that method works for me, unlike what people are reporting in #1396. But this method looks nicer :-)

Also, will this help with ACR Helm authentication at all?

@4c74356b41

This comment has been minimized.

Copy link

4c74356b41 commented Feb 8, 2019

In my experiments using imagePullSecrets doesnt work if you grant AKS permissions to ACR, so you have to strip AKS permissions on ACR and after that it will work with imagePullSecrets.

Not sure what do you mean about helm and ACR. helm is a client side tool

@timwebster9

@timwebster9

This comment has been minimized.

Copy link

timwebster9 commented Feb 8, 2019

@4c74356b41

The SP my cluster is using has AcrPull role assignment, and I'm using a different SP (also with AcrPull) for the credentials used in the imagePullSecrets. It seems to work.

Re: the ACR Helm thing - sorry I wasn't clear. ACR can also host Helm charts (i.e. it's a Helm repository), but since it's private it requires authentication.

https://docs.microsoft.com/en-us/azure/container-registry/container-registry-helm-repos

I'm guessing nobody has tried this or looked at it. I tried briefly using the method described here, but it didn't work. It's not an urgent requirement for us at the moment so I haven't made time to investigate further. The authentication method shown in the docs uses an OAuth token which expires after an hour, so not sure this will work.

However when using ACR as a container registry, you can authenticate directly using a service principal (i.e. with docker login), so I was wondering if this would work with the Helm repo authentication somehow.

@4c74356b41

This comment has been minimized.

Copy link

4c74356b41 commented Feb 8, 2019

I'm well aware of that, what I dont understand, with flux all your templates are in the repo already, doesnt matter you have them in a private repo as well? also, if you have acr pull for AKS sp it wont even use imagePullSecrets

@timwebster9

This comment has been minimized.

Copy link

timwebster9 commented Feb 8, 2019

I'm well aware of that, what I dont understand, with flux all your templates are in the repo already, doesnt matter you have them in a private repo as well? also, if you have acr pull for AKS sp it wont even use imagePullSecrets

The reason I needed imagePullSecrets was not for AKS to pull images, but for Flux to poll/scan the registry. Unless I'm mistaken, Flux doesn't use the AKS SP credentials to do this.

So back to the my original question: my guess is that this PR will provide those same credentials (via the host mount) to Flux, so we don't need the imagePullSecrets. Does this sound right?

We currently have some internal Helm charts published to ACR, so I wanted to see if Flux could install those. However we don't need to do this, rather we can just install them from Github instead.

I'm not sure what you mean by your templates are in the repo already. What repo? The Helm charts are in separate repos, not in the flux one. The Flux repo only has the HelmRelease resources that reference these charts.

@4c74356b41

This comment has been minimized.

Copy link

4c74356b41 commented Feb 8, 2019

oh, ok. I didnt knew you can do that. I thought it needed charts in the same repo.

indeed flux doesnt use aks sp credentials (this is why this PR exists), but what I'm telling you is that the solution worked for you because you didnt have ACRpull for the AKS sp, if you did - it wouldnt.

@timwebster9

This comment has been minimized.

Copy link

timwebster9 commented Feb 8, 2019

oh, ok. I didnt knew you can do that. I thought it needed charts in the same repo.

indeed flux doesnt use aks sp credentials (this is why this PR exists), but what I'm telling you is that the solution worked for you because you didnt have ACRpull for the AKS sp, if you did - it wouldnt.

I have ACR pull on both the AKS SP and the one I use for imagePullSecrets and everything works fine. I guess what you're saying is you need to use imagePullSecrets both for polling the registry and pulling images?

Before I added imagePullSecrets to my Flux resources AKS could pull images fine using the usual SP role assignment on ACR. I only had to add it for the registry scan bit.

@alanjcastonguay

This comment has been minimized.

Copy link
Contributor Author

alanjcastonguay commented Feb 8, 2019

@squaremo corrected whitespace (via gofmt) in 3cb2179, un-Exported two methods in df5ca66, added docs to faq.md in cf30277. I think that's all the requested changes.

I plumbed the hostPath volume mount into helm chart (disabled by default, always readOnly) in 0ce0668. That's an opinionated variant of how azure.json aught to be mounted (almost identical to @4c74356b41's, but readOnly). Someone using the helm chart to install flux would have a difficult time customizing the Flux Deployment to add the requisite volume. The helm template doubles as useful docs, too.

@4c74356b41

This comment has been minimized.

Copy link

4c74356b41 commented Feb 8, 2019

@alanjcastonguay cool, didnt think about readOnly, makes a lot of sense.
@timwebster9 ok, i guess you are special :)

@alanjcastonguay alanjcastonguay changed the title WIP: AKS credentials for ACR AKS credentials for ACR Feb 8, 2019
@squaremo

This comment has been minimized.

Copy link
Member

squaremo commented Feb 11, 2019

@alanjcastonguay This is a thorough job, thank you very much!

chart/flux/templates/deployment.yaml Outdated Show resolved Hide resolved
hostPath != mountPath

Co-Authored-By: alanjcastonguay <alanjcastonguay@gmail.com>
Copy link
Member

hiddeco left a comment

@hiddeco hiddeco merged commit 2ca1803 into fluxcd:master Feb 12, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@alanjcastonguay alanjcastonguay deleted the alanjcastonguay:aks-credentials-for-acr branch Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.