UPSTREAM: <carry>: CFE-910: Update route external certificate validation#1659
UPSTREAM: <carry>: CFE-910: Update route external certificate validation#1659thejasn wants to merge 6 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
backports/unvalidated-commits
|
@thejasn: This pull request references CFE-910 which is a valid jira issue. 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 kubernetes/test-infra repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thejasn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
93372cc to
226300b
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
thejasn
changed the title
|
@thejasn: This pull request references CFE-910 which is a valid jira issue. 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 kubernetes/test-infra repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
go.mod
Outdated
openshift-ci
bot
added
the
do-not-merge/hold
p0lyn0mial
left a comment
p0lyn0mial
left a comment
There was a problem hiding this comment.
I added a few comments.
Please also check why all CI jobs failed.
There was a problem hiding this comment.
How were the feature flags set before?
There was a problem hiding this comment.
k8s apiserver never had the OCP feature flags being injected
There was a problem hiding this comment.
If routecommon.RouteValidationOptions is a struct then I think you will need a function.
There was a problem hiding this comment.
I think that you will need a function here as well.
There was a problem hiding this comment.
could you please add a unit test that would ensue that the getters can be injected?
There was a problem hiding this comment.
should we check if a.ValidationInterface also wants SetRESTClientConfig?
There was a problem hiding this comment.
I think that we should extend ObjectValidator to accept the ctx for all its methods.
type ObjectValidator interface {
ValidateCreate(obj runtime.Object) field.ErrorList
ValidateUpdate(obj runtime.Object, oldObj runtime.Object) field.ErrorList
ValidateStatusUpdate(obj runtime.Object, oldObj runtime.Object) field.ErrorList
}
There was a problem hiding this comment.
How hard would it be to move this initialisation to a separate setter ?
As we did for
There was a problem hiding this comment.
Same question here.
How hard would it be to move this initialisation to a separate setter ?
As we did for
There was a problem hiding this comment.
BTW: Perhaps this question should be asked elsewhere. But could you explain why we need the SAR client? It seems to me that we want to make sure if the user can change Route.Spec.Host, right? Do you happen to know why this was implemented in this particular way?
There was a problem hiding this comment.
The SAR client is specifically needed by the host update validation (https://github.com/openshift/library-go/blob/c91dd9756953b9b7ff3139565886954ed0cc33d3/pkg/route/hostassignment/assignment.go#L120). It is used to verify rbac on the custom-host sub resource. I am re-using the SAR client to also validate permissions needed for reading secrets (https://github.com/openshift/enhancements/blob/master/enhancements/ingress/route-secret-injection-for-external-certificate-management.md#implementation-detailsnotesconstraints-optional).
openshift-merge-robot
added
the
needs-rebase
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
226300b to
934347b
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@thejasn: This pull request references CFE-910 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 sub-task to target the "4.15.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 kubernetes/test-infra repository. |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
934347b to
1edf6c9
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
1edf6c9 to
c524d58
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
c524d58 to
40fcceb
Compare
Updates api to pull externalCertificate fields Updates library-go to pull new route validations hack/pin-dependency.sh github.com/openshift/api 2a3e8b481cec52b6815a4c4f678529c5070c9504 hack/pin-dependency.sh github.com/openshift/library-go=github.com/thejasn/library-go 90c9115d7ae3af3b87ffe3dafc9b285fd5688b1f hack/update-vendor.sh
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
40fcceb to
25188d7
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/cc @alebedev87 |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
25188d7 to
bc0dc6e
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
bc0dc6e to
39def98
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
…n args and opts Introduce new route validation wrapper to construct all deps and initialize validation options.
thejasn
force-pushed
the
feature/route-external-certificate-validation
branch
from
39def98 to
bc8dc97
Compare
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@thejasn: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@thejasn: The following tests failed, say
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/test-infra repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
the
lifecycle/stale
|
PR needs rebase. 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/test-infra repository. |
|
Are E2E tests covering the validation changes passing for MicroShift? Since none of the other flavors use the kube-apiserver admission plugin to validate routes, the presubmits give limited signal. |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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 kubernetes/test-infra repository. |
6 participants
Description
Updates route validations based on openshift/enhancements#1307 which introduces a new field in the route API
externalCertificatebehind the TP feature gate.Injects openshift feature gates into the custom validator for routes, similar to openshift/openshift-apiserver#382. Openshift feature gates were already being passed by the operator in openshift/cluster-kube-apiserver-operator#1485, this is a follow up for consuming the feature gates for route validation.
Follow up from: openshift/library-go#1549