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

feat: add action to handle SSL values as secrets for TLS configuration #394

Merged
merged 16 commits into from
Mar 22, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Mar 18, 2024

⚠️ WARNING: This feature has been added due to #380, but will be supported only in 1.17 and 1.21 (and the versions released in between, iff any), after that, it will be removed in favour of integrating with a TLS certificates provider. Documentation will be provided for upgrades and migrations.

This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.

  • save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
    which the charm saves in a juju secret (owned by the charm) and uses
    them to reconcile the ingress Gateway with such information.
  • remove-tls-secret: a handy action that allows users to remove the
    TLS secret, which in turn removes the TLS configuration from the
    ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380

Manual testing instructions

This feature requires juju 3.x

  1. Deploy istio-operators from latest/edge and relate them
  2. Pack the istio-pilot charm and refresh. Wait for active and idle.
  3. Make sure the gateway resource is not configured for TLS:
$ kubectl get gateways -n istio-secrets istio-gateway -oyaml
apiVersion: networking.istio.io/v1beta1
kind: Gateway
metadata:
  creationTimestamp: "2024-03-18T13:01:38Z"
  generation: 5
  labels:
    app.juju.is/created-by: istio-pilot
    app.kubernetes.io/instance: istio-pilot-istio-secrets
    kubernetes-resource-handler-scope: gateway
  name: istio-gateway
  namespace: istio-secrets
  resourceVersion: "46114"
  uid: 57374582-c0fd-4b80-96e8-138196371cf9
spec:
  selector:
    istio: ingressgateway
  servers:
  - hosts:
    - '*'
    port:
      name: http
      number: 8080
      protocol: HTTP
  1. Run the save-tls-secret action to pass values (strings)
$ juju run istio-pilot/0 save-tls-secret ssl-key=foo ssl-crt=bar
Running operation 111 with 1 task
  - task 112 on unit-istio-pilot-0

Waiting for task 112...
  1. Confirm these values have been passed to the Secret the gateway uses for TLS:
$ kubectl get gateways -n istio-secrets istio-gateway -oyaml
apiVersion: networking.istio.io/v1beta1
kind: Gateway
metadata:
  creationTimestamp: "2024-03-18T13:01:38Z"
  generation: 6
  labels:
    app.juju.is/created-by: istio-pilot
    app.kubernetes.io/instance: istio-pilot-istio-secrets
    kubernetes-resource-handler-scope: gateway
  name: istio-gateway
  namespace: istio-secrets
  resourceVersion: "46454"
  uid: 57374582-c0fd-4b80-96e8-138196371cf9
spec:
  selector:
    istio: ingressgateway
  servers:
  - hosts:
    - '*'
    port:
      name: https
      number: 8443
      protocol: HTTPS # <---- it is now using HTTPS
    tls:
      credentialName: istio-gateway-gateway-secret # <---- It is now pointing at a secret
      mode: SIMPLE
$ kubectl get secret -nistio-secrets istio-gateway-gateway-secret -oyaml
apiVersion: v1
data:
  tls.crt: YmFy # <--- base64 encoded string
  tls.key: Zm9v # <--- base64 encoded string
kind: Secret
metadata:
  creationTimestamp: "2024-03-18T15:24:48Z"
  labels:
    app.juju.is/created-by: istio-pilot
    app.kubernetes.io/instance: istio-pilot-istio-secrets
    kubernetes-resource-handler-scope: gateway
  name: istio-gateway-gateway-secret
  namespace: istio-secrets
  resourceVersion: "46453"
  uid: f77394a2-a773-4407-8c1f-eb6a18db996c
type: kubernetes.io/tls
  1. Test other cases:
  • Remove the secret with remove-tls-secret action and watch the Gateway be reconfigured w/o TLS
  • Pass only one value to the save-tls-secret action and watch the unit go to BlockedStatus
  • Do an upgrade from 1.17/stable to this version of the charm

This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.
- save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
  which the charm saves in a juju secret (owned by the charm) and uses
  them to reconcile the ingress Gateway with such information.
- remove-tls-secret: a handy action that allows users to remove the
  TLS secret, which in turn removes the TLS configuration from the
  ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380
@DnPlas DnPlas force-pushed the KF-5346-add-ssl-secrets branch from 7a598d8 to 6b0a0a8 Compare March 20, 2024 08:01
@DnPlas DnPlas changed the title Kf 5346 add ssl secrets feat: add action to handle SSL values as secrets for TLS configuration Mar 20, 2024
@DnPlas DnPlas marked this pull request as ready for review March 20, 2024 08:11
@DnPlas DnPlas requested a review from a team as a code owner March 20, 2024 08:11
Copy link
Contributor

@NohaIhab NohaIhab left a comment

Choose a reason for hiding this comment

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

thank you @DnPlas
left some comments, and I suggest @ca-scribner gives it a look as well

charms/istio-pilot/actions.yaml Outdated Show resolved Hide resolved
charms/istio-pilot/actions.yaml Outdated Show resolved Hide resolved
charms/istio-pilot/actions.yaml Outdated Show resolved Hide resolved
charms/istio-pilot/actions.yaml Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/tests/unit/test_charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/tests/unit/test_charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/actions.yaml Show resolved Hide resolved
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

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

Added a first pass with initial thoughts. Will dig deeper next.

The main comment so far is that I think we should avoid running a partial reconciliation of the charm for handling secret events. We were burned on that in this charm in the past, and moved to doing a full reconcile on all events. afaict that same logic works well here too

charms/istio-pilot/actions.yaml Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
@DnPlas DnPlas changed the base branch from main to KF-5346-dev-branch March 21, 2024 15:05
@DnPlas DnPlas changed the base branch from KF-5346-dev-branch to main March 21, 2024 15:17
@DnPlas
Copy link
Contributor Author

DnPlas commented Mar 21, 2024

Unit tests are blocked because of #395. The issue should be resolved after merging #396 and rebasing this branch.

@DnPlas DnPlas merged commit e0d2c02 into main Mar 22, 2024
19 checks passed
@DnPlas DnPlas deleted the KF-5346-add-ssl-secrets branch March 22, 2024 14:26
DnPlas added a commit that referenced this pull request Mar 26, 2024
#394)

* feat: add action to handle SSL values as secrets for TLS configuration

This commits introduces actions that allow users to configure the TLS
ingress gateway for a single host directly passing the SSL cert and key
to the charm.
- save-tls-secret: allows users to pass the ssl-key and ssl-crt values,
  which the charm saves in a juju secret (owned by the charm) and uses
  them to reconcile the ingress Gateway with such information.
- remove-tls-secret: a handy action that allows users to remove the
  TLS secret, which in turn removes the TLS configuration from the
  ingress Gateway.

This commit also adds unit and integration tests to increase the
coverage due to the recent changes.

WARNING: please note this feature is only supported in 1.17 and 1.18,
and it will be removed after releasing 1.18 in favour of the TLS
provider method.

Fixes #380
DnPlas added a commit that referenced this pull request Mar 26, 2024
feat: add action to handle SSL values as secrets for TLS configuration (#394)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ssl-key and ssl-crt removal broke compatibility with a specific TLS use case
3 participants