-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: Enable registries with self-signed certificates #9863
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
base: main
Are you sure you want to change the base?
fix: Enable registries with self-signed certificates #9863
Conversation
This change enables Skaffold to push to container image registries that use self-signed or other untrusted certificates, when Skaffold runs on macOS or Windows. It also removes the need for the `SSL_CERT_FILE` environment variable workaround on Linux. Prior to this fix, Skaffold would fail to retrieve the digest from the registry after the image was built, even if the registry was configured as an insecure registry in Skaffold configuration: ``` getting image: Get "https://localhost:8443/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority; GET http://localhost:8443/v2/: unexpected status code 400 Bad Request: Client sent an HTTP request to an HTTPS server. ``` On Linux environments only, a possible workaround was to set the `SSL_CERT_FILE` environment variable. However, this workaround cannot be used on macOS or Windows. This change updates `getRemoteIndex()` and `getRemoteImage()` in `pkg/skaffold/docker/remote.go`, adding the `InsecureSkipVerify` TLS config field to the transport if the registry from the image name matches one of the insecure registries configured in Skaffold. Fixes: GoogleContainerTools#3039 GoogleContainerTools#3116 Related: google/go-containerregistry#1559
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @halvards, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances Skaffold's compatibility with various container image registries by enabling seamless interaction with those using self-signed or untrusted certificates. It resolves a critical TLS verification issue that previously hindered operations on macOS and Windows, while also simplifying the setup for Linux users by removing a manual workaround. The overall impact is improved reliability and ease of use when working with diverse registry environments.
Highlights
- Self-signed Certificate Support: Skaffold can now push to container image registries that use self-signed or other untrusted certificates on macOS and Windows, resolving previous connectivity issues.
- Linux Workaround Removal: The need for the
SSL_CERT_FILE
environment variable workaround on Linux environments has been eliminated, streamlining the configuration process. - TLS Verification Fix: This change directly addresses the
tls: failed to verify certificate
error encountered when interacting with insecure registries, ensuring successful image digest retrieval after builds. - Implementation Details: The fix involves updating
getRemoteIndex()
andgetRemoteImage()
inpkg/skaffold/docker/remote.go
to conditionally apply theInsecureSkipVerify
TLS configuration when interacting with configured insecure registries.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly enables Skaffold to work with insecure container registries by adding InsecureSkipVerify
to the TLS configuration for remote image operations. The change is well-implemented, notably cloning the default transport to avoid global side effects. I have one suggestion to refactor some duplicated code into a helper function, which will improve the overall maintainability of the code.
options := []remote.Option{ | ||
remote.WithAuthFromKeychain(primaryKeychain), | ||
} | ||
if IsInsecure(ref, cfg.GetInsecureRegistries()) { | ||
options = append(options, insecureTransportOption()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for creating remote options is duplicated in getRemoteImage
on lines 128-130. To improve maintainability and avoid duplication, you could extract this logic into a shared helper function.
For example, you could create a function like this:
func getBaseRemoteOptions(ref name.Reference, cfg Config) []remote.Option {
options := []remote.Option{
remote.WithAuthFromKeychain(primaryKeychain),
}
if IsInsecure(ref, cfg.GetInsecureRegistries()) {
options = append(options, insecureTransportOption())
}
return options
}
Then both getRemoteImage
and getRemoteIndex
can call this helper to get the base options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount of duplication reduced by this change would be minimal, and it would make the options creation logic harder to follow, so I would suggest not taking this suggested change.
This change enables Skaffold to push to container image registries that use self-signed or other untrusted certificates, when Skaffold runs on macOS or Windows. It also removes the need for the
SSL_CERT_FILE
environment variable workaround on Linux.Prior to this fix, Skaffold would fail to retrieve the digest from the registry after the image was built, even if the registry was configured as an insecure registry in Skaffold configuration:
On Linux environments only, a possible workaround was to set the
SSL_CERT_FILE
environment variable. However, this workaround cannot be used on macOS or Windows.This change updates
getRemoteIndex()
andgetRemoteImage()
inpkg/skaffold/docker/remote.go
, adding theInsecureSkipVerify
TLS config field to the transport if the registry from the image name matches one of the insecure registries configured in Skaffold.Fixes: #3039 #3116
Related: google/go-containerregistry#1559