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

multiple hostnames are used to access the built-in docker-registry service #65

Open
stefannica opened this issue Apr 22, 2021 · 7 comments · Fixed by #171
Open

multiple hostnames are used to access the built-in docker-registry service #65

stefannica opened this issue Apr 22, 2021 · 7 comments · Fixed by #171
Labels
area/core bug Something isn't working
Milestone

Comments

@stefannica
Copy link
Member

stefannica commented Apr 22, 2021

The built-in docker-registry service needs to be accessed from several places simultaneously, and this is reflected in the URLs used for the container images that it stores:

  • the container runtime engines running on the k8s nodes in the same cluster. Currently, the registry service is exposed using a nodeport and can be accessed on the k8s nodes using the localhost and nodeport values: 127.0.0.1:30500
  • other containerized services running in the same cluster (e.g. tekton jobs building containers, the tekton operator itself). The hostname used in this case is the one provided by the k8s cluster DNS services, computed from the namespace name and the service name: registry.fuseml-registry
  • we may also need to open the docker-registry API for access outside the k8s cluster. One such case is working with one OCI registry across several k8s clusters, or synchronizing between multiple OCI registries managed by fuseml in multiple k8s clusters (i.e. inter-cluster replication).

This shows that our deployment setup is currently suffering from a multiple identity problem regarding the location of the OCI registry endpoint, and this already has some negative side-effects:

  • the most pressing problem is that in our workflow definitions we can't identify a container image using a single location, like we do with public container registries. The image location needs to be different depending on the consumer: a pipeline step that builds an image needs to use a different location than the step that uses that image.
  • in some cases, Tekton needs to access the OCI registry to inspect its entrypoint, but that is not possible if the nodeport hostname is used. An unsavory workaround is currently in place to fix this (see this comment for details).
  • another problem with using hard-coded nodeports is that the hard-coded nodeport value might not be available in the cluster.

Some ideas on how to fix this:

  1. use the k8s DNS name (registry.fuseml-registry) and cluster port on the k8s nodes instead of the localhost/nodeport, if possible
  2. expose the registry using an ingress. This might make the matter of generating matching certificates more complicated. Any post-installation changes made to the ingress hostname or domain name will also invalidate the configured pipelines.
  3. don't encode the registry hostname explicitly in the workflow and runnable definitions. Using a placeholder attribute value that says "this image comes from the built-in docker-registry" instead of pointing to it explicitly can help with the identity crysis.

NOTE: when this issue is fixed, the Tekton entrypoint workaround mentioned above should also be removed.

@stefannica stefannica added the bug Something isn't working label Apr 22, 2021
@stefannica stefannica added this to To do in Fuseml Repo via automation Apr 22, 2021
@flaviodsr
Copy link
Member

  1. don't encode the registry hostname explicitly in the workflow and runnable definitions. Using a placeholder attribute value that says "this image comes from the built-in docker-registry" instead of pointing to it explicitly can help with the identity crysis.

Just a note that while I am working on the workflows, I am not really explicitly setting the registry. As the image name is one of the inputs, the workflow expects the registry to be part of the image name, for example, for using the internal FuseML registry the image name should be registry.fuseml-registry/mlflow/trainer:v1

@stefannica stefannica added this to the MVP milestone Apr 23, 2021
@alfsuse alfsuse added the mvp label Apr 23, 2021
@stefannica stefannica added this to Backlog in FuseML Project Board via automation Apr 23, 2021
@stefannica stefannica moved this from Backlog to MVP in FuseML Project Board Apr 23, 2021
@stefannica
Copy link
Member Author

Moving out of MVP scope due to time constraints and the fact that this is not a must have for MVP. This issue is not functionality impairing and a workaround is already in place to address the Tekton related issue.

@stefannica stefannica removed the mvp label May 17, 2021
@stefannica stefannica moved this from MVP to Backlog in FuseML Project Board May 17, 2021
@stefannica
Copy link
Member Author

Moving to V2 for further investigation. If it turns out to be too difficult to implement, or it requires adding TLS support to part of the FuseML components, we should do it later.

@stefannica stefannica added the v2 label Jun 21, 2021
@stefannica stefannica moved this from Backlog to Current Iteration in FuseML Project Board Jun 21, 2021
@flaviodsr flaviodsr self-assigned this Jun 21, 2021
@stefannica
Copy link
Member Author

Adding notes from older issue regarding removal of Quarks from FuseML.

  • Quarks is used to generate certificate requests for CA certificates and automatically get them signed by the kubernetes API server. Only the quarks secret functionality is used (https://quarks.suse.dev/docs/quarks-secret/). This is something that can also be done manually through the use of kubernetes CertificateSigningRequest (more here: https://kubernetes.io/docs/reference/access-authn-authz/certificate-signing-requests/). Let's call these cluster CA certificates.
  • cluster CA certificates are used to further generate and sign server certificates for the docker-registry component. They need to be accepted by the container engine running on all kubernetes nodes, because the kubernetes cluster will interact directly with the docker registry (i.e. pull images from it), which is why it is critical that they be signed by the kubernetes API server. Apparently, the root CA certificate used by the kubernetes API to sign cluster CA certificates is already installed on all nodes, so all signed certificates are immediately valid across the entire cluster
  • cluster CA certificates also need to be installed on all containers and external machines that interact with the docker registry: tekton pipeline containers that build and publish container images for instance, but this is less critical than the previous point about instant validation by kubernetes nodes. The fact that they are cluster CA certificates is irrelevant for clients other than the container engines
  • removing quarks requires that we find another way to generate and maintain cluster CA certificates. Some thoughts on that:
    • we cannot generate self-signed CA certificates: they have to be manually installed on all kubernetes nodes
    • we could generate and sign cluster CA certificates directly in the helm chart. The helm chart needs to interact with the k8s API to both create CertificateSigningRequests and accept them, so that might be a problem.
    • the certificate-manager k8s service might provide the solution, if it supports generating cluster CA certificates (to be researched), but this is not as lightweight as simply using the k8s built-in CertificateSigningRequests feature. It creates a hard dependency from FuseML to certificate-manager and it complicates the installation process. On the other hand, we'll probably need to use certificate-manager or another external certificate management tool at some point anyway, to generate certificates for TLS support
    • using istio for certificate management is also an option. Furthermore, istio has some integration with the kubernetes CSR feature, albeit currently experimental (to be researched). More on that here: https://istio.io/latest/docs/tasks/security/cert-management/custom-ca-k8s/
    • as a last resort: there might be another kubernetes service or application out there that we can use that is not related to cloudfoundry and is concerned exclusively with managing cluster CA certificates (unlike quarks). This project looks promising (read the linked medium article for more info): https://github.com/Trendyol/k8s-webhook-certificator

Looking towards the future, we should try to take into account the multi-cluster use-case, where several clusters may consume images hosted in a single docker registry server. Generating cluster CA certificates only works in the cluster that is used to generates them.

@flaviodsr
Copy link
Member

flaviodsr commented Jul 1, 2021

This issue covers 2 different, but related, problems:

  1. Remove FuseML dependency from quarks

    I was able to get FuseML working without quarks by using Trow, although it should also work with docker registry as long as it is setup properly. The catch is: the registry is accessed via http by the kubernetes nodes and via https by the pods. By default docker/crictl considers any registry on localhost as insecure, so when, for example, pulling an image from localhost/127.0.0.1 it is done through http. That is the reason that the trainer step succeeds to start the pod using an image from the FuseML registry even when the node does not have the registry certificate. It is done via http.

    The assumption that a certificate signed by the cluster CA is trusted by the kubernetes nodes is not correct. To confirm that, I have configured an ingress gateway/virtual service with SSL passthrough and tried pulling an image from one of the kubernetes nodes using the istio gateway url, it failed with certificate signed by unknown authority. In that way, I am not really sure why the need to have that certificate signed by the cluster CA, I suspect that it might be the pods who mounts the cluster CA and consequently trusts any certificate signed by the cluster CA. For FuseML this would be needed in case kaniko did not have an option to not verify ssl.

    Some notes about quarks-secrets, this is what happens when a quarks CRD (signerType: cluster) is created:
    1. CFSSL is used to create a private key and a Certificate Signing Request [1]
    2. The private key is stored in a kubernetes secret (to be used later) and a kubernetes Certificate Signing Request is created with the Certificate Signing Request created in 1. [2]
    3. Once the kubernetes CSR is created, the operator CSR reconciler approves the kuberntes CSR [3]. It is important to note that, as the operator did not specify signerName the certificate is singed by kubernetes.io/legacy-unknown which has some implications
    4. Upon the CSR approval, the status.certificate field of the kubernetes CSR is populated with the signed certificate PEM data. With that, the reconciler proceeds by creating a secret that stores [4]:
    - certificate: the signed certificate provided by the kubernetes CSR
    - ca: the root CA (fetched from the service account token secret [5])
    - private_key: the private key created on step b.
    - is_ca: false
    5. Finally, the kubernetes CSR and the secret holding the private key created in step a. are deleted

    Besides the certificate signed by the kubernetes CA, FuseML also make use of quarks-secrets ability of maintaining a copy of its secrets/certificates across different namespaces.

  2. Use the same URL to reference the registry from the kubernetes nodes and pods

    This is a more complex issue, AFAIK the only way to make that possible is to expose the registry externally (via ingress, gateway, ...). However, this means that the kubernetes nodes needs to pull the image not from localhost anymore, forcing it to be performed through https. Consequently, unless the registry is using a trusted certificate, this is not possible to do with the requirement of not touching the kubernetes nodes (to install a custom CA when using self signed certificates).

  1. https://github.com/cloudfoundry-incubator/quarks-secret/blob/129013cd23f9578d3eb0a1612bd8ab2dc340adfc/pkg/credsgen/in_memory_generator/certificate.go#L42
  2. https://github.com/cloudfoundry-incubator/quarks-secret/blob/129013cd23f9578d3eb0a1612bd8ab2dc340adfc/pkg/kube/controllers/quarkssecret/certificates.go#L211
  3. https://github.com/cloudfoundry-incubator/quarks-secret/blob/129013cd23f9578d3eb0a1612bd8ab2dc340adfc/pkg/kube/controllers/quarkssecret/certificatesigningrequest_reconciler.go#L166
  4. https://github.com/cloudfoundry-incubator/quarks-secret/blob/129013cd23f9578d3eb0a1612bd8ab2dc340adfc/pkg/kube/controllers/quarkssecret/certificatesigningrequest_reconciler.go#L71
  5. https://github.com/cloudfoundry-incubator/quarks-secret/blob/129013cd23f9578d3eb0a1612bd8ab2dc340adfc/pkg/kube/controllers/quarkssecret/certificatesigningrequest_reconciler.go#L256

@flaviodsr flaviodsr moved this from Current Iteration to In progress in FuseML Project Board Jul 2, 2021
FuseML Project Board automation moved this from In progress to Done Jul 12, 2021
@flaviodsr
Copy link
Member

Reopening as Use the same URL to reference the registry from the kubernetes nodes and pods is not really fixed.

@flaviodsr flaviodsr reopened this Jul 12, 2021
FuseML Project Board automation moved this from Done to In progress Jul 12, 2021
@flaviodsr flaviodsr removed their assignment Jul 14, 2021
@stefannica
Copy link
Member Author

The double identity problem can only be solved when we add TLS support to FuseML and the services that are part of it, trow included. Even so, valid certificates will be required for the exposed endpoints, otherwise certificates will need to be installed on the k8s nodes manually. A mixed solution should also be investigated, where if valid certificates are not provided, the current approach is reused.

@stefannica stefannica moved this from In progress to Current Iteration in FuseML Project Board Jul 22, 2021
@stefannica stefannica moved this from Current Iteration to V2 in FuseML Project Board Jul 22, 2021
@stefannica stefannica moved this from V2 to Backlog in FuseML Project Board Jul 22, 2021
@stefannica stefannica moved this from Backlog to V3 in FuseML Project Board Sep 9, 2021
@stefannica stefannica removed the v2 label Sep 15, 2021
@stefannica stefannica moved this from V3 to Backlog in FuseML Project Board Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core bug Something isn't working
Projects
Fuseml Repo
  
To do
Development

Successfully merging a pull request may close this issue.

3 participants