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

Make k8s keychain vs default keychain configurable #130

Merged
merged 9 commits into from
Feb 14, 2022

Conversation

@marcofranssen marcofranssen requested a review from a team as a code owner January 26, 2022 15:27
@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch 3 times, most recently from 0bab1c8 to c658be7 Compare January 26, 2022 15:37
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #130 (419b873) into main (9a1ac90) will decrease coverage by 0.38%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
- Coverage   77.92%   77.54%   -0.39%     
==========================================
  Files          15       15              
  Lines         607      610       +3     
==========================================
  Hits          473      473              
- Misses         95       97       +2     
- Partials       39       40       +1     
Flag Coverage Δ
unittests 77.54% <40.00%> (-0.39%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/oci/auth.go 40.00% <25.00%> (-7.06%) ⬇️
cmd/slsa-provenance/cli/container.go 77.77% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a1ac90...419b873. Read the comment docs.

@Brend-Smits
Copy link
Member

Brend-Smits commented Jan 26, 2022

I have been testing this in a private registry, but unfortunately, I don't think this is working yet.
Error is now: UNAUTHORIZED: authentication required, it is different than before.

@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch from e4f49b9 to 89e7674 Compare February 8, 2022 08:41
@Brend-Smits
Copy link
Member

Brend-Smits commented Feb 9, 2022

Unfortunately, I think this is not yet resolved, internal testing shows that it is still an issue. This can either be in my workflow (am I missing some step), or in slsa action.
Thoughts?
https://github.com/philips-internal/internal-test-slsa-provenance-action

On the other hand, docker-ci-scripts is working. This makes me think the workflow is wrong.

@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch from 89e7674 to 10711af Compare February 9, 2022 13:23
@marcofranssen
Copy link
Member Author

The rootcause of our problem is that we are running the slsa-provenance binary inside a Docker container in the action. Therefore the binary doesn't know about the docker login that happens outside of the container in a workflow.

We have 2 option here.

  • Docker credentials passed to the action as inputs
  • Only install the binary

@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch 7 times, most recently from e6280d8 to 05ae2f5 Compare February 10, 2022 14:53
@marcofranssen
Copy link
Member Author

Please note, once this PR gets merged, our draft releases will fail. Due to different asset urls for draft releases. Not sure how to compute those.

@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch 9 times, most recently from 9645bcd to 3cee7d3 Compare February 11, 2022 14:40
marcofranssen and others added 7 commits February 14, 2022 10:02
This to resolve workflows that require docker login

Signed-off-by: Marco Franssen <[email protected]>
Co-authored-by: Brend Smits <[email protected]>
Signed-off-by: Marco Franssen <[email protected]>
@marcofranssen marcofranssen force-pushed the configurable-auth-from-cli branch 2 times, most recently from c9965bc to 20993bb Compare February 14, 2022 09:06
This is something we only require for testing our releases

Signed-off-by: Marco Franssen <[email protected]>
@marcofranssen
Copy link
Member Author

@marcofranssen marcofranssen merged commit de12b70 into main Feb 14, 2022
@marcofranssen marcofranssen deleted the configurable-auth-from-cli branch February 14, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants