search

cloudfoundry / cf-k8s-networking

building a cloud foundry without gorouter....
Apache License 2.0
32 stars 17 forks source link

Add additional printer columns to Route CRD #49

Closed mike1808 closed 4 years ago

mike1808 commented 4 years ago

Summary of changes

Add additional printer columns to Route CRD so the App GUIDs and Route URL is displayed when kubectl get routes -owide is performed.

Related Issue(s)

Fixes #48

Additional Context

-Include any links to related PRs, stories, slack discussions, etc... that will help establish context. -Is there anything else of note that the reviewers should know about this change?

Acceptance Steps

  1. Map an app to a route
  2. Run kubectl get routes -n cf-workloads -owide
  3. Verify that Route name, URL, and age is displayed and match to the route.
cf-gitbot commented 4 years ago

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

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

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

mike1808 commented 4 years ago

This is work in progress for two reasons:

  1. No integration tests for this.
  2. But mainly, displaying App GUID doesn't look much helpful as it's truncated by kubectl as it's too long. What if for now I only display URL and in future, we extend Route CRD with optional app name so for Cluster operator, it will be easier to understand which apps are routes by the Route resources?
christianang commented 4 years ago

Having just the URL seems fine. I think your right that the App GUID isn't as helpful, but I am not sure if we would extend the Route CRD to include the App Name considering it would only be for display purposes. It feels like it adds additional complexity because we would have to make additional requests to Cloud Controller to get the App Name.

mike1808 commented 4 years ago

@christianang I thought Route CR is created by CAPI and they could put name without additional query. Also, I was comparing to cf command, which has both app names and URL.

I'll remove app GUIDs and will just "Number of apps" or something similar and create an integration test for this.

christianang commented 4 years ago

You would also have to keep the App Name up to date since they could technically change. Also I'm usually a fan of not having multiple sources to truth to a field.

tcdowney commented 4 years ago

You would also have to keep the App Name up to date since they could technically change. Also I'm usually a fan of not having multiple sources to truth to a field.

+1 to this whenever we do end up denormalizing mutable names of things in CF we end up regretting it. The app/space/org names on Diego metric tags here are one example. We felt at the time that the end-user benefit to having that information there outweighed any potential risk of it becoming stale, but users ended up using the name as the persistent identifier instead of the guid and it caused problems. 😞

Would prefer to avoid that here..