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

TLS part 1: Enable TLS if cert is available #164

Merged
merged 45 commits into from
Jul 5, 2023
Merged

TLS part 1: Enable TLS if cert is available #164

merged 45 commits into from
Jul 5, 2023

Conversation

sed-i
Copy link
Contributor

@sed-i sed-i commented Jun 14, 2023

Tracking issue: canonical/cos-lite-bundle#75

This PR enables TLS for alertmanager when a certificate is available.

This involves:

  • adding tls-certificates to metadata
  • writing private key and certificate to disk
  • writing a web config file
  • updating the pebble command with the web config file

This PR goes in tandem with canonical/observability-libs#49.

TODO:

  • Scale to two units and make sure both units' TLS endpoints are reachable
  • Remove relation and:
    • make sure TLS endpoint is NOT reachable
    • re-relate, and make sure TLS endpoint is reachable
  • Confirm TLS endpoint still reachable after an upgrade sequence
  • Think what to do with insecure_skip_verify in this line in alertmanager charm code: "global": {"http_config": {"tls_config": {"insecure_skip_verify": True}}},

@lucabello
Copy link
Contributor

I've addressed some of the missing pieces:

  • about the certificate files deletion, I've added it conditionally since container.push() overwrites the file anyway; this way we don't re-create certificates every time the configuration is updated
  • the reload logic has been moved to a separate method so it can be called in the order you mentioned was correct

Copy link
Contributor

@lucabello lucabello left a comment

Choose a reason for hiding this comment

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

CertManager has been changed to provide certificate-per-app (i.e., only the leader can generate a CSR); however, the previous scope was certificate-per-unit.

Are we saying that all units should share the same certificate?

src/charm.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
@sed-i
Copy link
Contributor Author

sed-i commented Jun 20, 2023

Tested successfully with:

# # Deploy bundle
# juju deploy --trust ./bundle.yaml
#
# # Obtain alertmanager IP address
# IPADDR=$(juju status --format json am | jq -r '.applications.am.units."am/0".address')
#
# # Make sure alertmanager web-external-hostname is locally routable
# echo "$IPADDR alertmanager.local" | sudo tee -a /etc/hosts
#
# # Make sure charm code created web-config, cert and key files
# juju ssh --container alertmanager am/0 ls /etc/alertmanager/
#
# # Inspect server cert and confirm `X509v3 Subject Alternative Name` field is as expected
# echo | openssl s_client -showcerts -servername $IPADDR:9093 -connect $IPADDR:9093 2>/dev/null | openssl x509 -inform pem -noout -text
#
# # Save CA cert locally
# juju show-unit am/0 --format yaml | yq '.am/0."relation-info"[0]."local-unit".data.ca' > /tmp/cacert.pem
#
# # Confirm alertmanager TLS endpoint reachable
# curl --fail-with-body --capath /tmp --cacert /tmp/cacert.pem https://alertmanager.local:9093/-/ready

---
bundle: kubernetes
applications:
  am:
    charm: ./alertmanager-k8s_ubuntu-20.04-amd64.charm
    series: focal
    scale: 1
    trust: true
    resources:
      alertmanager-image: ubuntu/prometheus-alertmanager:0.23-22.04_beta
    options:
      web_external_url: https://alertmanager.local
  ca:
    charm: self-signed-certificates
    channel: edge
    scale: 1
relations:
- [am:certificates, ca:certificates]

@sed-i
Copy link
Contributor Author

sed-i commented Jun 21, 2023

Now testing with this bundle (with traefik):

# # Deploy bundle
# juju deploy --trust ./bundle.yaml
#
# # Obtain IP addresses
# IPADDR=$(juju status --format json trfk | jq -r '.applications.trfk.address')
# IPADDR0=$(juju status --format json am | jq -r '.applications.am.units."am/0".address')
# IPADDR1=$(juju status --format json am | jq -r '.applications.am.units."am/1".address')
# IPADDR2=$(juju status --format json am | jq -r '.applications.am.units."am/2".address')
#
# # Make sure traefik external-hostname is locally routable
# echo "$IPADDR cluster.local" | sudo tee -a /etc/hosts
#
# # Make sure charm code created web-config, cert and key files
# juju ssh --container alertmanager am/0 ls /etc/alertmanager/
#
# # Inspect server cert and confirm `X509v3 Subject Alternative Name` field is as expected
# echo | openssl s_client -showcerts -servername cluster.local -connect cluster.local 2>/dev/null | openssl x509 -inform pem -noout -text
#
# # Save CA cert locally
# juju show-unit am/0 --format yaml | yq '.am/0."relation-info"[0]."local-unit".data.ca' > /tmp/cacert.pem
#
# # Confirm traefik ingress has `https` for alertmanager's server url
# juju ssh --container traefik trfk/0 cat /opt/traefik/juju/juju_ingress_ingress_6_am.yaml
#
# # Confirm alertmanager TLS endpoint reachable
# curl --fail-with-body --capath /tmp --cacert /tmp/cacert.pem https://cluster.local/tlstest-am-0/-/ready

---
bundle: kubernetes
applications:
  am:
    charm: ./alertmanager-k8s_ubuntu-20.04-amd64.charm
    series: focal
    scale: 3
    trust: true
    resources:
      alertmanager-image: ubuntu/prometheus-alertmanager:0.23-22.04_beta
    options:
      web_external_url: https://alertmanager.local
  ca:
    charm: self-signed-certificates
    channel: edge
    scale: 1
  trfk:
    charm: traefik-k8s
    channel: edge
    scale: 1
    options:
      external_hostname: cluster.local
relations:
- [am:certificates, ca:certificates]
- [trfk:certificates, ca:certificates]
- [am:ingress, trfk]

tests/integration/test_tls_web.py Dismissed Show dismissed Hide dismissed
@sed-i sed-i marked this pull request as ready for review June 22, 2023 18:47
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/config_utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

It's too large to really pass with a fine comb, but what I see is good and ITWT (in testing we trust)

lib/charms/alertmanager_k8s/v0/alertmanager_dispatch.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
lib/charms/observability_libs/v0/cert_manager.py Outdated Show resolved Hide resolved
src/workload_manager.py Outdated Show resolved Hide resolved
src/workload_manager.py Outdated Show resolved Hide resolved
src/workload_manager.py Outdated Show resolved Hide resolved
src/alertmanager.py Outdated Show resolved Hide resolved
src/config_builder.py Outdated Show resolved Hide resolved
@sed-i sed-i merged commit 33f83c4 into main Jul 5, 2023
12 checks passed
@sed-i sed-i deleted the feature/tls branch July 5, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants