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

Support Insecure Registries #2077

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

Conversation

prashantrewar
Copy link

@prashantrewar prashantrewar commented Feb 21, 2024

Summary

Output

Before

After

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1916

@prashantrewar prashantrewar requested review from a team as code owners February 21, 2024 16:00
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Feb 21, 2024
@github-actions github-actions bot added this to the 0.34.0 milestone Feb 21, 2024
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 22.50000% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 79.52%. Comparing base (6fc092b) to head (5355424).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2077      +/-   ##
==========================================
- Coverage   79.72%   79.52%   -0.19%     
==========================================
  Files         176      176              
  Lines       13263    13300      +37     
==========================================
+ Hits        10573    10576       +3     
- Misses       2021     2048      +27     
- Partials      669      676       +7     
Flag Coverage Δ
os_linux ?
os_macos 76.15% <12.50%> (-0.19%) ⬇️
os_windows 78.94% <22.50%> (-0.17%) ⬇️
unit 78.93% <22.50%> (-0.79%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

internal/build/lifecycle_execution.go Outdated Show resolved Hide resolved
pkg/client/client.go Show resolved Hide resolved
Signed-off-by: Prashant Rewar <[email protected]>
@@ -51,7 +51,7 @@ func Rebase(logger logging.Logger, cfg config.Config, pack PackClient) *cobra.Co
cmd.Flags().StringVar(&policy, "pull-policy", "", "Pull policy to use. Accepted values are always, never, and if-not-present. The default is always")
cmd.Flags().StringVar(&opts.ReportDestinationDir, "report-output-dir", "", "Path to export build report.toml.\nOmitting the flag yield no report file.")
cmd.Flags().BoolVar(&opts.Force, "force", false, "Perform rebase operation without target validation (only available for API >= 0.12)")

cmd.Flags().StringSliceVarP(&opts.InsecureRegistries, "insecure-registry", "", nil, "List of insecure registries")
Copy link
Member

Choose a reason for hiding this comment

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

I think, I will add something similar to the --force flag.

cmd.Flags().StringSliceVarP(&opts.InsecureRegistries, "insecure-registry", "", nil, "List of insecure registries (only available for API >= 0.13)")

@@ -231,7 +239,7 @@ func NewClient(opts ...Option) (*Client, error) {
}

if client.imageFetcher == nil {
client.imageFetcher = image.NewFetcher(client.logger, client.docker, image.WithRegistryMirrors(client.registryMirrors), image.WithKeychain(client.keychain))
client.imageFetcher = image.NewFetcher(client.logger, client.docker, image.WithRegistryMirrors(client.registryMirrors), image.WithKeychain(client.keychain), image.WithInsecureRegistries(client.insecureRegistries))
Copy link
Member

Choose a reason for hiding this comment

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

I think there is a misunderstood with this code, for what I can see, client.insecureRegistries is always going to be empty. because this option is never called when we initialized the client.

Usually, the client is being initialized with configuration we save in the pack configuration file, see pack config command. In this case, the insecure-registries are configured at runtime when the user invokes the pack build or pack rebase commands, but at that point in time, the client was already initialized and the image fetcher was already created.

I think we need a way to passthrough the insecure-registries to the Fetch method at runtime

Copy link
Member

Choose a reason for hiding this comment

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

@prashantrewar Do you have a chance to take a look at this?

Copy link
Author

Choose a reason for hiding this comment

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

I apologize @jjbustamante, right now I am busy with something, on the weekend I will try to fix it.

@natalieparellano natalieparellano modified the milestones: 0.34.0, 0.35.0 Apr 29, 2024
@natalieparellano
Copy link
Member

Any further updates here?

@jjbustamante
Copy link
Member

Hi, @prashantrewar! Do you have some bandwidth to take a look at this?

@prashantrewar
Copy link
Author

Hi, @prashantrewar! Do you have some bandwidth to take a look at this?

@jjbustamante Sorry, recently I'm busy with something else.

@natalieparellano natalieparellano removed this from the 0.35.0 milestone Jul 8, 2024
@natalieparellano natalieparellano added this to the 0.37.0 milestone Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Insecure Registries
3 participants