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

Push to OCI insecure registry #10408

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/helm/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func addChartPathOptionsFlags(f *pflag.FlagSet, c *action.ChartPathOptions) {
f.BoolVar(&c.InsecureSkipTLSverify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download")
f.StringVar(&c.CaFile, "ca-file", "", "verify certificates of HTTPS-enabled servers using this CA bundle")
f.BoolVar(&c.PassCredentialsAll, "pass-credentials", false, "pass credentials to all domains")
f.BoolVar(&c.PlainHTTP, "plain-http", false, "use plain http to connect oci registry")
}

// bindOutputFlag will add the output flag to the given command and bind the
Expand Down
4 changes: 4 additions & 0 deletions cmd/helm/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

@jdolitsky jdolitsky Feb 8, 2022

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 commands

Copy link
Author

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 use insecure-skip-tls-verify, so i add this flag in helm push, should i change it?

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

The 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)

@sabre1041
Functionality within this PR is good. However, I am concerned with the differentiation of terms between helm registry login and these subcommand.

--insecure is used in helm registry login and --insecure-skip-tls-verify is used here. Why the differentiation? There should be a single way to specifying ignoring self signed certificates

@pytimer
The helm pull use insecure-skip-tls-verify when use chart repository, so use this flag to be consistent in helm push/pull

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@scottrigby Would it make sense to incorporate --insecure-skip-tls-verify into the helm registry login command since it already has existing precedence in other commands?

f.BoolVar(&client.PlainHTTP, "plain-http", false, "use plain http and not https to connect oci registry")

return cmd
}
7 changes: 7 additions & 0 deletions pkg/action/install.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ type ChartPathOptions struct {
Username string // --username
Verify bool // --verify
Version string // --version
PlainHTTP bool // --plain-http

// registryClient provides a registry client but is not added with
// options from a flag
Expand Down Expand Up @@ -694,6 +695,12 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) (
return name, errors.Errorf("path %q not found", name)
}

if c.InsecureSkipTLSverify {
if err := c.registryClient.WithResolver(c.InsecureSkipTLSverify, c.PlainHTTP); err != nil {
return "", err
}
}

dl := downloader.ChartDownloader{
Out: os.Stdout,
Keyring: c.Keyring,
Expand Down
7 changes: 7 additions & 0 deletions pkg/action/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type Pull struct {
VerifyLater bool
UntarDir string
DestDir string
PlainHTTP bool
cfg *Configuration
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

@pytimer - can we also make sure this is used in helm install / helm upgrade?

See #10659 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

I missing it, i will try it tomorrow.

Copy link
Author

Choose a reason for hiding this comment

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

I see the helm install/upgrade code, now --insecure-skip-tls-verify not used in install/upgrade command when scheme is oci://.
I can do it in helm install/upgrade to support insecure OCI registry, but i'm not sure that in this pr or open another pr to do it.

Copy link
Contributor

@jdolitsky jdolitsky Feb 10, 2022

Choose a reason for hiding this comment

The 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,
Expand Down
12 changes: 10 additions & 2 deletions pkg/action/push.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ import (
//
// It provides the implementation of 'helm push'.
type Push struct {
Settings *cli.EnvSettings
cfg *Configuration
Settings *cli.EnvSettings
cfg *Configuration
InsecureSkipTLSverify bool
PlainHTTP bool
}

// PushOpt is a type of function that sets options for a push action.
Expand All @@ -56,6 +58,12 @@ func NewPushWithOpts(opts ...PushOpt) *Push {
func (p *Push) Run(chartRef string, remote 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
}
}

c := uploader.ChartUploader{
Out: &out,
Pushers: pusher.All(p.Settings),
Expand Down
31 changes: 31 additions & 0 deletions pkg/registry/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package registry // import "helm.sh/helm/v3/pkg/registry"

import (
"context"
"crypto/tls"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -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 {
Copy link

Choose a reason for hiding this comment

The 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 ClientOption?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I do agree, I prefer that ClientOption approach as well (more info). In fact, my PR got stuck for months and I ended up creating a new one just performing this simple change. I really hope the maintainers can move it forward soon.

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.
BTW, looking forward to getting HTTP support in Flux, we will give it a try in Kubeapps soon :P

Copy link
Author

Choose a reason for hiding this comment

The 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).
I also hope the manitainers can move these pr soon, because i use harbor self-sign OCI registry.

Copy link

@makkes makkes Jul 13, 2022

Choose a reason for hiding this comment

The 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 ClientOption for mandating plain HTTP. We'll have to figure out the desired API and machinery.

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)
Expand Down