feat: Ignore tls when pull image #449

Manoramsharma · 2024-08-13T03:35:27Z

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

N
Y

fix #408

2. What is the scope of this PR (e.g. component or file name):

pkg/oci/oci.go

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

Custom HTTP client is created using custom transport to ignore the tls while pulling a package image.

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

N
Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

Signed-off-by: Manoramsharma <[email protected]>

coveralls · 2024-08-13T03:37:39Z

Pull Request Test Coverage Report for Build 10662945577

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

22 of 31 (70.97%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.9%) to 41.589%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/downloader/downloader.go	1	5	20.0%
pkg/oci/oci.go	11	16	68.75%

Totals
Change from base Build 10589856639:	0.9%
Covered Lines:	3372
Relevant Lines:	8108

💛 - Coveralls

Peefy · 2024-08-13T03:44:00Z

You need to add a option for the OCI client users to control the tls flag.

cc @zong-zhe

Manoramsharma · 2024-08-13T04:20:43Z

You need to add a option for the OCI client users to control the tls flag.

cc @zong-zhe

Hi @

#408

You need to add a option for the OCI client users to control the tls flag.

cc @zong-zhe

Hi @Peefy in order to add an additional option or flag to allow users to control the downloading with or without tls cert. I tried to explore pkg/cmd/cmd_pull.go cli implementation here it calls PullFromOci() from client.

func KpmPull(c *cli.Context, kpmcli *client.KpmClient) error {
	return kpmcli.PullFromOci(c.Args().Get(1), c.Args().Get(0), c.String(FLAG_TAG))
}

As we have modified the NewOciClientWithOpts we just have to take care of the flag as an argument from the pull command and ultimately handling the opts in pullTarFromOci function, here to be more specific.

pkg/client/client.go

// pullTarFromOci will pull a kcl package tar file from oci registry.
func (c *KpmClient) pullTarFromOci(localPath string, ociOpts *opt.OciOptions) error {
	absPullPath, err := filepath.Abs(localPath)
	if err != nil {
		return reporter.NewErrorEvent(reporter.Bug, err)
	}

	repoPath := utils.JoinPath(ociOpts.Reg, ociOpts.Repo)
	cred, err := c.GetCredentials(ociOpts.Reg)
	if err != nil {
		return err
	}

	ociCli, err := oci.NewOciClientWithOpts(
		oci.WithCredential(cred),
		oci.WithRepoPath(repoPath),
		oci.WithSettings(c.GetSettings()),
	)

	if err != nil {
		return err
	}

	ociCli.SetLogWriter(c.logWriter)

	var tagSelected string
	if len(ociOpts.Tag) == 0 {
		tagSelected, err = ociCli.TheLatestTag()
		if err != nil {
			return err
		}
		reporter.ReportMsgTo(
			fmt.Sprintf("the lastest version '%s' will be pulled", tagSelected),
			c.logWriter,
		)
	} else {
		tagSelected = ociOpts.Tag
	}

	full_repo := utils.JoinPath(ociOpts.Reg, ociOpts.Repo)
	reporter.ReportMsgTo(
		fmt.Sprintf("pulling '%s:%s' from '%s'", ociOpts.Repo, tagSelected, full_repo),
		c.logWriter,
	)

	err = ociCli.Pull(absPullPath, tagSelected)
	if err != nil {
		return err
	}

	return nil
}

Signed-off-by: Manoramsharma <[email protected]>

pkg/oci/oci.go

pkg/client/client.go

Manoramsharma · 2024-08-13T05:09:59Z

Hi @zong-zhe made the changes accordingly, but there is this function:

// CompileOciPkg will compile the kcl package from the OCI reference or url.
func (c *KpmClient) CompileOciPkg(ociSource, version string, opts *opt.CompileOptions) (*kcl.KCLResultList, error) {
	ociOpts, err := c.ParseOciOptionFromString(ociSource, version)

	if err != nil {
		return nil, err
	}

	// 1. Create the temporary directory to pull the tar.
	tmpDir, err := os.MkdirTemp("", "")
	if err != nil {
		return nil, reporter.NewErrorEvent(reporter.Bug, err, "internal bugs, please contact us to fix it.")
	}
	// clean the temp dir.
	defer os.RemoveAll(tmpDir)

	localPath := ociOpts.SanitizePathWithSuffix(tmpDir)

	// 2. Pull the tar.
	err = c.pullTarFromOci(localPath, ociOpts)

	if err != nil {
		return nil, err
	}

	// 3.Get the (*.tar) file path.
	matches, err := filepath.Glob(filepath.Join(localPath, constants.KCL_PKG_TAR))
	if err != nil || len(matches) != 1 {
		if err != nil {
			return nil, reporter.NewErrorEvent(reporter.FailedGetPkg, err, "failed to pull kcl package")
		} else {
			return nil, errors.FailedPull
		}
	}

	return c.CompileTarPkg(matches[0], opts)
}

Also using the pullTarFromOci(), how to include the arguments here to handle the tls flag? Because of which my tests are failing else I feel the overall direction of the changes are good 👍 .

@Peefy please assign this to me, I think I have made a good progress now.

Signed-off-by: Manoramsharma <[email protected]>

zong-zhe · 2024-08-19T06:28:37Z

Hi @Manoramsharma 😄

This part of the work also includes the content here, the Pull method is provided for the KCL CLI.

https://github.com/kcl-lang/kpm/blob/main/pkg/client/pull.go

zong-zhe · 2024-08-19T06:21:56Z

pkg/client/client.go

@@ -663,7 +663,7 @@ func (c *KpmClient) CompileGitPkg(gitOpts *git.CloneOptions, compileOpts *opt.Co
 }

 // CompileOciPkg will compile the kcl package from the OCI reference or url.
