Add path annotation to apibindings

[mjudeikis]

Summary

This adds path annotation to apibindings. APIbindings are accessible via VirtualWorkspaces. This means providers will be able to resolve the canonical path and cluster more easily.

- apiVersion: apis.kcp.io/v1alpha2
  kind: APIBinding
  metadata:
    annotations:
      kcp.io/cluster: hezfu00lhrvcdnls
      kcp.io/path: root:test

Caveat: there is a race condition with logicalcluster. But it resolves quite quickly due to the binding handshake, which is followed by the update. Not ideal, but the alternatives (the ones I could think of) were even more hacky.

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes #3673

Release Notes

Add kcp.io/path annotation to APIBindings

[mjudeikis]

moving to draft. some strange errors pop up. Investigating.

[mjudeikis]

/test all

[mjudeikis]

/retest

[gman0]

Another approach would be to:

1. Skip mutating/validating objects whose logical cluster resolves to NotFound:
   • The reasoning would be that it's probably not a job of this plugin to deny requests with missing LogicalCluster -- something else should catch and validate this if we really need such a check.
2. Modify the APIBinding admission plugin to add an empty kcp.io/path annotation on the object if it's missing during create/update.
   • The pathannotation admission plugin would then catch this, and update the path accordingly once the APIBinding's logical cluster is ready.

Maybe this divides the responsibilities a bit better? But it may be that I'm missing some knowledge about why it is done like that in the first place :P

[gman0]

Also, existing APIBindings are not updated right? Is that a problem?

[mjudeikis]

Another approach would be to:

1. Skip mutating/validating objects whose logical cluster resolves to NotFound:

   • The reasoning would be that it's probably not a job of this plugin to deny requests with missing LogicalCluster -- something else should catch and validate this if we really need such a check.

2. Modify the APIBinding admission plugin to add an empty kcp.io/path annotation on the object if it's missing during create/update.

   • The pathannotation admission plugin would then catch this, and update the path accordingly once the APIBinding's logical cluster is ready.

Maybe this divides the responsibilities a bit better? But it may be that I'm missing some knowledge about why it is done like that in the first place :P

So my code is attempting to do option 2:
But I didn't want to do an empty annotation as some of the other code relies on this, and empty is invalid. So skipping kinda looked like a better alternative.

And it still gets updated/added once a logical cluster is added and the next resync/ status update.

[gman0]

I agree 👍

/lgtm

[kcp-ci-bot]

LGTM label has been added.

Git tree hash: ce0a2111a22aca51e986c418f7d76cd7090bf820

[mjudeikis]

/retest

[gman0]

/lgtm

We know the limitations, but it's better than the alternatives. And the limitations don't seem to be very serious.

[kcp-ci-bot]

LGTM label has been added.

Git tree hash: 2ddcde036236088f0320e92a0d243108434f2132

[xrstf]

For my own understanding: For just resolving the cluster ID to the workspace path, one could (like the syncagent does) simply also claim logicalclusters, right?

/approve

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/admission/OWNERS [xrstf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[xrstf]

This can be done already if one claims LogicalCluster, but this elevated apibinding/export contract quite high.

Yeah you said that already in the ticket. Nevermind. ^^

[xrstf]

/retest

[olamilekan000]

/retest

[kcp-ci-bot]

@mjudeikis: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-multiple-runs	2448833	link	true	/test pull-kcp-test-e2e-multiple-runs

Full PR test history

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. I understand the commands that are listed here.