SetClaimReference must pass to underlying object just claimRef allowed fields #552
Conversation
|
@pedjak Do you think it would be possible to switch to using a type without the extraneous fields? That seems ideal to me, since it would also update our claim OpenAPI schemas to better communicate that we don't actually pay any attention to all the extraneous fields (e.g. UID). In some places we've started defining our own types instead of ObjectReference, per #49. |
| Namespace: ref.Namespace, | ||
| Name: ref.Name, | ||
| } | ||
| _ = fieldpath.Pave(c.Object).SetValue("spec.claimRef", claimRef) |
There was a problem hiding this comment.
lgtm.
Just curious, why do we use a type if it is not supported?
There was a problem hiding this comment.
I also wonder if this method needs to live in crossplane-runtime at all, because it is only consumed in crossplane?
There was a problem hiding this comment.
That's a different question. What @negz and I mean is that corev1.ObjectReference is the wrong type. We should have a ClaimReference type instead that only has the fields that are allowed to be set.
There was a problem hiding this comment.
can you change this, i.e. introduce that type?
There was a problem hiding this comment.
I can, if we are ok with having breaking API change? @negz ?
There was a problem hiding this comment.
Does anything actually write (or read) the UID field in a claimRef today? If no, I feel fine making the change since while technically breaking at the API level it would functionally be a no-op.
Not sure if I got it right: would you like to change the signature of IMHO, it is quite alright to pass |
pedjak
force-pushed
the
fix-claimRef-set
branch
from
55a6e58 to
0b31516
Compare
Blast radius is minimal:
|
pedjak
force-pushed
the
fix-claimRef-set
branch
from
0b31516 to
b2bda45
Compare
…Reference `spec.claimRef` schema is just subset of corev1.ObjectReference, and hence `SetClaimReference` might get a reference that have more fields set, e.g. UID. The fields that do not exist in claimRef schema must be not set on the underlying object, otherwise K8s API server complains about non-existing field when client sends a patch request. * Introduced `claim.Reference` type that corresponds to `spec.claimRef` schema. * `composite.SetClaimReference` signature changed to accept an instance of `claim.Reference` * appropriate tests updated/new tests added Signed-off-by: Predrag Knezevic <[email protected]>
pedjak
force-pushed
the
fix-claimRef-set
branch
from
b2bda45 to
81b21d4
Compare
Description of your changes
spec.claimRefschema is just subset ofcorev1.ObjectReference, and henceSetClaimReferencemight get a reference that have more fields set, e.g. UID. The fields that do not exist in claimRef schema must be not set on the underlying object, otherwise K8s API server complains about non-existing field when client sends a patch request.claim.Referencetype that corresponds tospec.claimRefschema.composite.SetClaimReferencesignature changed to accept an instance ofclaim.ReferenceDiscovered while working on crossplane/crossplane#4047
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Adjusted unit test to catch regression.