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

Fixes in jobs managing Kyma certificates #11678

Merged
merged 35 commits into from
Aug 20, 2021

Conversation

koala7659
Copy link
Contributor

@koala7659 koala7659 commented Jul 13, 2021

Proposed changes:

  • Remove cert sychronisation deployment
  • Rename istio application connector gateway credential secret from app-connector-certs to kyma-gateway-certs-cacert
  • Remove Connector Service App CA Certs Backup and Restore Jobs
  • Remove job connector-service-migrate-ac-certs
  • Add certificate migration functionality to Certs Setup Job and Compass Runtime Agent

@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 13, 2021
@koala7659 koala7659 changed the title Fixes in certificates jobs Fixes in jobs managing Kyma certificates Jul 13, 2021
@netlify
Copy link

netlify bot commented Jul 13, 2021

✔️ 🥰 Documentation preview ready! 🥰

🔨 Explore the source changes: 5b3dd99

🔍 Inspect the deploy log: https://app.netlify.com/sites/kyma-project-docs-preview/deploys/611ddb2b991c8400072fb99f

😎 Browse the preview: https://deploy-preview-11678--kyma-project-docs-preview.netlify.app

@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2021
Copy link
Contributor

@franpog859 franpog859 left a comment

Choose a reason for hiding this comment

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

I installed it using ./kyma.js install --skip-components kiali,tracing --with-central-application-gateway --use-helm-template command and tested it running mocha test/2-commerce-mock.js -b. All tests passed 🎉🎉🎉

@everesio
Copy link
Contributor

everesio commented Aug 3, 2021

lgtm

Copy link
Contributor

@franpog859 franpog859 left a comment

Choose a reason for hiding this comment

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

I also tested it on Gardener + Azure. I installed the previous version, generated the certificates, and checked if they work using a new Application and the v1/metadata/services Application Registry endpoint. I upgraded Kyma to this PR's version, checked if proper components' images are there, and checked the certificate again. After that, I rotated the TLS Gardener certificates using the CertificatesRevocation CR with the renewal: true property. I checked if the certificates are rotated and checked my certs again. Everything worked fine in this scenario 🎉🎉🎉

I'm not submitting the LGTM review as I know that there is more to be tested

@@ -39,8 +39,11 @@ compassRuntimeAgent:
namespace: compass-system
caCertificate:
secret:
name: app-connector-certs
name: kyma-gateway-certs-cacert

Choose a reason for hiding this comment

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

Why such name change? It implies that this is the main kyma-gateway certificate, while it's rather app-connector related cert. Isn't it?

Copy link
Contributor

@akgalwas akgalwas Aug 17, 2021

Choose a reason for hiding this comment

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

Some time ago Application Connectivity Certs Sync deployment was introduced. The responsibility of the deployment was copying TLS certificates to app-connector-certs secret. The implications were as follows:

  • the logic for creating app-connector-certs secret was implemented in Application Connectivity Certs Sync deployment and Certs Setup Job
  • the logic for setting app-connector-certs secret was in two places (some keys were set by Application Connectivity Certs Sync and some were set by Certs Setup Job)

The rename reduced an overall complexity a lot:

  • Application Connectivity Certs Sync deployment has been removed
  • Certs Setup Job is the only place where Application Connectivity Certs are managed
  • kyma-gateway-application-connector Gateway uses both kyma-gateway-certs and kyma-gateway-certs-cacert

This rename may be a bit misleading at the first glance, but it simplified Application Connector's certificate management.

Copy link
Member

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Aug 18, 2021

Choose a reason for hiding this comment

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

I can see you want to simplify the setup, but it looks like it's incorrect.
Some thoughts, please correct me if I am wrong.

  • Kyma Gateway is using credentialName: kyma-gateway-certs attribute
  • It points to a secret with key and TLS certificate used to handle HTTPS traffic to the cluster. It's a main "entry" for all external traffic (not related to application-connector)
  • Kyma Gateway TLS mode is SIMPLE - it does not support Mutual TLS at this moment
  • Configuring another secret with name kyma-gateway-certs-cacert is, according to Istio documentation, a way to provide Certificate necessary to validate client's certificates used in Mutual TLS.
  • So on one hand we're configuring the Kyma-Gateway to NOT use Mutual TLS (because it's mode is SIMPLE), on the other we're explicitly providing some configuration - a secret - that is a part of the Mutual TLS setup...
  • Istio may reject such configuration or treat that as mis-configuration and yield warnings, if it's not doing that at the moment
  • Istio experts and other people trying to debug problems on the cluster will be certainly confused by such configuration.

If you need Mutual TLS for application-connector gateway, it should have it's own secret with a name distinct from the secret used by Kyma-Gateway.
Unless I deeply misunderstand how Mutual TLS in Istio works (it may be the case...) I think that what is proposed here is a "hack" that works only by accident - Istio should complain about misconfiguration of Kyma-Gateway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to support @Tomasz-Smelcerz-SAP in this case.

We are now under construction of multiple domains. Each new domain, which is managed by client, requires a separate Gateway. As a result, the Gateway requires reference to the correct certificate for the domain. When clients are going to manage their domains with wildcards certificates there would always be a pair - Gateway and a Secret that comes from a Certificate. If something goes wrong with a network traffic it should be traceable which certificate fails. The approach that you introduce would suggest that generic Kyma-gateway has invalid cert while it is not true - it is your application connector. Whose responsibility would it be then to debug such problems? I have mixed feelings about this approach and wondered if there is some other way that can be taken instead.

Copy link
Contributor

@akgalwas akgalwas Aug 18, 2021

