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: Allow ACR authentication using Azure CLI #586

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

xescab
Copy link

@xescab xescab commented Jul 4, 2023

Install azure-cli in Docker image in order to use the az acr login command.

Can be used with Azure Managed Identities with the following script:

---
apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-image-updater-config
  namespace: argocd
data:
  log.level: debug
  registries.conf: |
    registries:
    - name: acrexample
      api_url: https://acrexample.azurecr.io/
      prefix: acrexample.azurecr.io
      ping: yes
      insecure: no
      credentials: ext:/app/scripts/acr-login.sh
      credsexpire: 10h
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-image-updater-config-acr
  namespace: argocd
data:
  acr-login.sh: |
    #!/bin/sh
    LOGIN=$(az login --identity)
    REGISTRY="acrexample"
    TOKEN=$(az acr login --name $REGISTRY --expose-token --output tsv --query accessToken)
    echo "00000000-0000-0000-0000-000000000000:$TOKEN"

Closes #550 and #473

@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.98%. Comparing base (2bf4b0a) to head (cd76581).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #586   +/-   ##
=======================================
  Coverage   67.98%   67.98%           
=======================================
  Files          31       31           
  Lines        3124     3124           
=======================================
  Hits         2124     2124           
  Misses        856      856           
  Partials      144      144           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jannfis
Copy link
Contributor

jannfis commented Jul 6, 2023

Awesome! Do you mind adding some pieces of examples to the docs as well?

@xescab
Copy link
Author

xescab commented Jul 17, 2023

Awesome! Do you mind adding some pieces of examples to the docs as well?

Sure, I've added a new section to the https://github.com/argoproj-labs/argocd-image-updater/blob/master/docs/configuration/registries.md documentation. Do you think is it clear enough, or should add all the patches required? Where?

Thanks!

@xescab
Copy link
Author

xescab commented Jul 17, 2023

By the way, how can I fix the GitHub Actions / Spell checking errors? Basically I see I need to add some words to a dictionary, but I don't know how. Thanks!

@jwhy89
Copy link

jwhy89 commented Jul 28, 2023

Waiting on this also 🙏🏼

@jwhy89
Copy link

jwhy89 commented Jul 31, 2023

By the way, how can I fix the GitHub Actions / Spell checking errors? Basically I see I need to add some words to a dictionary, but I don't know how. Thanks!

I think you can add words to the .github/actions/spelling/allow.txt file.

jwhy89

This comment was marked as outdated.

@jwhy89
Copy link

jwhy89 commented Jul 31, 2023

@xescab xescab#1

@jannfis
Copy link
Contributor

jannfis commented Jul 31, 2023

Sorry for the late reply. The suggestion from @jwhy89 is correct. The spell checking is non-critical though, but the DCO actually is. Can you please ensure that your commits are all signed off to satisfy the DCO requirement? Thank you!

@Retoxx-dev
Copy link

Hi, where are we with this? It'd be great to have that ACR feature.

@xescab
Copy link
Author

xescab commented Sep 6, 2023

@xescab xescab#1

Thanks @jwhy89! I've integrated your changes into my last commit, as I had to re-commit the other changes to sign the commits for the DCO requirement.

@xescab
Copy link
Author

xescab commented Sep 6, 2023

Sorry for the late reply. The suggestion from @jwhy89 is correct. The spell checking is non-critical though, but the DCO actually is. Can you please ensure that your commits are all signed off to satisfy the DCO requirement? Thank you!

Hi, I'm back from vacation. Thanks for looking into this. Just signed the previous commits. Please let me know if something else is missing.

@jwhy89
Copy link

jwhy89 commented Sep 7, 2023

Sorry for the late reply. The suggestion from @jwhy89 is correct. The spell checking is non-critical though, but the DCO actually is. Can you please ensure that your commits are all signed off to satisfy the DCO requirement? Thank you!

Hi, I'm back from vacation. Thanks for looking into this. Just signed the previous commits. Please let me know if something else is missing.

Your DCO is still failing and there's a test that needs to be fixed. You can merge my branch into yours. Then probably fix the DCO again. xescab#2

@xescab xescab force-pushed the feat/add-azure-cli branch 2 times, most recently from 872b984 to 15de0f1 Compare September 8, 2023 15:41
@xescab
Copy link
Author

xescab commented Sep 8, 2023

All checks pass now! :)

Can someone review it again and hopefully approve it? Thanks!

@xescab
Copy link
Author

xescab commented Sep 13, 2023

@jannfis, can you take a look?

@xescab xescab requested a review from jwhy89 September 20, 2023 08:19
@xescab
Copy link
Author

xescab commented Sep 20, 2023

What can I do to get this merged? :)

@xescab
Copy link
Author

xescab commented Nov 20, 2023

@jannfis, can you take a look?

Any news?

@etiennetremel
Copy link

@jannfis seems like you are the only maintainer, any chance you could review these changes?

etiennetremel added a commit to etiennetremel/argocd-image-updater that referenced this pull request Feb 14, 2024
@etiennetremel
Copy link

Playing around with it again, that might not be a good idea to include the entire Azure CLI in the Dockerfile.

Warning

The size of the end image with Azure CLI is really big: 2.62Gb.
Original image is 281Mb...

There should be a better way to retrieve the ACR token using the rest API Azure provide, but I couldn't figure out how to adjust the steps defined here.

In the meantime, I have merged this PR into the main branch in my own repo and published a publicly available Docker container on ghcr.io with some instructions to get it working with Azure Workload Identity:

https://github.com/etiennetremel/argocd-image-updater

Install azure-cli in Docker image in order to use the `az acr login` command.

Can be used with Azure Managed Identities with the following script:

```yaml
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-image-updater-config
  namespace: argocd
data:
  log.level: debug
  registries.conf: |
    registries:
    - name: acrexample
      api_url: https://acrexample.azurecr.io/
      prefix: acrexample.azurecr.io
      ping: yes
      insecure: no
      credentials: ext:/app/scripts/acr-login.sh
      credsexpire: 10h
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: argocd-image-updater-config-acr
  namespace: argocd
data:
  acr-login.sh: |
    #!/bin/sh
    LOGIN=$(az login --identity)
    REGISTRY="acrexample"
    TOKEN=$(az acr login --name $REGISTRY --expose-token --output tsv --query accessToken)
    echo "00000000-0000-0000-0000-000000000000:$TOKEN"
```

Closes argoproj-labs#550 and argoproj-labs#473

Signed-off-by: Francesc Arbona <[email protected]>
@chengfang
Copy link
Collaborator

What's the size of the end image with only the necessary azure-cli components?

Looking at azure-cli install guide: Azure/azure-cli#19591, there are some tricks to reduce the size such as deleting pycache afterwards, and selecting only the latest version of azure mgmt packages. Would these help in our case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot pull images from Azure Container Registry
7 participants