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

authn.kubernetes.Resolve now behaves exactly like Kubernetes #1349

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

dprotaso
Copy link
Contributor

Prior we weren't matching host names and partial paths properly

This was breaking Gitlab and Azure tag to digest resolution in Knative (knative/serving#12761)

@dprotaso
Copy link
Contributor Author

I think I need to pull the first commit into a separate PR since I need to bump the ggcr version in the kubernetes/go.mod

Otherwise the kubernetes pkg won't pull the right ggcr min version containing the new json marshalling helpers.

@dprotaso
Copy link
Contributor Author

Step 1: #1350

Will rebase this PR with go.mod updates

pkg/authn/kubernetes/keychain.go Outdated Show resolved Hide resolved
@imjasonh
Copy link
Collaborator

Should we promote this extra fuzzy URL matching to match k8s' behavior up to pkg/authn and use it for all Docker config handling?

Thanks so much for doing this investigation and work @dprotaso, you're a rockstar 👨‍🎤 ❤️

@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 14, 2022

Should we promote this extra fuzzy URL matching to match k8s' behavior up to pkg/authn and use it for all Docker config handling?

I was wondering if the fuzzy matching is only a k8s behaviour or is that a docker config convention? I would do it in a separate PR if it's not K8s convention.

Prior we weren't matching host names and partial paths properly

This was breaking Gitlab and Azure tag to digest resolution in Knative
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2022

Codecov Report

Merging #1349 (8c468cd) into main (f1b7291) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1349   +/-   ##
=======================================
  Coverage   74.19%   74.19%           
=======================================
  Files         113      113           
  Lines        8439     8439           
=======================================
  Hits         6261     6261           
  Misses       1574     1574           
  Partials      604      604           

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 f1b7291...8c468cd. Read the comment docs.

@dprotaso
Copy link
Contributor Author

Also note - unit/build tests don't work in nested modules since ./... only applies to all packages in the current module

- run: go test -coverprofile=coverage.txt -covermode=atomic -race ./...

Realized this here (gopher slack): https://gophers.slack.com/archives/C9BMAAFFB/p1649939462805589

@dprotaso
Copy link
Contributor Author

Unit test failure seems unrelated

--- FAIL: TestWriteIndex_Progress (2.28s)
    progress_test.go:193: saw progress revert: was high of 2659099, saw 2524410

https://github.com/google/go-containerregistry/runs/6024983982?check_suite_focus=true#step:4:715

@dprotaso
Copy link
Contributor Author

presubmit looks legit - looking into that

@dprotaso
Copy link
Contributor Author

dprotaso commented Apr 14, 2022

After this I need to bump the k8schain/go.mod with the pkg/authn/kubernetes sha (in a separate PR)

@imjasonh
Copy link
Collaborator

I was wondering if the fuzzy matching is only a k8s behaviour or is that a docker config convention? I would do it in a separate PR if it's not K8s convention.

Sounds like it's just K8s' version, but it seems useful to apply Postel's law here and everywhere. Maybe this means we can drop (some of?) our dep from pkg/authn -> docker/cli, and things like this:

cfg, err = cf.GetAuthConfig(key)

(not for this PR, just something to consider in the future)

@dprotaso
Copy link
Contributor Author

(not for this PR, just something to consider in the future)

👍

@imjasonh imjasonh merged commit 892d7a8 into google:main Apr 14, 2022
@dprotaso dprotaso deleted the lookup-fix branch April 14, 2022 14:34
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.

None yet

3 participants