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: enable certificates relation on istio-pilot #338

Merged
merged 9 commits into from
Oct 17, 2023

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Sep 28, 2023

Enabling the certificates relation enables the integration with tls-certificates-operator and other charms that provide CA certificates. These CA certificates are used by istio-pilot to configure TLS on the ingress Gateway resource.
In the past this was done exclusively via charm configuration, which is now removed in favour of CA certificates provided through the certificates relation.

Manual testing

  1. Deploy the following:
juju deploy istio-gateway istio-ingressgateway --trust --config kind=ingress --channel latest/edge
juju deploy self-signed-certificates --channel edge
  1. In parallel, build istio-pilot from this PR's branch
  2. Deploy istio-pilot and relate
juju deploy ./istio-pilot*.charm --trust
juju relate istio-pilot istio-ingressgateway
juju relate istio-pilot:certificates self-signed-certificates:certificates
  1. Get the Gateway (kubectl get Gateway <name of default-gateway> -n<namespace> -oyaml) and Secret (secret name should be -gateway-secret) to see the certificate in place. The Gateway should show this:
    port:
      name: https
      number: 8443
      protocol: HTTPS
    tls:
      mode: SIMPLE

The Secret should have a cert and key in a long encoded string.

  1. Additionally you can curl the ingressgateway (curl -v IP.nio.io usually) to see TLS in action

TODO:

  • Increase test coverage
  • Fix linting issues
  • Provide manual testing instructions

@DnPlas DnPlas force-pushed the KF-4415-relate-tls-cert-operator branch from 7a87bd4 to 8037c86 Compare September 28, 2023 19:30
@DnPlas DnPlas force-pushed the KF-4415-relate-tls-cert-operator branch from 5c93997 to 70d92bd Compare October 2, 2023 09:09
@DnPlas DnPlas force-pushed the KF-4415-relate-tls-cert-operator branch 3 times, most recently from ba782d0 to df30ab1 Compare October 2, 2023 20:15
@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 2, 2023

CI failures should be fixed by #341

@DnPlas DnPlas marked this pull request as ready for review October 2, 2023 20:22
@DnPlas DnPlas requested a review from a team as a code owner October 2, 2023 20:22
@DnPlas DnPlas force-pushed the KF-4415-relate-tls-cert-operator branch from 5948695 to 4875352 Compare October 4, 2023 14:43
Enabling the certificates relation enables the integration with tls-certificates-operator
and other charms that provide CA certificates. These CA certificates are used by istio-pilot
to configure TLS on the ingress Gateway resource.
In the past this was done exclusively via charm configuration, which is now removed in favour
of CA certificates provided through the certificates relation.
@DnPlas DnPlas force-pushed the KF-4415-relate-tls-cert-operator branch from 4875352 to ec8289c Compare October 6, 2023 08:14
@kimwnasptd
Copy link
Contributor

What if a user accidentally sets the ingressgateway's Service to be nodeport, instead of loadbalancer?

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.

thanks @DnPlas
left small comments and I think a good addition would be an integration test that deploys self-signed-certificates or any example provider, and makes sure to reach the ingressgateway with TLS.

charms/istio-pilot/charmcraft.yaml Show resolved Hide resolved
charms/istio-pilot/charmcraft.yaml 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
@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 10, 2023

What if a user accidentally sets the ingressgateway's Service to be nodeport, instead of loadbalancer?

Well, the istio-pilot charm is smart enough to handle these scenarios through the _get_gateway_service method. Is there anything that worries you?

@orfeas-k
Copy link
Contributor

Tried the manual testing steps and confirm the following output

*   Trying 10.64.140.43:443...
* Connected to 10.64.140.43.nip.io (10.64.140.43) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: /etc/ssl/certs
* TLSv1.0 (OUT), TLS header, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS header, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS header, Finished (20):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (OUT), TLS header, Unknown (21):
* TLSv1.3 (OUT), TLS alert, unknown CA (560):
* SSL certificate problem: unable to get local issuer certificate
* Closing connection 0
curl: (60) SSL certificate problem: unable to get local issuer certificate
More details here: https://curl.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.

@DnPlas
Copy link
Contributor Author

DnPlas commented Oct 13, 2023

Tried the manual testing steps and confirm the following output

*   Trying 10.64.140.43:443...
* Connected to 10.64.140.43.nip.io (10.64.140.43) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: /etc/ssl/certs
* TLSv1.0 (OUT), TLS header, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS header, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS header, Finished (20):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (OUT), TLS header, Unknown (21):
* TLSv1.3 (OUT), TLS alert, unknown CA (560):
* SSL certificate problem: unable to get local issuer certificate
* Closing connection 0
curl: (60) SSL certificate problem: unable to get local issuer certificate
More details here: https://curl.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.

Thanks for confirming, ultimately, since this is a self signed certificate, it has to be loaded to your local trusted certificate location for your OS so that curl can establish the connection, but at least we see the TLS handshake, which helps us visualise the attempt.

tests/test_bundle.py Outdated Show resolved Hide resolved
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.

Changes LGTM, but the CI seems flaky. Let's merge once CI passes.

@DnPlas DnPlas merged commit e80dcc8 into main Oct 17, 2023
17 checks passed
@DnPlas DnPlas deleted the KF-4415-relate-tls-cert-operator branch October 17, 2023 15:20
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.

4 participants