Skip to content

Commit

Permalink
fix markdown formatting and update status conditions
Browse files Browse the repository at this point in the history
Signed-off-by: Sanskar Jaiswal <[email protected]>
  • Loading branch information
aryan9600 committed Oct 20, 2022
1 parent 3436122 commit bd571df
Showing 1 changed file with 32 additions and 24 deletions.
56 changes: 32 additions & 24 deletions rfcs/0004-insecure-http/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,38 @@
**Creation Date:** 2022-09-08

## Summary

Flux should have a consistent way of disabling insecure HTTP connections.

At the controller level, a flag should be present which would disable all outgoing HTTP connections.
At the object level, a field should be provided which would enable the use of non-TLS endpoints.

If the use of a non-TLS endpoint is not supported, it should be made clear to users through the use of
logs and status conditions.
If the use of a non-TLS endpoint is not supported, reconciliation will fail and the object will be marked
as stalled, signalling that human intervention is required.

## Motivation

Today the use of non-TLS based connections is inconsistent across Flux controllers.

Controllers that deal only with `http` and `https` schemes have no way to block use of the `http` scheme at controller-level.
Some Flux objects provide a `.spec.insecure` field to enable the use of non-TLS based endpoints, but they don't clearly notify
users when the option is not supported (e.g. Azure/GCP Buckets).

### Goals

* Provide a flag across relevant Flux controllers which disables all outgoing HTTP connections.
* Add a field which enables the use of non-TLS endpoints to appropriate Flux objects.
* Provide a way for users to be made aware that their use of non-TLS endpoints is not supported if that is the case.

### Non-Goals

* Break Flux's current behavior of allowing HTTP connections.
* Change in behavior of communication between Flux components.

## Proposal

### Controllers

Flux users should be able to enforce that controllers are using HTTPS connections only.
This shall be enabled by adding a new boolean flag `--insecure-allow-http` to the following controllers:
* source-controller
Expand Down Expand Up @@ -60,27 +65,24 @@ spec:
- --insecure-allow-http=false
```
> Note: The flag shall not be added to the following controllers:
> * kustomize-controller: This flag is excluded from this controller, as the upstream `kubenetes-sigs/kustomize` project
> does not support disabling HTTP connections while fetching resources from remote bases. We can revisit this if the
> upstream project adds support for this at a later point in time.
> * helm-controller: This flag does not serve a purpose in this controller, as the controller does not make any HTTP calls.
> Furthermore although both controllers can also do remote applies, serving `kube-apiserver` over plain
> HTTP is disabled by default. While technically this can be enabled, the option for this configuration was also disabled
> quite a while back (ref: https://github.com/kubernetes/kubernetes/pull/65830/).

Both kustomize-controller and helm-controller currently have a flag `--insecure-kubeconfig-tls` which makes the controller skip
TLS verification when connecting to a Kubernetes cluster with an HTTPS connection. This flag shall be renamed to
`--insecure-skip-tls-verify` to align it with the Flux CLI which offers this command for the same purpose.
**Note:** The flag shall not be added to the following controllers:
* kustomize-controller: This flag is excluded from this controller, as the upstream `kubenetes-sigs/kustomize` project
does not support disabling HTTP connections while fetching resources from remote bases. We can revisit this if the
upstream project adds support for this at a later point in time.
* helm-controller: This flag does not serve a purpose in this controller, as the controller does not make any HTTP calls.
Furthermore although both controllers can also do remote applies, serving `kube-apiserver` over plain
HTTP is disabled by default. While technically this can be enabled, the option for this configuration was also disabled
quite a while back (ref: https://github.com/kubernetes/kubernetes/pull/65830/).

### Objects

Some Flux objects, like `GitRepository`, provide a field for specifying a URL, and the URL would contain the scheme.
In such cases, the scheme can be used for inferring the transport type of the connection and consequently,
whether to use HTTP or HTTPS connections for that object.
But there are a few objects that don't allow such behavior, for example:

* `ImageRepository`: It provides a field, `.spec.image`, which is used for specifying the URL of the image present on
a container registry. But any URL containing a scheme is considered invalid and HTTPS is the default transport used.
* `ImageRepository`: It provides a field, `.spec.image`, which is used for specifying the address of the image present on
a container registry. But any address containing a scheme is considered invalid and HTTPS is the default transport used.
This prevents users from using images present on insecure registries.
* OCI `HelmRepository`: When using an OCI registry as a Helm repository, the `.spec.url` is expected to begin with `oci://`.
Since the scheme part of the URL is used to specify the type of `HelmRepository`, there is no way for users to specify
Expand All @@ -90,6 +92,7 @@ For such objects, we shall introduce a new boolean field `.spec.insecure`, which
need their object to point to an HTTP endpoint, can set this to `true`.

### CLI

The Flux CLI offers several commands for creating Flux specific resources. Some of these commands may involve specifying
an endpoint such as creating an `OCIRepository`:

Expand All @@ -107,6 +110,7 @@ for the required commands, which will be used for specifying the value of `.spec
> when using an HTTPS connection.

### Precedence & Validity

Objects with `.spec.insecure` as `true ` will only be allowed if HTTP connections are allowed at the controller level.
Similarly, an object can have `.spec.insecure` as `true` only if the Saas/Cloud provider allows HTTP connections.
For example, using a `Bucket` with its `.spec.provider` set to `azure` would be invalid since Azure doesn't allow
Expand All @@ -115,6 +119,7 @@ HTTP connections.
### User Stories

#### Story 1

> As a cluster admin of a multi-tenant cluster, I want to ensure all controllers access endpoints using only HTTPS
> regardless of tenants' object definitions.

Expand All @@ -134,8 +139,8 @@ patches:
target:
kind: Deployment
name: "(source-controller|notification-controller|image-reflector-controller|image-automation-controller)"
# Since this above flag is not available in kustomize-controller for reasons explained in a previous section,
# we disable the Kustomize remote build by disallowing use of remote bases. This ensures that kustomize-controller
# Since the above flag is not available in kustomize-controller for reasons explained in a previous section,
# we disable Kustomize remote builds by disallowing use of remote bases. This ensures that kustomize-controller
# won't initiate any plain HTTP connections.
- patch: |
- op: add
Expand All @@ -147,6 +152,7 @@ patches:
```

#### Story 2

> As an application developer, I'm trying to debug a new image pushed to my local registry which
> is not served over HTTPS.

Expand All @@ -164,14 +170,16 @@ spec:
```

### Alternatives

Instead of adding a flag, we can instruct users to make use of Kyverno policies to enforce that
all objects have `.spec.insecure` as `false` and any URLs present in the definition don't have `http`
as the scheme. This is less attractive, as this would ask users to install another software and prevent
Flux multi-tenancy from being standalone.

## Design Details

If a controller is started with `--insecure-allow-http=false`, any URL in a Flux object which has `http`
as the scheme will result in an error and the following condition will be added to the object's
as the scheme will result in an unsuccessful reconciliation and the following condition will be added to the object's
`.status.conditions`:

```yaml
Expand All @@ -180,23 +188,23 @@ status:
- lastTransitionTime: "2022-09-06T09:14:21Z"
message: "Use of insecure HTTP connections isn't allowed for this controller"
observedGeneration: 1
reason: URLInvalid
reason: InsecureConnectionsDisallowed
status: "True"
type: FetchFailedCondition
type: Stalled
```

Similarly, if an object has `.spec.insecure` as `true` but the Cloud provider doesn't allow HTTP connections,
the reconciler will error out and add the condition below to the object's `.status.conditions`:
the reconciliation will fail and the following condition will be added to the object's `.status.conditions`:

```yaml
status:
conditions:
- lastTransitionTime: "2022-09-06T09:14:21Z"
message: "Use of insecure HTTP connections isn't allowed for Azure Storage"
observedGeneration: 1
reason: InsecureConnectionsDisallowed
reason: UnsupportedConnectionType
status: "True"
type: FetchFailedCondition
type: Stalled
```

If an object has `.spec.insecure` as `true`, the registry client or bucket client shall be created with the use
Expand Down

0 comments on commit bd571df

Please sign in to comment.