kubernetes-sigs
Document that providers MUST validate references are in the same namespace
Goals
- Replace current uses of
corev1.ObjectReferencewith more appropriate types
Non-Goals/Future Work
- Modify other types used
User Story
As a consumer of Cluster API I would like to have fields that are currently using corev1.ObjectReference to use more appropriate data types so that it is clear which fields are used (all of them) vs having to guess which fields are in use.
Detailed Description
Upstream documentation discourages the use of corev1.ObjectReference: https://godoc.org/k8s.io/api/core/v1#ObjectReference. In order to align better with upstream convention we should adopt types that are more fitting for their purpose.
When the referenced type is constant and in the same namespace, we should likely use corev1.LocalObjectReference instead.
When we also need type information for the referenced resource and that resource is in the same namespace, we should likely use corev1.TypedLocalReference
If for some reason we need to cross namespaces with a reference, we should likely create our own type that is a subset of corev1.ObjectReference
To ease this transition, we should gracefully handle the conversions within the conversion webhooks.
Contract changes [optional]
Any type that is currently using corev1.ObjectReference would be updated to a more appropriate type.
Data model changes [optional]
Any type that is currently using corev1.ObjectReference would be updated to a more appropriate type.
/kind proposal
We really should just have one type with
// API version of the referent.
// +optional
APIVersion string `json:"apiVersion,omitempty" protobuf:"bytes,5,opt,name=apiVersion"`
// Kind of the referent.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
// +optional
Kind string `json:"kind,omitempty" protobuf:"bytes,1,opt,name=kind"`
// Namespace of the referent.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/
// +optional
Namespace string `json:"namespace,omitempty" protobuf:"bytes,2,opt,name=namespace"`
// Name of the referent.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
// +optional
Name string `json:"name,omitempty" protobuf:"bytes,3,opt,name=name"`
We can have it in our own types?
I'd rather avoid having a Namespace field where we do not allow cross-namespace references, otherwise it's just an extra thing we need to validate and adds potential for a poor user experience.
I would say the same thing for references that are known types (Secret, ConfigMap, etc), for similar reasons.
cross referencing PR https://github.com/kubernetes/kubernetes/pull/87459 to provide more context.
Also relevant PR to update the api conventions doc: https://github.com/kubernetes/community/pull/4705
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale
I'm happy to work on this for https://github.com/kubernetes-sigs/cluster-api/milestone/16, also happy to help work with this on other folks if anyone else is interested in contributing to this.
@detiber If you have time or would like to, @srm09 was interested in getting up to speed on this issue and the problems it solves
@detiber What would be a potential usecase of using LocalObjectReference over TypedLocalObjectReference. AFAIU, the latter type is a more stringent case of the former.
IMHO, we should always use the latter to guarantee the object being referred to.
In case anyone else is interested, @srm09 is leading discussion of this issue in the following doc: https://docs.google.com/document/d/1S4zP7k39R0FcKg_MGk4jNHsFwIre06iL2ySYlTSpOGk/edit?ts=5f8f2fe3
/milestone Next
Moving the milestone for now, not sure if we'll have someone that can push this forward across CAPI + providers.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale
sounds sensible to postpone it. Do we have a milestone for v1beta2 or is it "next" the most appropirate?
Next seems good for now, once we want to prioritize for the next API release we can reprioritze this issue
During Oct 22nd backlog grooming meeting we've discussed about replacing the object references with custom objects and opted for keeping corev1.ObjectReference for the time being. We can definitely reconsider once we're ready to talk about v1beta2, although there were a few concerns:
- A significant breaking change for little benefit
- corev1.ObjectRefence is used everywhere and there are a lot of utilities out there
/retitle Document that providers MUST validate references are in the same namespace /assign @devigned /milestone v1.1
/milestone next Given that we won't bump the apiVersion with v1.2 so we can't introduce a breaking API change
@sbueringer: The provided milestone is not valid for this repository. Milestones in this repository: [Next, v0.3, v0.4, v1.0, v1.1, v1.2]
Use /milestone clear to clear the milestone.
In response to this:
/milestone next Given that we won't bump the apiVersion with v1.2 so we can't introduce a breaking API change
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
