search

cloudfoundry / cf-for-k8s

The open source deployment manifest for Cloud Foundry on Kubernetes
Apache License 2.0
301 stars 115 forks source link

Generate the instance index env injector certificate #625

Closed danail-branekov closed 3 years ago

danail-branekov commented 3 years ago

WHAT is this change about?

With the next Eirini release we are dropping EiriniX as a framework for our instance index environment variable injector (the one that injects CF_INSTANCE). Instead we are managing the webhook configuration statically via a dedicated yaml in the eirini deployment yamls.

In order to adopt this change in CF4K8S needs to

This is what this PR does.

Please note that dropping EiriniX results into the webhook created by EiriniX (eirini-x-mutating-hook) to no longer work thus preventing statefulsets from creating pods (i.e. cf push would be broken). Therefore its MutatingWebHookConfiguration should be deleted as part of the cf-for-k8s upgrade process. Is there an automated step to achieve this?

Does this PR introduce a change to config/values.yml?

Yes

Acceptance Steps

  1. Deploy CF4K8S
  2. Push an application
  3. ssh onto the application pod via kubectl exec -n cf-workloads --container opi -it <application-pod-id> /bin/bash
  4. Once in the shell, verify that the CF_INSTANCE_INDEX env var is set via echo $CF_INSTANCE_INDEX

Tag your pair, your PM, and/or team

@cloudfoundry/eirini

Things to remember

cf-gitbot commented 3 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/176980332

The labels on this github issue will be updated when the story is started.

cf-rel-int-status-bot commented 3 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

jamespollard8 commented 3 years ago

Hey @danail-branekov, thanks for the PR and for including tests changes.

1) Why is the new certificate necessary? Could we just use Istio service mesh to handle mTLS?

2)

MutatingWebHookConfiguration should be deleted as part of the cf-for-k8s upgrade process. Is there an automated step to achieve this?

Unfortunately we're not aware of any automation options here, and we'd strongly prefer to implement this without a breaking change if possible.

Best, James and @jspawar

kieron-dev commented 3 years ago
  1. Why is the new certificate necessary? Could we just use Istio service mesh to handle mTLS?

The cert is for the cluster control plane -> webhook service communication. It actually plain TLS with no client authentication, which should be fine for just mutating yaml strings.

I think it's not possible for the control plane to successfully call to a service with istio wrapping. The control plane uses <service-name>.<namespace>.svc as the hostname in the request, and expects that to be included in the certificate. But when we last experimented with this leaving istio involved, we saw istio inserted some strange host names in its certificate, and the expected name was absent. So we turned off istio of the webhook service deployment and manually provided a certificate.

We believe other admission webhooks go for a similar approach, but create the hook and certificate dynamically, which has the downside of leaving permission to register admission webhooks in runtime. So we've gone for the safer approach of creating the hook as part of the deployment step.

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

cf-rel-int-status-bot commented 3 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

danail-branekov commented 3 years ago

We have updated the PR to make the webhook reuse the name of the EiriniX version (eirini-x-mutating-hook). This is the easiest way to address the Eirini bump issue described in the PR description. Also see this slack discussion for details.

matt-royal commented 3 years ago

Hi @danail-branekov. It looks like the unit tests are still failing due to config/eirini/set-instance-index-env-injector-webhook-ca.yml. It assumes that the eirini-x-mutating-hook will have a webhooks key, but it doesn't appear to. Please fix the failing test and then we'll merge.

gcapizzi commented 3 years ago

Closing in favour of #643.