Skip to content
This repository has been archived by the owner on Oct 20, 2023. It is now read-only.

Rename HTTPRoutes HTTPRoute #30

Merged

Conversation

nicholasjackson
Copy link
Collaborator

When creating a CRD for the HTTPRoutes this causes a pluralization problem for example...

---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:  
  name: httproutess.smi-spec.io
spec:  
  group: smi-spec.io
  version: v1alpha1
  scope: Namespaced 
  names:    
    kind: HTTPRoute
    shortNames: 
      - htr
    plural: httproutess
    singular: httproutes

as opposed to

---
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:  
  name: httproutes.smi-spec.io
spec:  
  group: smi-spec.io
  version: v1alpha1
  scope: Namespaced 
  names:    
    kind: HTTPRoute
    shortNames: 
      - htr
    plural: httproutes
    singular: httproute

@grampelberg
Copy link
Collaborator

Well that's a bummer. Is there a better word we can use? It is a container of multiple routes, so HTTPRoute isn't quite right. (I can be convinced that I'm bikeshedding right now).

Also, I'd assumed the group would be spec.smi-spec.io so that we can version the individual groups separately moving forward.

ps htr is the best short name ever.

@nicholasjackson
Copy link
Collaborator Author

I agree it is not great, I was struggling myself, how about HTTPRouteList?

You are right on the namespace, I was just trying to get the codegening to work with our CRD build, will update that tomorrow once we get a few things figured out.

@slack
Copy link
Collaborator

slack commented May 8, 2019

Hah, Nic, we are on the same page. Was staring at this issue and had:
HTTPRouteList, HTTPRouteMatcher, HTTPRouteGroup

@grampelberg
Copy link
Collaborator

grampelberg commented May 8, 2019

I'm a big +1 to HTTPRouteGroup. @slack is your super power naming things? HTTPRouteList WFM as well.

@msftclas
Copy link

msftclas commented May 8, 2019

CLA assistant check
All CLA requirements met.

@nicholasjackson
Copy link
Collaborator Author

Ok how is this?

Copy link
Collaborator

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

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

I like it, just one little comment/question, otherwise, merge away!

@@ -154,7 +156,8 @@ protected label.

A `ClusterIdentityBinding` grants access for a specific identity, originating in
a specific namespace, to a ClusterTrafficTarget associated with pods in any
namespace.
namespace. The ClusterTrafficTarget referenced by targetRef should always be in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both target and binding aren't namespaced (in my head at least).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha good catch, so the reason I added that is when we are building the CRD and are actually writing the code to read the TrafficTarget, the KubeAPI client requires the object namespace as a parameter.

Originally I added namespace to the TargetRef so that you could do the lookup, then reverted that as IdentityBindings would always be in the same location as TrafficTargets. However, that assumption does not apply for ClusterIdentityBinding.

I think we need to add namespace to the ref then?

---
kind: IdentityBinding
apiVersion: smi-spec.io/v1alpha1
metadata:
  name: account-specific
  namespace: default
subjects:
- kind: ServiceAccount
  name: service-b
  namespace: default
targetRef:
  kind: TrafficTarget
  name: foo 
  namespace: default

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like RoleBinding and ClusterRoleBinding don't need the namespace. I suspect that is because you can only targetRef the cluster or non-cluster resource. I think that targetRef is a custom object there though (and not a ObjectReference). You're in the code so I'll defer to you though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need namespace on everything, it will just make things easier when coding the CRDs no assumptions. I have added this in the latest commit. We can always change things later to make things easier.

@nicholasjackson nicholasjackson merged commit c5fcf1d into servicemeshinterface:master May 10, 2019
@nicholasjackson nicholasjackson deleted the rename_httproutes branch May 10, 2019 05:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants