NE-2376: Remove restriction of unmanaged x-k8s.io#1336
NE-2376: Remove restriction of unmanaged x-k8s.io#1336rikatz wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
@rikatz: This pull request references NE-2376 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold |
|
/test okd-scos-images |
|
/hold merge it after openshift/origin#30658 is merged and has a new release, otherwise this may break openshift/origin |
|
/retest |
|
/hold cancel |
|
Changes looks good to me, I've got only one question: should we consider adding a release note or documentation about this change? /approve |
|
/retest-required @davidesalerno we will add docs about it, it will be its own Jira. Also this will be backported from 4.19 to 4.21 |
|
/cc @rhamini3 How to test:
|
|
marking PR as verified as pre-merge is successful. the xlistener set CRD is successfully installed while the tlsRoutes CRD is blocked by the VAP |
|
/verified by @rhamini3 |
|
@rhamini3: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @rikatz re-adding, last commit was just a rebase over master |
|
@rikatz: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/assign |
|
/assign @davidesalerno @alebedev87 |
|
/retest |
|
/verified by @rikatz re-adding, last commit was just a rebase over master |
|
@rikatz: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
/unassign Letting Miciah give the final ok. |
| crd := o.(*apiextensionsv1.CustomResourceDefinition) | ||
| return crd.Spec.Group == gatewayapiv1.GroupName || crd.Spec.Group == experimentalGatewayAPIGroupName | ||
| crd, ok := o.(*apiextensionsv1.CustomResourceDefinition) | ||
| return ok && crd.Spec.Group == gatewayapiv1.GroupName |
There was a problem hiding this comment.
There has been some debate about whether the type assertion needs to guard against panic: #1165 (comment)
In any case, the operator is already inconsistent in this respect across the various controllers, and I don't consider it to be a major issue.
There was a problem hiding this comment.
yeah I am in favor of trying to be as defensive as possible. I've seen weird panics happening on weird places, and I prefer to have 2 extra lines instead of having some misbehavior :D
| olmEnabled: true, | ||
| existingObjects: []runtime.Object{ | ||
| co("ingress"), | ||
| crd("listenersets.gateway.networking.x-k8s.io"), |
There was a problem hiding this comment.
It would make sense to have a test case with listenersets.gateway.networking.k8s.io (without x-) to represent a real CRD name which the current version of the operator should flag as problematic. (Also, it would be satisfying when we do add support for listenersets to correct all the tests that say "this shouldn't work" to say instead "this should work"!) However, it it isn't a major issue.
There was a problem hiding this comment.
this is a unit test. We do not really really on the CRD name, but more on the group as defined on https://github.com/openshift/cluster-ingress-operator/pull/1336/files#diff-bea9a36dd02f12bbe121fcd437437e64ffde3aadaf50872f7d259b4b023fa8f9R263 as you have commented below :)
So being listenerset or not here would not change once we really support it.
There was a problem hiding this comment.
I don't understand. managedCRDMap does not have listenersets, so a unit test with listenersets.gateway.networking.k8s.io would expect {"unmanagedGatewayAPICRDNames":"listenersets.gateway.networking.k8s.io"} right now, right?
There was a problem hiding this comment.
Oh, I see what you're saying—the test's fake indexer doesn't actually use managedCRDMap as the real indexer does; instead, the fake indexer checks for an artificial group-name to determine whether or not the CRD is managed, and so while using listenersets.gateway.networking.k8s.io might make for a more realistic test case, it wouldn't actually improve test coverage given the fake indexer.
There was a problem hiding this comment.
Fair enough. Improving the test's fake indexer to be more realistic is out of scope for this PR.
| // Assume that all experimental CRDs are unmanaged. | ||
| if strings.Contains(o.GetName(), "gateway.networking.x-k8s.io") { | ||
| if strings.Contains(o.GetName(), "test.gateway.networking.k8s.io") { |
There was a problem hiding this comment.
I'm not sure the code comment makes sense anymore?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidesalerno, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Previously, cluster-ingress-operator would manage and block any updates to CRDs that were from group gateway.networking.x-k8s.io. After some analysis we identified that OSSM/Istio does filter these resources in case it does not have a flag to enable alpha APIs, which means it is safe to allow users to deploy these CRDs. This way, this change removes any restriction on x-k8s.io group, and removes any further reconciliation from CIO on x-k8s.io group.
|
/verified by @rikatz re-adding, last commit was just a rebase over master |
|
@rikatz: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
Thanks! Only change since my previous was a rebase and a fix to a code comment, so I see the comment-fix is in a separate commit; if you like, you should be able to use |
|
/label tide/merge-method-squash |
|
thanks. CI seems a bit flaky on other parts, I will keep an eye on it so we can get it merged :) |
|
/retest-required |
|
/test hypershift-e2e-aks |
1 similar comment
|
/test hypershift-e2e-aks |
|
@rikatz: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Previously, cluster-ingress-operator would manage and block any updates to CRDs that were from group gateway.networking.x-k8s.io.
After some analysis we identified that OSSM/Istio does filter these resources in case it does not have a flag to enable alpha APIs, which means it is safe to allow users to deploy these CRDs.
This way, this change removes any restriction on x-k8s.io group, and removes any further reconciliation from CIO on x-k8s.io group.