-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Push to OCI insecure registry #10408
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,5 +54,9 @@ func newPushCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { | |
}, | ||
} | ||
|
||
f := cmd.Flags() | ||
f.BoolVar(&client.InsecureSkipTLSverify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart upload") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re naming convo in https://github.com/helm/helm/pull/10408/files#r802048813 This is a follow up: #10408 (review) and #10408 (comment)
Sounds like this is already inconsistent 🤔 I wonder if there is a good reason for that or if it was just an oversight. Can we pick a consistent flag? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @scottrigby Would it make sense to incorporate |
||
f.BoolVar(&client.PlainHTTP, "plain-http", false, "use plain http and not https to connect oci registry") | ||
|
||
return cmd | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,7 @@ type Pull struct { | |
VerifyLater bool | ||
UntarDir string | ||
DestDir string | ||
PlainHTTP bool | ||
cfg *Configuration | ||
} | ||
|
||
|
@@ -76,6 +77,12 @@ func NewPullWithOpts(opts ...PullOpt) *Pull { | |
func (p *Pull) Run(chartRef string) (string, error) { | ||
var out strings.Builder | ||
|
||
if p.InsecureSkipTLSverify || p.PlainHTTP { | ||
if err := p.cfg.RegistryClient.WithResolver(p.InsecureSkipTLSverify, p.PlainHTTP); err != nil { | ||
return out.String(), err | ||
} | ||
} | ||
Comment on lines
+80
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pytimer - can we also make sure this is used in See #10659 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missing it, i will try it tomorrow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you have the bandwidth to add to install/upgrade, please do! |
||
|
||
c := downloader.ChartDownloader{ | ||
Out: &out, | ||
Keyring: p.Keyring, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ package registry // import "helm.sh/helm/v3/pkg/registry" | |
|
||
import ( | ||
"context" | ||
"crypto/tls" | ||
"encoding/json" | ||
"fmt" | ||
"io" | ||
|
@@ -145,6 +146,36 @@ func ClientOptCredentialsFile(credentialsFile string) ClientOption { | |
} | ||
} | ||
|
||
func (c *Client) newResolver(insecure, plainHTTP bool) (remotes.Resolver, error) { | ||
headers := http.Header{} | ||
headers.Set("User-Agent", version.GetUserAgent()) | ||
opts := []auth.ResolverOption{auth.WithResolverHeaders(headers)} | ||
|
||
if insecure { | ||
httpClient := http.DefaultClient | ||
httpClient.Transport = &http.Transport{ | ||
TLSClientConfig: &tls.Config{ | ||
InsecureSkipVerify: true, | ||
}, | ||
} | ||
opts = append(opts, auth.WithResolverClient(httpClient)) | ||
} | ||
if plainHTTP { | ||
opts = append(opts, auth.WithResolverPlainHTTP()) | ||
} | ||
|
||
return c.authorizer.ResolverWithOpts(opts...) | ||
} | ||
|
||
func (c *Client) WithResolver(insecure, plainHTTP bool) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm coming across this PR as I'm implementing HTTP support for OCI in Flux right now and I'd love to see it progressing. Thanks for coming up with this change. Wouldn't this be more in line with how all other options of a client are configured if this was a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I do agree, I prefer that We need that functionality in Kubeapps as we are currently using the Helm pre 3.8 code for pulling OCI artifacts and, without customizing the resolver... we can't use it as we need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like @antgamdia said, you can see new pr, or this commit that base on @antgamdia pr, #10638 (comment). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hu, that's funny as I also created a PR, just that it only adds the specific |
||
resolver, err := c.newResolver(insecure, plainHTTP) | ||
if err != nil { | ||
return err | ||
} | ||
c.resolver = resolver | ||
return nil | ||
} | ||
|
||
type ( | ||
// LoginOption allows specifying various settings on login | ||
LoginOption func(*loginOperation) | ||
|
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.
Should this just be--insecure
?Nvm, looks like this is same flag we use for
helm repo
commandsThere 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.
I see the
helm pull
useinsecure-skip-tls-verify
, so i add this flag inhelm push
, should i change it?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.
no, lets keep like this