[release-4.1] Bug 1732934: remove defaultservingcert description in apiserver config

[damemi]

Backporting to 4.1 for https://bugzilla.redhat.com/show_bug.cgi?id=1731105, this was fixed in master in #70

[openshift-ci-robot]

@damemi: This pull request references an invalid Bugzilla bug:

• expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, but it is ON_QA instead

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

In response to this:

[release-4.1] Bug 1731105: remove defaultservingcert description in apiserver config

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.

[eparis]

/bugzilla refresh

[openshift-ci-robot]

@eparis: This pull request references an invalid Bugzilla bug:

• expected the bug to target the "4.1.z" release, but it targets "4.2.0" instead
• expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, but it is MODIFIED instead

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

In response to this:

/bugzilla refresh

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-robot]

@damemi: This pull request references an invalid Bugzilla bug:

• expected dependent Bugzilla bug to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

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

In response to this:

[release-4.1] Bug 1732934: remove defaultservingcert description in apiserver config

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.

[damemi]

/bugzilla refresh

[openshift-ci-robot]

@damemi: This pull request references an invalid Bugzilla bug:

• expected dependent Bugzilla bug to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

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

In response to this:

/bugzilla refresh

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.

[damemi]

/bugzilla refresh

[openshift-ci-robot]

@damemi: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

[damemi]

/retest

[damemi]

/retest

[deads2k]

you need to pin all the other dependencies.

This is what we used to do in origin.

make WHAT=tools/depcheck/
_output/local/bin/linux/amd64/depcheck pin --glide.yaml=glide.yaml --glide.lock=glide.lock

[deads2k]

/lgtm

[jwforres]

I see two sets of API changes in this backport, the one referenced in the BZ, which is the defaultservingcert in the apiserver CRD.

However I also see changes to the infrastructure CRD. Where is the associated BZ for this change? Who is the owner for this particular CRD?

[damemi]

@jwforres you're right, though it seems odd that those changes to infrastructure in release-4.1 weren't previously carried to the release-4.1 branch of CCO. I think this may be a bug, and have opened https://bugzilla.redhat.com/show_bug.cgi?id=1734512 to track it (and additionally #82 to fix it). Note that pulling in one change and not the other requires pinning to the exact commit in that PR, rather than release-4.1 which would also include the API server changes in this pr.

@wking it looks like you contributed the relevant changes in this PR to the infrastructure config API in openshift/api#300, could you possibly comment on this as well?

[deads2k]

/lgtm

these changes look correct. Pinning to a particular SHA for one of our repos is strictly worse than pinning to the running branch.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, deads2k

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 [deads2k]

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

[wking]

@wking it looks like you contributed the relevant changes in this PR to the infrastructure config API in openshift/api#300, could you possibly comment on this as well?

@damemi, are you asking for a 4.1.z backport of openshift/installer#1930? Or did you want me to comment on something else?

[damemi]

@wking ah, I see, if these config options are used by the installer then we can't have them in 4.1 without the 4.1 installer also supporting them. Unfortunately, I don't see a way we can get this kubeapiserver fix (which we do want backported) without also getting the infrastructure changes. If that's the case then I think we will need a backport for the installer as well.

Still, it's weird that these installer-related API changes made it into release-4.1 of the API but not release-4.1 of the installer itself (or other components, as here). That seems like something we wouldn't want, right?

[wking]

if these config options are used by the installer then we can't have them in 4.1 without the 4.1 installer also supporting them. Unfortunately, I don't see a way we can get this kubeapiserver fix (which we do want backported) without also getting the infrastructure changes. If that's the case then I think we will need a backport for the installer as well.

Populating these fields for new 4.1 installs using some future version (e.g. 4.1.10) is possible. The issue is migrating 4.1.0 and other early clusters (where they were not populated) to fill the new fields in if folks want to be able to rely on them being populated. I'm not clear on how various consuming operators are being implemented; ideally they are looking for the new infrastructure fields and then falling back to the old, deprecated properties if the new fields are unset. But before you can ignore the deprecated properties, we'll need migration tooling to port the data from the old locations to the new locations. We have plans to manage that migration with the config operator, but it doesn't have a migration-operator deployed in-cluster now (in either master or 4.1.z), so it would be a bit of a lift to add it.

Can you go into more detail about the Kubernetes API-server backport? This particular PR has passed all its CI, and I guess fixes the explain issue. That isn't tied to installer/migration changes, is it? Are there other changes you're interested in backporting that are?

[damemi]

@wking Right, this fixes the oc explain issue I linked above. To fix that issue requires bumping the openshift/api dependencies here to the release-4.1 branch in order to pull in the previously cherry-picked apiserver struct change (openshift/api#375)

However prior to that cherry-pick going into 4.1, also merged were the changes to the infrastructure types I've pinged you on here (which I believe were just merged to master regularly before release-4.1 was cut, right?). Unfortunately those changes weren't propagated here to CCO in time, and now in order to get the changes I do want to backport also includes your changes.

So yes, the intent of this PR isn't tied to installer/migration changes but those infrastructure types have been caught in the crossfire and may need to be included as collateral changes here. As you mentioned this change has passed CI, so it looks clean, but I would appreciate your review/approval on the infrastructure changes (see c79d154) and also to make you aware of them being included.

[wking]

As you mentioned this change has passed CI, so it looks clean, but I would appreciate your review/approval on the infrastructure changes (see c79d154) and also to make you aware of them being included.

I don't see a problem with documenting the new fields in 4.1.z, even if the deprecation recommendation is a bit early without the migration logic in place. The new fields are not populated, but that's not a problem as long as folks continue to fall-back on cluster-config-v1. So I don't think we need to wait on installer support or migration logic before landing this doc change.

However, if other folks feel that the doc change is too misleading, I'm fine landing a 4.1.z API patch that adjusts the deprecation wording to mention the possible cluster-config-v1 fallback for the life of 4.1.z. And I'm personally fine with holding this doc bump until we have time to backport the installer changes to 4.1.z (easy) and create and backport a migration operator (more work, but we'll have to do the creation work eventually anyway), but that's really up to the z-stream patch managers. Am I missing something?

[damemi]

@wking thanks, given that this is ultimately just a documentation change I don't see this creating much of a problem either.

@jwforres with that explanation of the infrastructure changes you mentioned, is this good to sign-off?

[derekwaynecarr]

I think this is fine for 4.1.z.

Thanks for the explanation.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-ci-robot]

@damemi: All pull requests linked via external trackers have merged. The Bugzilla bug has been moved to the MODIFIED state.

In response to this:

[release-4.1] Bug 1732934: remove defaultservingcert description in apiserver config

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.