Skip to content

Conversation

@marun
Copy link

@marun marun commented Jun 29, 2021

This is a 4.7 followup to #714 to ensure upgrades from 4.7 to 4.8 are not disrupted by the lack of service ca configmaps before BoundServiceAccountTokenVolume is enabled by the 4.8 kube-apiserver.


A new controller is added to create a configmap per namespace that is annotated for service ca injection. The controller is derived from the controller that creates configmaps for the root ca.

This new controller is being added in 4.7 to ensure that the service ca configmaps will be present in all namespaces in the cluster before an upgrade to 4.8 is attempted. An upgrade to 4.8 will enable the BoundServiceAccountTokenVolume feature and all new pods will expect the configmaps to already be present.

A similar strategy was pursued upstream, in that publication of root ca configmaps was added in a release previous to enablement of BoundServiceAccountTokenVolume.

/cc @s-urbaniak @stlaz @sttts @soltysh

@openshift-ci openshift-ci bot requested review from s-urbaniak, soltysh, stlaz and sttts June 29, 2021 15:13
@openshift-ci-robot
Copy link

@marun: 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:

@openshift-ci openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Jun 29, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 29, 2021

@marun: This pull request references Bugzilla bug 1977383, which is valid. 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.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.z) matches configured target release for branch (4.7.z)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1946479 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 1946479 targets the "4.8.0" release, which is one of the valid target releases: 4.8.0
  • bug has dependents

Requesting review from QA contact:
/cc @wangke19

Details

In response to this:

Bug 1977383: Ensure service ca configmap is created in all namespaces

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 openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 29, 2021
@openshift-ci openshift-ci bot requested a review from wangke19 June 29, 2021 15:13
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2021
@marun marun changed the title Bug 1977383: Ensure service ca configmap is created in all namespaces Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Jun 29, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 29, 2021

@marun: This pull request references Bugzilla bug 1977383, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.z) matches configured target release for branch (4.7.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1946479 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 1946479 targets the "4.8.0" release, which is one of the valid target releases: 4.8.0
  • bug has dependents

Requesting review from QA contact:
/cc @wangke19

Details

In response to this:

Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces

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.

@marun marun changed the title Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces WIP Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Jun 29, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2021
@marun
Copy link
Author

marun commented Jun 29, 2021

Marking WIP until openshift/origin#26283 merges.

Copy link

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2021
@soltysh soltysh removed the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Jun 29, 2021
@s-urbaniak
Copy link

/retest

@marun marun changed the title WIP Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Jun 29, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 29, 2021

@marun: This pull request references Bugzilla bug 1977383, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.z) matches configured target release for branch (4.7.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1946479 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 1946479 targets the "4.8.0" release, which is one of the valid target releases: 4.8.0
  • bug has dependents

Requesting review from QA contact:
/cc @wangke19

Details

In response to this:

Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces

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-bot
Copy link

/retest

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

4 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@sdodson
Copy link
Member

sdodson commented Jul 2, 2021

/retest
openshift/origin#26303 has merged

@marun marun changed the title WIP Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces Jul 2, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 2, 2021

@marun: This pull request references Bugzilla bug 1977383, which is valid.

6 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.z) matches configured target release for branch (4.7.z)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
  • dependent bug Bugzilla bug 1946479 is in the state VERIFIED, which is one of the valid states (VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), CLOSED (CURRENTRELEASE))
  • dependent Bugzilla bug 1946479 targets the "4.8.0" release, which is one of the valid target releases: 4.8.0
  • bug has dependents

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces

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.

annotations := map[string]string{
// This annotation prompts the service ca operator to inject
// the service ca bundle into the configmap.
"service.beta.openshift.io/inject-cabundle": "true",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this injecting the same content contained in token secret's service-ca.crt? I remember seeing a bug that the content is different.

I want to see pods start properly during upgrades, but if we know that we're going to change the included content, it seems like we want to place the correct content in there to begin with.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the thread on slack, my understanding was that we do not want to maintain token secret parity. Holding the PR until this discussion is resolved one way or another.

@marun
Copy link
Author

marun commented Jul 2, 2021

/retest
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2021
@marun
Copy link
Author

marun commented Jul 3, 2021

metal ipi is definitely unrelated and likely points to a broken job.

@deads2k
Copy link

deads2k commented Jul 7, 2021

/lgtm
/hold cancel

This doesn't harm 4.7.z and if we decide to go this direction, merging early will assist.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, marun, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@sdodson sdodson added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 7, 2021
@sdodson
Copy link
Member

sdodson commented Jul 7, 2021

/override ci/prow/e2e-metal-ipi
Failed at infrastructure provisioning.

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-metal-ipi

Details

In response to this:

/override ci/prow/e2e-metal-ipi
Failed at infrastructure provisioning.

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.

@deads2k deads2k removed the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label Jul 7, 2021
@deads2k
Copy link

deads2k commented Jul 7, 2021

We have to eat this change forever.

@openshift-bot
Copy link

/retest

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

@marun
Copy link
Author

marun commented Jul 7, 2021

/retest

Not sure why we're still gating on the cmd job? iirc it's not known to pass reliably against master.

@marun
Copy link
Author

marun commented Jul 7, 2021

/retest

@sdodson
Copy link
Member

sdodson commented Jul 7, 2021

/override ci/prow/e2e-aws-downgrade
Flakey since July 1
/override ci/prow/e2e-aws-serial
No chance this is related and has passed on previous runs of same commit.
/override ci/prow/e2e-aws-jenkins
Passed previously on the same commit.

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aws-downgrade, ci/prow/e2e-aws-jenkins, ci/prow/e2e-aws-serial

Details

In response to this:

/override ci/prow/e2e-aws-downgrade
Flakey since July 1
/override ci/prow/e2e-aws-serial
No chance this is related and has passed on previous runs of same commit.
/override ci/prow/e2e-aws-jenkins
Passed previously on the same commit.

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-merge-robot openshift-merge-robot merged commit bd7b30d into openshift:release-4.7 Jul 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

@marun: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 1977383 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 1977383: [release-4.7] Ensure service ca configmap is created in all namespaces

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.

@sdodson
Copy link
Member

sdodson commented Jul 7, 2021

/bugzilla refresh

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

@sdodson: Bugzilla bug 1977383 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

Details

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.

@sdodson
Copy link
Member

sdodson commented Jul 7, 2021

/bugzilla refresh

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

@sdodson: All pull requests linked via external trackers have merged:

Bugzilla bug 1977383 has been moved to the MODIFIED state.

Details

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.

@marun marun deleted the publish-service-ca-configmaps branch July 7, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants