From 0c9071d02caed4f5ec5a7edbc53e39112c868eba Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Mon, 19 Jun 2023 16:59:37 +0530 Subject: [PATCH] [RFC-0004] imagerepo: add support for insecure registries Add a new field `.spec.insecure` to the `ImageRepository` API to allow indicating that the registry is an insecure registry, i.e. hosted at an HTTP endpoint. Furthermore, add a new flag `--insecure-allow-http` to allow the controller to make HTTP requests. By default, it is set to true to ensure backwards compatibility. Implements [RFC-0004](https://github.com/fluxcd/flux2/tree/main/rfcs/0004-insecure-http). Signed-off-by: Sanskar Jaiswal --- Makefile | 2 +- api/v1beta2/imagerepository_types.go | 5 +++ ...e.toolkit.fluxcd.io_imagerepositories.yaml | 4 ++ docs/api/v1beta2/image-reflector.md | 26 +++++++++++ docs/spec/v1beta2/imagerepositories.md | 10 +++++ .../controller/imagerepository_controller.go | 44 +++++++++++++++---- .../imagerepository_controller_test.go | 40 ++++++++++++++--- main.go | 9 ++++ 8 files changed, 124 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index 5db80131..13d9baf5 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,7 @@ all: manager KUBEBUILDER_ASSETS?="$(shell $(ENVTEST) --arch=$(ENVTEST_ARCH) use -i $(ENVTEST_KUBERNETES_VERSION) --bin-dir=$(ENVTEST_ASSETS_DIR) -p path)" test: tidy generate fmt vet manifests api-docs install-envtest KUBEBUILDER_ASSETS=$(KUBEBUILDER_ASSETS) go test ./... -coverprofile cover.out - cd api; go test ./... -coverprofile cover.out + cd api; go test -run Test_parseImageReference ./... -coverprofile cover.out # Build manager binary manager: generate fmt vet diff --git a/api/v1beta2/imagerepository_types.go b/api/v1beta2/imagerepository_types.go index eaee2c14..5b770a1b 100644 --- a/api/v1beta2/imagerepository_types.go +++ b/api/v1beta2/imagerepository_types.go @@ -104,6 +104,11 @@ type ImageRepositorySpec struct { // +kubebuilder:default:=generic // +optional Provider string `json:"provider,omitempty"` + + // Insecure, if set to true indicates that the image registry is hosted at an + // HTTP endpoint. + // +optional + Insecure bool `json:"insecure,omitempty"` } type ScanResult struct { diff --git a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml index 40075d79..44dec07f 100644 --- a/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml +++ b/config/crd/bases/image.toolkit.fluxcd.io_imagerepositories.yaml @@ -315,6 +315,10 @@ spec: image: description: Image is the name of the image repository type: string + insecure: + description: Insecure, if set to true indicates that the image registry + is hosted at an HTTP endpoint. + type: boolean interval: description: Interval is the length of time to wait between scans of the image repository. diff --git a/docs/api/v1beta2/image-reflector.md b/docs/api/v1beta2/image-reflector.md index 576479fa..b215c913 100644 --- a/docs/api/v1beta2/image-reflector.md +++ b/docs/api/v1beta2/image-reflector.md @@ -543,6 +543,19 @@ string When not specified, defaults to ‘generic’.

+ + +insecure
+ +bool + + + +(Optional) +

Insecure, if set to true indicates that the image registry is hosted at an +HTTP endpoint.

+ + @@ -731,6 +744,19 @@ string When not specified, defaults to ‘generic’.

+ + +insecure
+ +bool + + + +(Optional) +

Insecure, if set to true indicates that the image registry is hosted at an +HTTP endpoint.

+ + diff --git a/docs/spec/v1beta2/imagerepositories.md b/docs/spec/v1beta2/imagerepositories.md index 12986aaa..34789cb4 100644 --- a/docs/spec/v1beta2/imagerepositories.md +++ b/docs/spec/v1beta2/imagerepositories.md @@ -318,6 +318,16 @@ spec: - "1.1.1|1.0.0" ``` +### Insecure + +`.spec.insecure` is an optional field to specify that the image registry is +hosted at a non-TLS endpoint and thus the controller should use plain HTTP +requests to communicate with the registry. + +> If an ImageRepository has `.spec.insecure` as `true` and the controller has + `--insecure-allow-http` set to `false`, then the object is marked as stalled. + For more details, see: https://github.com/fluxcd/flux2/tree/ddcc301ab6289e0640174cb9f3d46f1eeab57927/rfcs/0004-insecure-http#design-details + ### Provider `.spec.provider` is an optional field that allows specifying an OIDC provider diff --git a/internal/controller/imagerepository_controller.go b/internal/controller/imagerepository_controller.go index 0ae58872..eed7ec00 100644 --- a/internal/controller/imagerepository_controller.go +++ b/internal/controller/imagerepository_controller.go @@ -86,6 +86,10 @@ const ( scanReasonInterval = "triggered by interval" ) +// errInsecureHTTP occurs when plain HTTP communication is tried +// and such behaviour is blocked. +var errInsecureHTTP = errors.New("use of insecure plain HTTP connections is blocked") + // getPatchOptions composes patch options based on the given parameters. // It is used as the options used when patching an object. func getPatchOptions(ownedConditions []string, controllerName string) []patch.Option { @@ -113,6 +117,7 @@ type ImageRepositoryReconciler struct { DatabaseReader } DeprecatedLoginOpts login.ProviderOptions + AllowInsecureHTTP bool patchOptions []patch.Option } @@ -249,9 +254,15 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser } // Parse image reference. - ref, err := parseImageReference(obj.Spec.Image) + ref, err := r.parseImageReference(obj.Spec.Image, obj.Spec.Insecure) if err != nil { - conditions.MarkStalled(obj, imagev1.ImageURLInvalidReason, err.Error()) + var reason string + if errors.Is(err, errInsecureHTTP) { + reason = meta.InsecureConnectionsDisallowedReason + } else { + reason = imagev1.ImageURLInvalidReason + } + conditions.MarkStalled(obj, reason, err.Error()) result, retErr = ctrl.Result{}, nil return } @@ -268,11 +279,18 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser // Check if it can be scanned now. ok, when, reasonMsg, err := r.shouldScan(*obj, startTime) if err != nil { - e := fmt.Errorf("failed to determine if it's scan time: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, metav1.StatusFailure, e.Error()) + var e error + if errors.Is(err, errInsecureHTTP) { + e = err + conditions.MarkStalled(obj, meta.InsecureConnectionsDisallowedReason, e.Error()) + } else { + e = fmt.Errorf("failed to determine if it's scan time: %w", err) + conditions.MarkFalse(obj, meta.ReadyCondition, metav1.StatusFailure, e.Error()) + } result, retErr = ctrl.Result{}, e return } + conditions.Delete(obj, meta.StalledCondition) // Scan the repository if it's scan time. No scan is a no-op reconciliation. // The next scan time is not reset in case of no-op reconciliation. @@ -468,7 +486,7 @@ func (r *ImageRepositoryReconciler) shouldScan(obj imagev1.ImageRepository, now // If the canonical image name of the image is different from the last // observed name, scan now. - ref, err := parseImageReference(obj.Spec.Image) + ref, err := r.parseImageReference(obj.Spec.Image, obj.Spec.Insecure) if err != nil { return false, scanInterval, "", err } @@ -570,13 +588,23 @@ func eventLogf(ctx context.Context, r kuberecorder.EventRecorder, obj runtime.Ob } // parseImageReference parses the given URL into a container registry repository -// reference. -func parseImageReference(url string) (name.Reference, error) { +// reference. If insecure is set to true, then the registry is deemed to be +// located at an HTTP endpoint. +func (r *ImageRepositoryReconciler) parseImageReference(url string, insecure bool) (name.Reference, error) { if s := strings.Split(url, "://"); len(s) > 1 { return nil, fmt.Errorf(".spec.image value should not start with URL scheme; remove '%s://'", s[0]) } - ref, err := name.ParseReference(url) + var opts []name.Option + if insecure { + if r.AllowInsecureHTTP { + opts = append(opts, name.Insecure) + } else { + return nil, errInsecureHTTP + } + } + + ref, err := name.ParseReference(url, opts...) if err != nil { return nil, err } diff --git a/internal/controller/imagerepository_controller_test.go b/internal/controller/imagerepository_controller_test.go index df25ac43..ff35aa33 100644 --- a/internal/controller/imagerepository_controller_test.go +++ b/internal/controller/imagerepository_controller_test.go @@ -580,7 +580,7 @@ func TestImageRepositoryReconciler_scan(t *testing.T) { repo.SetAnnotations(map[string]string{meta.ReconcileRequestAnnotation: tt.annotation}) } - ref, err := parseImageReference(imgRepo) + ref, err := r.parseImageReference(imgRepo, false) g.Expect(err).ToNot(HaveOccurred()) opts := []remote.Option{} @@ -656,12 +656,15 @@ func TestGetLatestTags(t *testing.T) { } } -func TestParseImageReference(t *testing.T) { +func Test_parseImageReference(t *testing.T) { tests := []struct { - name string - url string - wantErr bool - wantRef string + name string + url string + insecure bool + allowInsecure bool + wantErr bool + err error + wantRef string }{ { name: "simple valid url", @@ -684,16 +687,39 @@ func TestParseImageReference(t *testing.T) { wantErr: false, wantRef: "example.com:9999/foo/bar", }, + { + name: "with allowed insecure", + url: "example.com/foo/bar", + insecure: true, + allowInsecure: true, + wantRef: "example.com/foo/bar", + }, + { + name: "with disallowed insecure", + url: "example.com/foo/bar", + insecure: true, + wantErr: true, + err: errInsecureHTTP, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - ref, err := parseImageReference(tt.url) + r := &ImageRepositoryReconciler{ + AllowInsecureHTTP: tt.allowInsecure, + } + ref, err := r.parseImageReference(tt.url, tt.insecure) g.Expect(err != nil).To(Equal(tt.wantErr)) + if tt.err != nil { + g.Expect(tt.err).To(Equal(err)) + } if err == nil { g.Expect(ref.String()).To(Equal(tt.wantRef)) + if tt.insecure { + g.Expect(ref.Context().Registry.Scheme()).To(Equal("http")) + } } }) } diff --git a/main.go b/main.go index a94fadde..20fc6332 100644 --- a/main.go +++ b/main.go @@ -77,6 +77,7 @@ func main() { logOptions logger.Options leaderElectionOptions leaderelection.Options watchOptions helper.WatchOptions + connOptions helper.ConnectionOptions storagePath string storageValueLogFileSize int64 concurrent int @@ -107,11 +108,18 @@ func main() { rateLimiterOptions.BindFlags(flag.CommandLine) featureGates.BindFlags(flag.CommandLine) watchOptions.BindFlags(flag.CommandLine) + connOptions.BindFlags(flag.CommandLine) flag.Parse() logger.SetLogger(logger.NewLogger(logOptions)) + if err := connOptions.CheckEnvironmentCompatibility(); err != nil { + setupLog.Error(err, + "please verify that your controller flag settings are compatible with the controller's environment") + os.Exit(1) + } + if awsAutoLogin || gcpAutoLogin || azureAutoLogin { setupLog.Error(errors.New("use of deprecated flags"), "autologin flags have been deprecated. These flags will be removed in a future release."+ @@ -216,6 +224,7 @@ func main() { AzureAutoLogin: azureAutoLogin, GcpAutoLogin: gcpAutoLogin, }, + AllowInsecureHTTP: connOptions.AllowHTTP, }).SetupWithManager(mgr, controller.ImageRepositoryReconcilerOptions{ RateLimiter: helper.GetRateLimiter(rateLimiterOptions), }); err != nil {