-func (c *KpmClient) CompileOciPkg(ociSource, version string, opts *opt.CompileOptions) (*kcl.KCLResultList, error) {
+func (c *KpmClient) CompileOciPkg(ociSource, version string, opts *opt.CompileOptions, skipTLSVerify bool) (*kcl.KCLResultList, error) {


Try to move skipTLSVerify to CompileOptions. Do not add method parameters directly, which is a public method. This breaks compatibility.

Peefy · 2024-08-20T06:57:06Z

pkg/oci/oci.go

@@ -99,13 +100,22 @@ type OciClient struct {
 	logWriter      io.Writer
 	settings       *settings.Settings
 	isPlainHttp    *bool
+	isTLSVerify    *bool


s/isTLSVerify/skipTLSVerify

Peefy · 2024-08-20T06:59:23Z

@Manoramsharma Can you fix the issue mentioned in the comment? I will prepare to merge it.

Manoramsharma · 2024-08-20T07:21:02Z

@Manoramsharma Can you fix the issue mentioned in the comment? I will prepare to merge it.

Hi @Peefy I will complete it today.

Peefy · 2024-08-28T03:06:47Z

@Manoramsharma have you worked on this PR? We are planing release a new version with this feature.

Manoramsharma · 2024-08-28T09:40:44Z

@Manoramsharma have you worked on this PR? We are planing release a new version with this feature.

Hi @Peefy @zong-zhe I need some more context on how Pull command is working? where it fetches user CLI input and detect the flag ultimately executing the function defined against the CLI implementation.

zong-zhe · 2024-08-28T09:45:29Z

Hi @Manoramsharma 😄

The kcl mod pull command entry is here:

kpm/pkg/client/pull.go

Line 60 in a4e3061

func (c *KpmClient) Pull(options ...PullOption) (*pkg.KclPkg, error) {

Manoramsharma · 2024-08-28T10:49:17Z

Hi @Manoramsharma 😄

The kcl mod pull command entry is here:

kpm/pkg/client/pull.go

Line 60 in a4e3061

func (c *KpmClient) Pull(options ...PullOption) (*pkg.KclPkg, error) {

I have one more confusion, issue talks about the removal of tls certificate while downloading the package specifically the https is it talking about git package or the oci package?

Also I can see the downloadPkg function is called which is part of Client.go and further the downloader is called for downloading the specific package dependency. So where the role of cmd_pull.go comes in this flow?

cc: @zong-zhe

zong-zhe · 2024-08-29T02:16:41Z

I have one more confusion, issue talks about the removal of tls certificate while downloading the package specifically the https is it talking about git package or the oci package?

OCI package

zong-zhe · 2024-08-29T02:18:53Z

Also I can see the downloadPkg function is called which is part of Client.go and further the downloader is called for downloading the specific package dependency. So where the role of cmd_pull.go comes in this flow?

cmd_pull.go is a deprecated command line, and currently kpm is mainly used as a tripartite library for the CLI, which does its function by calling the methods I mentioned above.

kpm/pkg/cmd/cmd_pull.go

Line 2 in a4e3061

// Deprecated: The entire contents of this file will be deprecated.

…est cases Signed-off-by: zongz <[email protected]>

Signed-off-by: zongz <[email protected]>

Ignore tls when pull image

db1ef96

Signed-off-by: Manoramsharma <[email protected]>

Added the flag to control the tls cert

ce69a11

Signed-off-by: Manoramsharma <[email protected]>

Peefy reviewed Aug 13, 2024

View reviewed changes

pkg/oci/oci.go Show resolved Hide resolved

pkg/client/client.go Outdated Show resolved Hide resolved

added flag for tls cert while compiling pkg

2981501

Signed-off-by: Manoramsharma <[email protected]>

zong-zhe reviewed Aug 19, 2024

View reviewed changes

Peefy reviewed Aug 20, 2024

View reviewed changes

Merge branch 'main' into patch-no-tls

46d187d

zong-zhe added 2 commits September 2, 2024 15:24

feat: add 'insecureSkipVerify' to skip the insecure verify and some t…

8aee96b

…est cases Signed-off-by: zongz <[email protected]>

fix: rename 'InsecureSkipVerify' to 'InsecureSkipTLSverify'

99b76be

Signed-off-by: zongz <[email protected]>

Peefy merged commit 9c9507b into kcl-lang:main Sep 2, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ignore tls when pull image #449

feat: Ignore tls when pull image #449

Manoramsharma commented Aug 13, 2024

coveralls commented Aug 13, 2024 •

edited

Loading

Peefy commented Aug 13, 2024 •

edited

Loading

Manoramsharma commented Aug 13, 2024

Manoramsharma commented Aug 13, 2024 •

edited

Loading

zong-zhe commented Aug 19, 2024

zong-zhe Aug 19, 2024

Peefy Aug 20, 2024

Peefy commented Aug 20, 2024

Manoramsharma commented Aug 20, 2024

Peefy commented Aug 28, 2024

Manoramsharma commented Aug 28, 2024

zong-zhe commented Aug 28, 2024

Manoramsharma commented Aug 28, 2024

zong-zhe commented Aug 29, 2024

zong-zhe commented Aug 29, 2024

feat: Ignore tls when pull image #449

feat: Ignore tls when pull image #449

Conversation

Manoramsharma commented Aug 13, 2024

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

2. What is the scope of this PR (e.g. component or file name):

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

coveralls commented Aug 13, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10662945577

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Peefy commented Aug 13, 2024 • edited Loading

Manoramsharma commented Aug 13, 2024

Manoramsharma commented Aug 13, 2024 • edited Loading

zong-zhe commented Aug 19, 2024

zong-zhe Aug 19, 2024

Choose a reason for hiding this comment

Peefy Aug 20, 2024

Choose a reason for hiding this comment

Peefy commented Aug 20, 2024

Manoramsharma commented Aug 20, 2024

Peefy commented Aug 28, 2024

Manoramsharma commented Aug 28, 2024

zong-zhe commented Aug 28, 2024

Manoramsharma commented Aug 28, 2024

zong-zhe commented Aug 29, 2024

zong-zhe commented Aug 29, 2024

coveralls commented Aug 13, 2024 •

edited

Loading

Peefy commented Aug 13, 2024 •

edited

Loading

Manoramsharma commented Aug 13, 2024 •

edited

Loading