Document that providers MUST validate references are in the same namespace

[detiber]

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]

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]

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]

cross referencing PR kubernetes/kubernetes#87459 to provide more context.

[detiber]

Also relevant PR to update the api conventions doc: kubernetes/community#4705

[vincepri]

/kind cleanup
/area api

[vincepri]

/kind cleanup

[fabriziopandini]

/cc

[fejta-bot]

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]

/lifecycle frozen

[vincepri]

/milestone v0.4.0

[detiber]

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.

[sbueringer]

/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]

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

[sbueringer]

/milestone Next

[fabriziopandini]

/triage accepted

the ask to improve documentation is still valid; the discussion about how to model reference is tracked in #6539 so I suggest dropping it from this thread

/help

[k8s-ci-robot]

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

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

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:

/triage accepted

the original ask to improve documentation is still valid; the discussion about how to model reference is tracked in #6539 so I suggest to drop it from this thread

/help

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.

[sbueringer]

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]

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]

/unassign @devigned

[sbueringer]

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

/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]

@fabriziopandini: Closing this issue.

In response to this:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.