search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.6k stars 1.33k forks source link

Document that providers MUST validate references are in the same namespace #2318

Closed detiber closed 1 year ago

detiber commented 4 years ago

Goals

  1. Replace current uses of corev1.ObjectReference with more appropriate types

Non-Goals/Future Work

  1. 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

vincepri commented 4 years ago

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?

detiber commented 4 years ago

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.

enxebre commented 4 years ago

cross referencing PR https://github.com/kubernetes/kubernetes/pull/87459 to provide more context.

detiber commented 4 years ago

Also relevant PR to update the api conventions doc: https://github.com/kubernetes/community/pull/4705

vincepri commented 4 years ago

/kind cleanup /area api

vincepri commented 4 years ago

/kind cleanup

fabriziopandini commented 4 years ago

/cc

fejta-bot commented 4 years ago

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

detiber commented 4 years ago

/lifecycle frozen

vincepri commented 4 years ago

/milestone v0.4.0

detiber commented 4 years ago

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 commented 4 years ago

/assign

vincepri commented 4 years ago

@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

vincepri commented 4 years ago

/priority important-soon

srm09 commented 4 years ago

@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.

detiber commented 4 years ago

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

detiber commented 4 years ago

/assign @srm09

detiber commented 4 years ago

/lifecycle active

fabriziopandini commented 4 years ago

/area api /kind api-change

vincepri commented 3 years ago

/milestone Next

Moving the milestone for now, not sure if we'll have someone that can push this forward across CAPI + providers.

vincepri commented 3 years ago

/priority backlog

fejta-bot commented 3 years ago

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

vincepri commented 3 years ago

/lifecycle frozen

enxebre commented 3 years ago

@vincepri is this intended for v1beta1?

vincepri commented 3 years ago

Probably too late right now?

enxebre commented 3 years ago

sounds sensible to postpone it. Do we have a milestone for v1beta2 or is it "next" the most appropirate?

vincepri commented 3 years ago

Next seems good for now, once we want to prioritize for the next API release we can reprioritze this issue

vincepri commented 3 years ago

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:

/retitle Document that providers MUST validate references are in the same namespace /assign @devigned /milestone v1.1

sbueringer commented 2 years ago

/milestone next Given that we won't bump the apiVersion with v1.2 so we can't introduce a breaking API change

k8s-ci-robot commented 2 years ago

@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](https://github.com/kubernetes-sigs/cluster-api/issues/2318#issuecomment-1044978740): >/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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
sbueringer commented 2 years ago

/milestone Next

fabriziopandini commented 2 years ago

/triage accepted

the ask to improve documentation is still valid; the discussion about how to model reference is tracked in https://github.com/kubernetes-sigs/cluster-api/issues/6539 so I suggest dropping it from this thread

/help

k8s-ci-robot commented 2 years ago

@fabriziopandini: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/2318): >/triage accepted > >the original ask to improve documentation is still valid; the discussion about how to model reference is tracked in https://github.com/kubernetes-sigs/cluster-api/issues/6539 so I suggest to drop it from this thread > >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
sbueringer commented 2 years ago

Stupid question, but is this only about adding documentation that control plane providers should validate their machine template ref?

As far as I can tell no other bootstrap/ infra provider resource has to contain a ref according to the contract. Or do we also want to provide guidance for them in case they have ref's with ObjectReference in their types?

fabriziopandini commented 2 years ago

I will keep this generic, because providers can have other references in their types as well (a recent example is infra objects referencing to IPClaims)

fabriziopandini commented 1 year ago

/unassign @devigned

sbueringer commented 1 year ago

@fabriziopandini I think I would close this issue:

  1. It has been open forever and nobody is volunteering
  2. Does it really make sense to document: "If you have a ref in your API object that has to reference an object in the same namespace please add validation for it"? a) I assume that it could be totally valid for provider CRs to reference objects in different namespaces. b) Something generic like this applies basically to all API fields. "Please add validation if not all possible values are valid".
  3. Are provider authors actually finding that hint in our documentation?
fabriziopandini commented 1 year ago

/close as per the comment above. also, this is probably that will probably be part of the bigger discussion on reviewing how we model references

k8s-ci-robot commented 1 year ago

@fabriziopandini: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/cluster-api/issues/2318#issuecomment-1529711027): >/close >as per the comment above. >also, this is probably that will probably be part of the bigger discussion on reviewing how we model references Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.