feat: ECR credential integration into Finch by kiryl1 · Pull Request #462 · runfinch/finch

kiryl1 · 2023-06-28T21:09:38Z

Issue #, if available: 116

Description of changes:
Integrating ECR credential helper into Finch.

To Install the Credential Helper

The ECR credential helper can be setup with minimal configuration by setting the creds_helpers option in finch.yaml with the value - ecr-login.
Once the option has been set the credential helper will be installed on either finch vm init or finch vm start. The binary will be downloaded on the host machine and a config.json will be created and populated in the ~/.finch/ folder if it doesn't already exist. If it already exists, the value of credsStore will be overwritten to ecr-login .

How to stop using the credential helper with Finch

config.json needs to be either deleted, or the credsStore parameter needs to have the value ecr-login removed. Additionally the creds_helper option in the finch.yaml needs to be removed.
To fully remove the credential helper from the host machine, this can be done by deleting it from the
~/.finch/cred-helpers folder.

The credential helper will support credentials from both the aws credentials file and those that are stored as environment variables.

Testing done:
Added Unit Tests
Added Integration Test

[ x] I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

pendo324

Left some comments. You'll also need to sign your commit with the DCO by using the -s flag when you run git commit (more info)

vsiravar

Left a few minor comments. Overall looks good. Suggest adding a note in the README similar to additional_directories explaining what the user experience is going to be like.

Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324

Looks good, just a few things. Also think we have to update the GitHub workflow files and Makefile to actually add the Registry option, right?

…ting config.json in integration test, changed binares struct to credhelperbin Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324

Changes look good to me. Lets take it out of draft and work on configuring the integration tests to run with the correct registry by adding the secret value and then using it in ci.yaml?

vsiravar · 2023-07-11T00:31:34Z

We use conventional commits for creating finch release based on semantic versioning. We should probably prefix the PR title as feat: .

Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324

Still need the changes to actually use the remote registry unless I'm missing something, otherwise LGTM

vsiravar · 2023-07-11T22:08:50Z

Still need the changes to actually use the remote registry unless I'm missing something, otherwise LGTM

Yeah, I think it may be worth exporting the aws credentials like how we export COSIGN_PASSWORD if users wish to use env vs file. Might also simply the e2e test which will rely on https://github.com/aws-actions/configure-aws-credentials which exports the credentials as env variables.

Signed-off-by: kiryl1 <kirylkul@gmail.com>

vsiravar

LGTM, we can merge once the linter check passes. https://github.com/runfinch/finch/actions/runs/5526370034/jobs/10097740941?pr=462#step:4:18795

Signed-off-by: kiryl1 <kirylkul@gmail.com>

ginglis13

LGTM

ginglis13 · 2023-07-14T21:10:43Z

Wanted to callout an unexpected behavior which we have options to resolve. Consider this non-blocking for the PR but follow up work. I follow these steps after successfully configured the credential helper and verifying that push and pull work with my ECR repo:

finch vm stop
finch vm remove
remove credsStore: ecr-login from ~/.finch/finch.yaml
finch vm init,
pull -> Success!

I no longer have finch configured with a credsStore in ~/.finch/finch.yaml, but I am still able to auth, push and pull with finch. This is because ~/.finch/config.json still contains the "credsStore":"ecr-login" entry. What's expected? Should this be removed automatically? Or should users be expected to edit this files themselves? (I personally vote the former)

I can turn this comment into an issue if you folks agree @kiryl1 @pendo324 @vsiravar

pendo324 · 2023-07-14T22:18:58Z

What's expected? Should this be removed automatically? Or should users be expected to edit this files themselves? (I personally vote the former)

I can turn this comment into an issue if you folks agree @kiryl1 @pendo324 @vsiravar

+1 for turning this into an issue. I think the expected behavior to remove the feature is probably to just edit the same config file again, not also have to edit config.json. This is a pretty big PR so following it up in another more focussed PR makes sense to me. We should just make sure to document the current behavior properly @kiryl1

vsiravar · 2023-07-14T22:39:51Z

Should this be removed automatically?

+1 for this, it'll end up confusing the user if the config is missing in ~/.finch/finch.yaml but the finch still tries to use the credentials helper.

…cs to README, changed data type of credential helper to a slice instead of just string Signed-off-by: kiryl1 <kirylkul@gmail.com>

Signed-off-by: kiryl1 <kirylkul@gmail.com>

ginglis13 · 2023-07-17T19:29:10Z

Are e2e tests using the credential helper? For example in build 1018 CI / e2e-tests (self-hosted, macos, amd64, 13, test):

# Set output variables
has_creds=false

And

Finch Container Development E2E Tests log in a container registry when the private registry is running and an image is built should push an image after successfully logging in the registry with a correct credential

...

  WARNING: Your password will be stored unencrypted in /Users/ec2-user/.finch/config.json.
  Configure a credential helper to remove this warning. See
  https://docs.docker.com/engine/reference/commandline/login/#credentials-store

Also, looks like each e2e step is running twice? e.g.. there are two occurrences of CI / e2e-tests (self-hosted, macos, amd64, 13, test) . (e: seeing Run echo "Skipping CI for docs & contrib files" in one of each of the duplicates, likely from 949a184)

kiryl1 · 2023-07-17T20:02:10Z

Are e2e tests using the credential helper? For example in build 1018 CI / e2e-tests (self-hosted, macos, amd64, 13, test):
# Set output variables
has_creds=false
And
Finch Container Development E2E Tests log in a container registry when the private registry is running and an image is built should push an image after successfully logging in the registry with a correct credential

...

  WARNING: Your password will be stored unencrypted in /Users/ec2-user/.finch/config.json.
  Configure a credential helper to remove this warning. See
  https://docs.docker.com/engine/reference/commandline/login/#credentials-store
Also, looks like each e2e step is running twice? e.g.. there are two occurrences of CI / e2e-tests (self-hosted, macos, amd64, 13, test) . (e: seeing Run echo "Skipping CI for docs & contrib files" in one of each of the duplicates, likely from 949a184)

No the existing e2e tests are not using the Credential Helper, Since the existing e2e tests are using finch login the tests would fail if the credential helper was enabled.

Im also not exactly sure why it appears to run in duplicates.

🤖 I have created a release *beep* *boop* --- ## [0.7.0](v0.6.2...v0.7.0) (2023-07-18) ### Features * ECR credential integration into Finch ([#462](#462)) ([d3514b3](d3514b3)) ### Bug Fixes * Add cleanup script to Makefile ([#444](#444)) ([da91f87](da91f87)) ### Build System or External Dependencies * **deps:** Bump github.com/docker/docker from 24.0.2+incompatible to 24.0.4+incompatible ([#469](#469)) ([ad37f4f](ad37f4f)) * **deps:** Bump github.com/docker/docker from 24.0.2+incompatible to 24.0.4+incompatible ([#481](#481)) ([15d2a4b](15d2a4b)) * **deps:** Bump github.com/onsi/ginkgo/v2 from 2.10.0 to 2.11.0 ([#456](#456)) ([f7e0916](f7e0916)) * **deps:** Bump github.com/onsi/ginkgo/v2 from 2.9.7 to 2.10.0 ([#449](#449)) ([a415e3e](a415e3e)) * **deps:** Bump github.com/onsi/gomega from 1.27.7 to 1.27.8 ([#448](#448)) ([96fc8d0](96fc8d0)) * **deps:** Bump github.com/runfinch/common-tests from 0.7.0 to 0.7.1 ([#477](#477)) ([54c03bb](54c03bb)) * **deps:** Bump github.com/shirou/gopsutil/v3 from 3.23.5 to 3.23.6 ([#464](#464)) ([43a6720](43a6720)) * **deps:** Bump github.com/sirupsen/logrus from 1.9.2 to 1.9.3 ([#446](#446)) ([1823677](1823677)) * **deps:** Bump golang.org/x/crypto from 0.10.0 to 0.11.0 ([#465](#465)) ([dc5a3e7](dc5a3e7)) * **deps:** Bump golang.org/x/crypto from 0.9.0 to 0.10.0 ([#451](#451)) ([fef6e77](fef6e77)) * **deps:** Bump golang.org/x/tools from 0.10.0 to 0.11.0 ([#466](#466)) ([a8b32f9](a8b32f9)) * **deps:** Bump golang.org/x/tools from 0.9.3 to 0.10.0 ([#455](#455)) ([e321f1d](e321f1d)) * **deps:** Bump k8s.io/apimachinery from 0.27.2 to 0.27.3 ([#454](#454)) ([d6746a4](d6746a4)) * **deps:** Bump lima version ([#476](#476)) ([7b330d3](7b330d3)) * **deps:** Bump submodules ([#482](#482)) ([92f2494](92f2494)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

pendo324 reviewed Jun 28, 2023

View reviewed changes

Comment thread pkg/dependency/credhelper/cred_helper_binary.go Outdated

pendo324 suggested changes Jun 28, 2023

View reviewed changes

Comment thread pkg/dependency/credhelper/cred_helper_binary_test.go Outdated

vsiravar suggested changes Jul 2, 2023

View reviewed changes

Comment thread pkg/config/config.go Outdated

Comment thread pkg/config/nerdctl_config_applier.go Outdated

Comment thread pkg/dependency/credhelper/cred_helper_binary.go Outdated

Comment thread pkg/dependency/credhelper/cred_helper_binary.go Outdated

added credential helper integration

0d287cc

Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 force-pushed the dev branch from e9a9036 to 0d287cc Compare July 3, 2023 23:28

kiryl1 added 2 commits July 7, 2023 16:44

added integration test, removed extra line from bash rc

1653c5f

Signed-off-by: kiryl1 <kirylkul@gmail.com>

