search

openshift / api

Canonical location of the OpenShift API definition.
http://www.openshift.org
Apache License 2.0
94 stars 516 forks source link

OCPBUGS-36479: remove duplicate featuregate 'ExternalRouteCertificate' #1959

Closed chiragkyal closed 3 months ago

chiragkyal commented 4 months ago

We're using RouteExternalCertificate feature gate as part of https://issues.redhat.com/browse/CFE-811 feature.

xref:

This PR removes the inadvertently duplicated ExternalRouteCertificate gate added in https://github.com/openshift/api/pull/1731

openshift-ci-robot commented 4 months ago

@chiragkyal: This pull request references Jira Issue OCPBUGS-36479, which is invalid:

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/api/pull/1959): > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 4 months ago

Hello @chiragkyal! Some important instructions when contributing to openshift/api: API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

chiragkyal commented 4 months ago

/assign @JoelSpeed

chiragkyal commented 4 months ago

/assign @Miciah

chiragkyal commented 4 months ago

/cc @lihongan

deads2k commented 4 months ago

/lgtm /approve

good for backport.

deads2k commented 4 months ago

verify failures looks real and look like they should be fixed.

JoelSpeed commented 4 months ago

Changes in the latest commit look ok to me, will see if @Miciah agress with the new list types

Miciah commented 4 months ago

Changes in the latest commit look ok to me, will see if @Miciah agress with the new list types

The changes make sense to me.

Just to make sure I understand correctly: If the cluster-ingress-operator and router use the client-go UpdateStatus method and controller-runtime Status().Update method, which use Put, does that mean that an update that changes one of these slice values will still replace the existing slice in whole, listType=map notwithstanding? I want to be sure that code like https://github.com/openshift/router/blob/a7313722c6e0541fcc00e92c459dd5d32a4a1534/pkg/router/controller/status.go#L268 and https://github.com/openshift/cluster-ingress-operator/blob/ddd1ee6dfb7e7c37d9525f48242baab55c7527fc/pkg/operator/controller/ingress/router_status.go#L113 will still work properly as long as we use Update and not Patch.

And in contrast, if someone uses oc patch to adjust weights on alternateBackends with listType=map, now the patch will not replace the slice in whole but will instead merge and retain slice entries that aren't listed in the patch, right?

JoelSpeed commented 4 months ago

I think yes? Your client should be setting a field manager name. If you push a status update that contains conditions, SSA assumes you are updating all of the conditions int the slice that belong to your field manager. So, if you in one request send 3 conditions, and in the next send 4, it will add that fourth to the list, and update the 3 in place. If you then send 2 conditions, it would assume you no longer want the other two, so would remove the 2 not sent, and update the 2 that were sent. If meanwhile, someone else added a condition with a different field manager name, your requests would not affect this condition, so it wouldn't update the whole slice no, it would only update those conditions within the slice, that are owned by your field manager name.

LIkewise for the backends, it depends on the field manager name owning each backend within the list. In most likelihood, I don't think you'd notice a difference as a kubectl user

lihongan commented 3 months ago

/jira refresh

openshift-ci-robot commented 3 months ago

@lihongan: This pull request references Jira Issue OCPBUGS-36479, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.17.0) matches configured target version for branch (4.17.0) * bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @lihongan

