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

refactor: use secret config instead of action for configuring TLS ingress gateway #401

Merged
merged 15 commits into from
May 29, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Apr 2, 2024

⚠️ WARNING: This feature has been added due to #380, but will be supported only in 1.17 and 1.22 (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.

The recommended way of handling user secrets is by saving the user secret ID in a charm configuration, that way the charm can use it to get the contents of the secret. When the charm is granted with the user secret via the CLI, it allows the charm to observe secret events, which help handle changes in their contents. The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because of the above, this commit does a small refactor to remove the set-tls and unset-tls actions and instead use user secrets directly.

This commit also renames ssl to tls to be more accurate when referring to the actual configuration that gets done in the Gateway.

Part of #398

Merge after #402

Manual testing

NOTE: this feature works with juju > 3.3, installing and bootstrapping a controller version 3.4 is recommended to be in sync with the CI (changes introduced by #402).

  • Add a secret to configure Ingress Gateway with TLS
  1. Build and deploy the charms from this branch. Relate them and wait for them to become active and idle.
  2. Add a user secret juju add-secret istio-tls-secret tls-crt=<some str> tls-key=<another str>
  3. Grant access for istio-pilot to access the secret contents juju grant-secret istio-tls-secret istio-pilot
  4. From the output of command in step 2, pass the secret ID to the istio-pilot charm as a config option with juju config istio-pilot tls-secret-id=secret:<secret ID resulting from step 2>. If you have lost it, you can always check with juju secrets.
  5. Wait for the charm to become active and idle
  6. Verify the Gateway resource is configured with TLS and that the Secret (the kubernetes resource) has the expected values:
$ 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
  • Check the TLS configuration changes if the secret changes
  1. Change the secret contents juju update-secret secret:<secret id> tls-crt=new-crt tls-key=new-key
  2. Check a secret-changed`` message in juju debug-log. The juju status` msg should mention that the TLS configuration is being applied.
  3. Wait for the charm to become active and idle
  4. Verify the kubernetes Secret to confirm the new values are there
  • Remove the TLS configuration
  1. Remove the secret ID from the istio-pilot configuration juju config istio-pilot tls-secret-id=""
  2. Remove the user secret from the model juju remove-secret istio-tls-secret
  3. Observe the Gateway is not configured with TLS and the kubernetes Secret does not exist anymore
  • Verify that the charm can be upgraded from 1.17/stable to the charm in this branch

TODO

  • Refactor integration tests

DnPlas added a commit that referenced this pull request Apr 2, 2024
Bumping juju and ops packages to use them in newer versions of the charms,
plus testing them in a CI with a more recent juju version.

This commit also skips some test cases that will be removed in a follow
up commit introduced by #401.

Part of canonical/bundle-kubeflow#859
Part of #398

Signed-off-by: Daniela Plascencia <daniela.plascencia@canonical.com>
@DnPlas DnPlas marked this pull request as ready for review April 2, 2024 23:33
@DnPlas DnPlas requested a review from a team as a code owner April 2, 2024 23:33
@DnPlas DnPlas marked this pull request as draft April 2, 2024 23:35
@DnPlas DnPlas force-pushed the KF-5501-use-secrets-config-branch-1-17 branch from 281d923 to a836a8c Compare April 3, 2024 00:29
@DnPlas DnPlas marked this pull request as ready for review April 3, 2024 00:32
@DnPlas DnPlas marked this pull request as draft April 3, 2024 00:47
@DnPlas DnPlas force-pushed the KF-5501-use-secrets-config-branch-1-17 branch from a836a8c to 0ca9c17 Compare April 3, 2024 00:47
@DnPlas DnPlas marked this pull request as ready for review April 3, 2024 01:34
@DnPlas DnPlas marked this pull request as draft April 3, 2024 01:35
The recommended way of handling user secrets is by saving the user secret ID
in a charm configuration, that way the charm can use it to get the contents of the secret.
When the charm is granted with the user secret via the CLI, it allows the charm to observe
secret events, which help handle changes in their contents.
The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because
of the above, this commit does a small refactor to remove the set-tls and unset-tls actions
and instead directly use user secrets.

This commit also renames ssl to tls to be more accurate when referring
to the actual configuration that gets done in the Gateway.

Part of #398
@DnPlas DnPlas force-pushed the KF-5501-use-secrets-config-branch-1-17 branch from 0ca9c17 to 9f1691f Compare April 3, 2024 01:36
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.

lgtm apart from a few minor comments

Will do another pass after the blocking PR is merged/rebased

charms/istio-pilot/src/charm.py Show resolved Hide resolved
charms/istio-pilot/config.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
@DnPlas DnPlas marked this pull request as ready for review May 22, 2024 11:31
@DnPlas DnPlas mentioned this pull request May 24, 2024
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Really nice work @DnPlas! I left some comments

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 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
requirements-integration.txt Show resolved Hide resolved
charms/istio-pilot/src/charm.py Outdated Show resolved Hide resolved
charms/istio-pilot/config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Good job once more! Could you also include the PR's title/commit the this refers to TLS?

@DnPlas DnPlas changed the title refactor: use secret config instead of action refactor: use secret config instead of action for configuring TLS ingress gateway May 28, 2024
@DnPlas DnPlas merged commit 1283f97 into track/1.17 May 29, 2024
19 checks passed
@DnPlas DnPlas deleted the KF-5501-use-secrets-config-branch-1-17 branch May 29, 2024 14:28
DnPlas added a commit that referenced this pull request May 29, 2024
…ress gateway (#401)

* refactor: use secret config instead of action

The recommended way of handling user secrets is by saving the user secret ID
in a charm configuration, that way the charm can use it to get the contents of the secret.
When the charm is granted with the user secret via the CLI, it allows the charm to observe
secret events, which help handle changes in their contents.
The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because
of the above, this commit does a small refactor to remove the set-tls and unset-tls actions
and instead directly use user secrets.

This commit also renames ssl to tls to be more accurate when referring
to the actual configuration that gets done in the Gateway.

Part of #398
DnPlas added a commit that referenced this pull request May 29, 2024
…ress gateway (#401)

* refactor: use secret config instead of action

The recommended way of handling user secrets is by saving the user secret ID
in a charm configuration, that way the charm can use it to get the contents of the secret.
When the charm is granted with the user secret via the CLI, it allows the charm to observe
secret events, which help handle changes in their contents.
The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because
of the above, this commit does a small refactor to remove the set-tls and unset-tls actions
and instead directly use user secrets.

This commit also renames ssl to tls to be more accurate when referring
to the actual configuration that gets done in the Gateway.

Part of #398
DnPlas added a commit that referenced this pull request May 29, 2024
…ress gateway (#401)

* refactor: use secret config instead of action

The recommended way of handling user secrets is by saving the user secret ID
in a charm configuration, that way the charm can use it to get the contents of the secret.
When the charm is granted with the user secret via the CLI, it allows the charm to observe
secret events, which help handle changes in their contents.
The istio-pilot charm implemented a way to use actions to wrap the secret logic, but because
of the above, this commit does a small refactor to remove the set-tls and unset-tls actions
and instead directly use user secrets.

This commit also renames ssl to tls to be more accurate when referring
to the actual configuration that gets done in the Gateway.

Part of #398
DnPlas added a commit that referenced this pull request May 30, 2024
This PR cherry-picks all the work done for enabling TLS ingress gateway using juju secrets.

* docs: add instructions for TLS with secrets (#421)
* refactor: use secret config instead of action for configuring TLS ingress gateway (#401)
* fix: check routes is not empty before popping values (#424)

Fixes #398
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.

3 participants