-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Provision contract fields for authz (contract update afterwards) #4065
[WIP] Provision contract fields for authz (contract update afterwards) #4065
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: creydr 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:
Approvers can indicate their approval by writing |
/cc @pierDipi objections on doing the contract update at the end of the reconcilation? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4065 +/- ##
============================================
- Coverage 52.88% 48.34% -4.55%
============================================
Files 343 244 -99
Lines 18013 14608 -3405
Branches 294 0 -294
============================================
- Hits 9526 7062 -2464
+ Misses 7603 6833 -770
+ Partials 884 713 -171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@creydr as far as I can tell from the code, we don't use the |
We use the audience and later the event policies from the status. So this "safes" us one reconciliation. But I might miss here something too... |
141dbd5
to
aa5791b
Compare
aa5791b
to
a4daa5d
Compare
@creydr: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
Closing as discussed with @pierDipi, as sources can start to send events to addressables as soon as they have an address (they don't check the ready condition), so we should set the addressable condition, only when the addressable is ready to accept events. |
Fixes #4041
Proposed Changes
Same as #4064, except that it refactors the Broker, KafkaChannel and KafkaSink reconcilers to do the contract update at the end of the reconcilation. This leads to having the .status fields which are part of the contract (e.g. OIDC audience or applying EventPolicies) are being updated directly in the contract.
Anyhow, the probing con only be done after the contract update, as otherwise the data-plane is not aware of the resource and this would lead to 404 on probing the resources endpoint. This has the side effect, that the resource will be marked as addressable, before the probe run (anyhow we have the
ProbeSucceeded
condition, which can render the resource as NotReady anyhow)