In response to [this](https://github.com/openshift/api/pull/1959#issuecomment-2244687526): >/jira refresh Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
lihongan commented 3 months ago

/label qe-approved

verified by pre-merge testing, the ExternalRouteCertificate has been removed

$ oc get clusterversion
NAME      VERSION                                                   AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.17.0-0.ci.test-2024-07-23-070357-ci-ln-gdmdmkk-latest   True        False         97m     Cluster version is 4.17.0-0.ci.test-2024-07-23-070357-ci-ln-gdmdmkk-latest

$ oc get featuregates.config.openshift.io cluster -oyaml | grep -i route
    - name: RouteExternalCertificate
openshift-ci-robot commented 3 months ago

@chiragkyal: This pull request references Jira Issue OCPBUGS-36479, which is valid.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.17.0) matches configured target version for branch (4.17.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @lihongan

In response to [this](https://github.com/openshift/api/pull/1959): >We're using `RouteExternalCertificate` feature gate as part of https://issues.redhat.com/browse/CFE-811 feature. > >xref: > >- Ingress Operator: https://github.com/openshift/cluster-ingress-operator/blob/b18a07c414696eb01170180d9dc8b5c221909d68/pkg/operator/operator.go#L134 >- o/openshift-apiserver: https://github.com/openshift/openshift-apiserver/blob/3c3cb886cb4621e7584156af68bdcf108c27cdaa/pkg/cmd/openshift-apiserver/cmd.go#L107 >- o/kubernetes: [https://github.com/openshift/kubernetes/blob/421e90ed9ef07c1a797386e662255dfb830a6[…]ver/admission/customresourcevalidation/route/validation_opts.go](https://github.com/openshift/kubernetes/blob/421e90ed9ef07c1a797386e662255dfb830a6b9f/openshift-kube-apiserver/admission/customresourcevalidation/route/validation_opts.go#L24) > >This PR removes the inadvertently duplicated `ExternalRouteCertificate` gate added in https://github.com/openshift/api/pull/1731 > > > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
lihongan commented 3 months ago

/retest

JoelSpeed commented 3 months ago

/lgtm

openshift-ci[bot] commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chiragkyal, deads2k, JoelSpeed

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: - ~~[OWNERS](https://github.com/openshift/api/blob/master/OWNERS)~~ [JoelSpeed,deads2k] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
chiragkyal commented 3 months ago

/jira refresh

openshift-ci-robot commented 3 months ago

@chiragkyal: This pull request references Jira Issue OCPBUGS-36479, which is valid.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.18.0) matches configured target version for branch (4.18.0) * bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @lihongan

In response to [this](https://github.com/openshift/api/pull/1959#issuecomment-2293246458): >/jira refresh Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 3 months ago

@chiragkyal: 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
ci/prow/e2e-azure 165518dbbc9fe1f327aae08c4dc5eec91ebf8993 link false /test e2e-azure

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-ci-robot commented 3 months ago

@chiragkyal: Jira Issue OCPBUGS-36479: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-36479 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/api/pull/1959): >We're using `RouteExternalCertificate` feature gate as part of https://issues.redhat.com/browse/CFE-811 feature. > >xref: > >- Ingress Operator: https://github.com/openshift/cluster-ingress-operator/blob/b18a07c414696eb01170180d9dc8b5c221909d68/pkg/operator/operator.go#L134 >- o/openshift-apiserver: https://github.com/openshift/openshift-apiserver/blob/3c3cb886cb4621e7584156af68bdcf108c27cdaa/pkg/cmd/openshift-apiserver/cmd.go#L107 >- o/kubernetes: [https://github.com/openshift/kubernetes/blob/421e90ed9ef07c1a797386e662255dfb830a6[…]ver/admission/customresourcevalidation/route/validation_opts.go](https://github.com/openshift/kubernetes/blob/421e90ed9ef07c1a797386e662255dfb830a6b9f/openshift-kube-apiserver/admission/customresourcevalidation/route/validation_opts.go#L24) > >This PR removes the inadvertently duplicated `ExternalRouteCertificate` gate added in https://github.com/openshift/api/pull/1731 > > > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
chiragkyal commented 3 months ago

/cherrypick release-4.17

openshift-cherrypick-robot commented 3 months ago

@chiragkyal: new pull request created: #2004

In response to [this](https://github.com/openshift/api/pull/1959#issuecomment-2293970824): > >/cherrypick release-4.17 > > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
chiragkyal commented 2 months ago

/cherrypick release-4.16

openshift-cherrypick-robot commented 2 months ago

@chiragkyal: new pull request created: #2007

In response to [this](https://github.com/openshift/api/pull/1959#issuecomment-2301469467): >/cherrypick release-4.16 Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.