Choose a reason for hiding this comment

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

Some facts:

Let me to answer your concerns.

  • So one hand we're configuring the Kyma-Gateway to NOT use Mutual TLS (because it's mode is SIMPLE), on the other we're explicitly providing some configuration - a Certificate - that should be used in Mutual TLS setup...
  • Istio may reject such configuration or treat that as mis-configuration and yield warnings, if it's not doing that at the moment
  • Istio experts and other people trying to debug problems on the cluster will be certainly confused by such configuration.

Istio documentation describes credentialName as follows:

For gateways running on Kubernetes, the name of the secret that holds the TLS certs including the CA certificates. Applicable only on Kubernetes. The secret (of type generic) should contain the following keys and values: key: and cert: . For mutual TLS, cacert: can be provided in the same secret or a separate secret named -cacert. Secret of type tls for server certificates along with ca.crt key for CA certificates is also supported. Only one of server certificates and CA certificate or credentialName can be specified.

We have two gateways (kyma-gateway and kyma-gateway-application-connector) that share TLS certificates. I think that is ok.
As it comes to kyma-gateway, the type is SIMPLE so kyma-gateway-certs-cacert is ignored. I cannot find any information in the Istio documentation that proves this setup is incorrect.

Conclusions:

  • Application Connectivity Certs Sync deployment was a workaround for Istio issue and was removed to decrease complexity.
  • At this point I cannot see hard evidence against the proposed solution (well, I'm definitely not Istio expert, but the tests performed as a part of verification were successful)

Copy link
Contributor

@akgalwas akgalwas Aug 18, 2021

Choose a reason for hiding this comment

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

If something goes wrong with a network traffic it should be traceable which certificate fails. The approach that you introduce would suggest that generic Kyma-gateway has invalid cert while it is not true - it is your application connector. Whose responsibility would it be then to debug such problems? I have mixed feelings about this approach and wondered if there is some other way that can be taken instead.

@mjakobczyk I don't get your point.
We have the following case:

  • One gateway in SIMPLE mode, one gateway in MUTUAL mode
  • Both share server certificate

So if server (TLS) certificate is incorrect then all requests will fail (no matter if accessed resource is protected with TLS or mTLS ). It will be quite easy to observe as TLS handshake will fail. In case CA Cert is incorrect only requests that access mTLS protected resources will fail.

Copy link
Contributor Author

@koala7659 koala7659 Aug 18, 2021

Choose a reason for hiding this comment

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

I'm supporting @akgalwas here. Please notice following arguments:

  • Such configuration as proposed was present in Kyma for a long time. The only reason we have now this separate secret and deployment to synchronise certificates is a long gone fixed Istio bug. If this bug hadn't happened - nobody wouldn't have separated TLS credentials for both gateways into two secrets.

  • We never had separate TLS certs for different gateways. Kyma was designed to provide same (TLS) certificate to secure all its gateways. In the current solution it was implemented by having two secrets with SAME (TLS) certificate data manually synchronised by separate deployment. This is overhead. We can do it in much simpler way without these intermediate elements.

  • In the current and proposed solution both gateways end with loading SAME (TLS) certificates data to secure connection. In proposed solution an additional layer of security is provided by mTLS mode in app-connector-gateway where a separate mTLS certificate is loaded from additional secret kyma-gateway-certs-cacert which is exactly how it is supposed to work. This second secret will be ignored by standard kyma-gateway

  • From Istio point of view such configuration is fine. In fact it is not recommended for WWW endpoints. Using same (TLS) certificate to secure multiple gateways can cause 404 error for browsers using HTTP2 protocol
    This is a reason the when running istioctl analyze tool we can see the warning.
    (Gateway kyma-gateway-application-connector.kyma-system) Duplicate certificate in multiple gateways [kyma-system/kyma-gateway-application-connector kyma-system/kyma-gateway] may cause 404s if clients re-use HTTP2 connections.
    But we are sure that for our case (we do not use HTTP2 and we have no browser interaction on connector-gateway) both gateways will work fine.

  • If you find the naming confusing, please consider renaming secret kyma-gateway-certs into something more generic like kyma-tls-certs. In fact this SAME (TLS) certificate is used in both gateways.

  • Proposed solution is more explicit and easy to understand than the existing one. We use same (TLS) certificate for both gateways and this fact is now crystal clear for anyone reading istio configuration for gatways. The previous solution was very obscure and hard to grasp.

@koala7659
Copy link
Contributor Author

I have successfully tested those fixes in for KymaOS in following installation/upgrade scenarios:

  • global.applicationConnectorCa and global.applicationConnectorCaKey overrides provided during installation.
  • Overrides not provided during installation.
  • global.applicationConnectorCa and global.applicationConnectorCaKey overrides provided during upgrade. Upgrading to the PR should use provided certificates.
  • Overrides not provided during upgrade. Upgrading to the PR should preserve existing certificates.

Tested environments:

GKE (with own domain)
AKS (with own domain)

@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 19, 2021
@kyma-bot
Copy link
Contributor

kyma-bot commented Aug 19, 2021

@koala7659: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
pre-kyma-components-telemetry-operator 336baf6 link /test pre-kyma-components-telemetry-operator
pre-main-kyma-gardener-gcp-eventing 51175c4 link /test pre-main-kyma-gardener-gcp-eventing
pre-main-kyma-gardener-gcp-api-gateway 51175c4 link /test pre-main-kyma-gardener-gcp-api-gateway

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@akgalwas
Copy link
Contributor

/test pre-main-kyma-gke-upgrade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controller for handling Application Connector certificates