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

fix: image registries with no credentials but with a CA #927

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dkoshkin
Copy link
Contributor

What problem does this PR solve?:
Passing imageRegistries with a CA but without credentials is still a valid configuration and should not error.

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

@github-actions github-actions bot added the fix label Sep 27, 2024
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-registries-with-no-credentials-but-with-ca branch from 88f104f to 794b055 Compare September 27, 2024 23:03
@dkoshkin
Copy link
Contributor Author

dkoshkin commented Sep 27, 2024

Actually I don't think this is completely correct yet, I'm not seeing yet where a registry is skipped if it doesn't have credentials when generating the credential provider config files. Fixed in the latest commit.

@dkoshkin dkoshkin force-pushed the dkoshkin/fix-registries-with-no-credentials-but-with-ca branch from 68c0154 to e3b4e0e Compare September 28, 2024 04:31
@dkoshkin dkoshkin force-pushed the dkoshkin/fix-registries-with-no-credentials-but-with-ca branch from e3b4e0e to a7279d4 Compare September 28, 2024 15:15
}
} else {
return false, fmt.Errorf("invalid image registry: %s: %w", config.URL, ErrCredentialsNotFound)
if config.Mirror || config.HasCACert {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not move this logic into requiresStaticCredentials func now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into this some more and I don't really see a way to simplify.
We need to differentiate when requiresStaticCredentials is true and when config.Mirror || config.HasCACert vs nothing is set there.

@dkoshkin dkoshkin force-pushed the dkoshkin/fix-registries-with-no-credentials-but-with-ca branch from a7279d4 to 890d3f9 Compare October 11, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants