cf-for-k8s icon indicating copy to clipboard operation
cf-for-k8s copied to clipboard

Manifest annotations and labels should be applied to cf-workload pods

Open braunsonm opened this issue 5 years ago • 5 comments

Describe the bug

CF Manifests support annotations and labels. These should be translated to labels and annotations on the pods to allow greater customization.

For instance in our usecase we want to apply a certain policy using a podSelector. If we could deploy our apps with a label using the manifest, we could match on that label.

To Reproduce*

Steps to reproduce the behavior:

  1. Create a test app with this manifest:
---
applications:
  - name: go-webserver
    health-check-type: process
    instances: 1
    metadata:
      annotations:
        another: "hey"
      labels:
        test: "hello"
  1. cf push
  2. Notice that neither the annotation nor labels gets applied on the pod

braunsonm avatar Nov 20 '20 19:11 braunsonm

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

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

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

cf-gitbot avatar Nov 20 '20 19:11 cf-gitbot

@piyalibanerjee @matt-royal What do y'all think of this request?

Birdrock avatar Nov 23 '20 19:11 Birdrock

For a use-case, since cf-for-k8s doesn't support internal domains or C2C communication, we were going to have apps tag themselves as internal with a label. Then disable ingress access using a network policy matching that label.

---
kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: deny-app-ingress-internal-services
  namespace: cf-workloads
spec:
  podSelector:
    matchLabels:
      mydomain/internal: true
  policyTypes:
  - Ingress

This coupled with a Sidecar rule to allow C2C is currently the only way for CF apps to talk to eachother without going through the internet. We have some apps which do not have authentication on them and are meant for internal usage only.

braunsonm avatar Nov 23 '20 20:11 braunsonm

I just did a dive through the cloud_controller_ng and eirini code bases to understand what this would take. Adding support for annotations requires a small change to CCNG, but is already fully supported in eirini. Labels, on the other hand, would require changes to both components.

The technical work aside, this feature request does seem like it could have security implications. @tcdowney, @MasslessParticle, and I discussed this offline and here's a summary of the concerns and mitigations we came up with:

  • Concern: allowing users to insert arbitrary annotations seems risky, since annotations can trigger arbitrary behavior from k8s controllers. The fact that user workloads are in a different namespace from cf-system components may reduce the risk here.
    • potential mitigation: prefix all user-defined annotations (e.g. cloudfoundry.org/<user_annotation_key>). This may cause friction for existing tools, such as Prometheus, that expect specific annotation keys.
    • potential mitigation: add an allowlist to CCNG and only allow annotations that match.

matt-royal avatar Nov 23 '20 21:11 matt-royal

I do understand the use-case, but I also share the security concerns. I would recommend that this feature is guarded by explicit configuration, e.g. allow_application_annotations and allow_application_labels. The most convenient config API might be to allow a list of allowed keys (or key prefixes) and support * as an all-in option.

loewenstein avatar Nov 24 '20 07:11 loewenstein