removed focus from test

e76bd81

Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324 reviewed Jul 10, 2023

View reviewed changes

Comment thread e2e/vm/cred_helper_test.go Outdated

Comment thread pkg/dependency/credhelper/cred_helper_binary.go Outdated

added registry flag to makefile, added note in readme, removed popula…

82fcbe6

…ting config.json in integration test, changed binares struct to credhelperbin Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 marked this pull request as ready for review July 10, 2023 23:30

pendo324 previously approved these changes Jul 10, 2023

View reviewed changes

vsiravar reviewed Jul 11, 2023

View reviewed changes

Comment thread e2e/vm/cred_helper_test.go Outdated

vsiravar reviewed Jul 11, 2023

View reviewed changes

Comment thread e2e/vm/cred_helper_test.go Outdated

vsiravar reviewed Jul 11, 2023

View reviewed changes

Comment thread e2e/vm/cred_helper_test.go

added go mod and go sum

ec78355

Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 dismissed pendo324’s stale review via ec78355 July 11, 2023 17:45

kiryl1 changed the title ~~ECR credential integration into Finch~~ feat: ECR credential integration into Finch Jul 11, 2023

removed nested describe node, linter changes

bfd4294

Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324 reviewed Jul 11, 2023

View reviewed changes

ginglis13 reviewed Jul 11, 2023

View reviewed changes

added support for credentials as env variables, style changes

3706612

Signed-off-by: kiryl1 <kirylkul@gmail.com>

pendo324 previously approved these changes Jul 12, 2023

View reviewed changes

vsiravar self-requested a review July 12, 2023 16:15

vsiravar previously approved these changes Jul 12, 2023

View reviewed changes

linter change

c5443ff

Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 dismissed stale reviews from vsiravar and pendo324 via c5443ff July 12, 2023 16:29

ginglis13 previously approved these changes Jul 12, 2023

View reviewed changes

ningziwen reviewed Jul 14, 2023

View reviewed changes

Comment thread cmd/finch/nerdctl.go

Merge branch 'main' into dev

f994666

kiryl1 dismissed stale reviews from ginglis13, vsiravar, and pendo324 via f994666 July 14, 2023 21:55

kiryl1 added 4 commits July 16, 2023 17:49

changed option credsHelper to creds_helpers for consistency, added do…

acc65fa

…cs to README, changed data type of credential helper to a slice instead of just string Signed-off-by: kiryl1 <kirylkul@gmail.com>

Merge branch 'dev' of https://github.com/kiryl1/finch into dev

61bd4fa

removed redundant nil check, removed unneeded docs in README

031c50a

Signed-off-by: kiryl1 <kirylkul@gmail.com>

linter fix

de1786b

Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 requested a review from ningziwen July 17, 2023 16:51

ningziwen reviewed Jul 17, 2023

View reviewed changes

Comment thread README.md Outdated

ningziwen reviewed Jul 17, 2023

View reviewed changes

Comment thread README.md

updated readme

949a184

Signed-off-by: kiryl1 <kirylkul@gmail.com>

kiryl1 requested a review from ningziwen July 17, 2023 18:20

pendo324 approved these changes Jul 17, 2023

View reviewed changes

ningziwen approved these changes Jul 17, 2023

View reviewed changes

ginglis13 approved these changes Jul 17, 2023

View reviewed changes

kiryl1 merged commit d3514b3 into runfinch:main Jul 17, 2023

github-actions Bot mentioned this pull request Jul 17, 2023

chore(main): release 0.7.0 #445

Closed

ginglis13 mentioned this pull request Jul 17, 2023

Still authenticated after removing creds_helpers from Finch VM configuration #480

Open

github-actions Bot mentioned this pull request Jul 18, 2023

chore(main): release 0.7.0 #483

Merged

github-actions Bot mentioned this pull request Jul 18, 2023

chore(main): release 0.8.0 #484

Closed

github-actions Bot mentioned this pull request Dec 9, 2023

chore(main): release 1.0.1 #728

Merged

ollypom mentioned this pull request Feb 8, 2024

Support BYO credential helpers #800

Open

Conversation

kiryl1 commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Acceptance

Uh oh!

Uh oh!

pendo324 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vsiravar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pendo324 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pendo324 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vsiravar commented Jul 11, 2023

Uh oh!

pendo324 left a comment

Choose a reason for hiding this comment

Uh oh!

vsiravar commented Jul 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vsiravar left a comment

Choose a reason for hiding this comment

Uh oh!

ginglis13 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ginglis13 commented Jul 14, 2023

Uh oh!

pendo324 commented Jul 14, 2023

Uh oh!

vsiravar commented Jul 14, 2023

Uh oh!

Uh oh!

Uh oh!

ginglis13 commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiryl1 commented Jul 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kiryl1 commented Jun 28, 2023 •

edited

Loading

ginglis13 commented Jul 17, 2023 •

edited

Loading