Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

force scheduler insecure #80

Merged
merged 1 commit into from
Mar 28, 2019
Merged

force scheduler insecure #80

merged 1 commit into from
Mar 28, 2019

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 26, 2019

this will help land the rebase. The secure option fails to bootstrap.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 26, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Mar 26, 2019

/retest

@deads2k deads2k added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Mar 26, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019
@sjenning
Copy link
Contributor

fyi @smarterclayton @derekwaynecarr
why do we need to do this?
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Mar 26, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 26, 2019

fyi @smarterclayton @derekwaynecarr
why do we need to do this?
/hold

The rebase is blocked on this because in 1.13 our dynamic reload patch interacts badly with the cert generation change that no longer serializes to disk. This happens because the cert for this component is missing. This combination results in a case where kube-scheduler panics and dies. Choices we have to land the rebase

  1. operator fixes the missing cert by introducing a rotated cert to manage this
  2. operator fixes the missing cert by conditionally enabling secure serving based on availability of a generated serving cert.
  3. remove our dynamic reloading
  4. change a dynamic reloading path with an untestable scenario in 1.12 that attempts to allow illegal conditions and may still fail in the rebase
  5. hard code the rebase to ignore the operator flags and start insecure regardless
  6. revert upstream pulls that changed cert serialization
  7. Or what this pull does, simply serve insecurely for a week to land the rebase.

We tried to work around this in another pull, but that pull wasn't able to bootstrap the server.

@sjenning
Copy link
Contributor

@deads2k dynamic reload patch? what is this? link to source?

So there will need to be a follow-on to this to reenable secure connections after the rebase?

@deads2k
Copy link
Contributor Author

deads2k commented Mar 26, 2019

So there will need to be a follow-on to this to reenable secure connections after the rebase?

yes

@deads2k dynamic reload patch? what is this? link to source?

It's spread across 6-10 pulls. openshift/kubernetes@00438e8 is most of it in 1.13, but it's slightly different in 1.12

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2019
@sjenning
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 26, 2019
@soltysh soltysh mentioned this pull request Mar 27, 2019
Merged
@soltysh
Copy link
Member

soltysh commented Mar 27, 2019

/retest

@soltysh
Copy link
Member

soltysh commented Mar 27, 2019

@sjenning we tried to land #77 but --cert-dir is not a recognized flag by the kube-scheduler although it's wired there 🤷‍♂️ anyway, let's try to land this to unstuck the rebase.

soltysh
soltysh approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

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:

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

@soltysh soltysh mentioned this pull request Mar 27, 2019
Merged
@soltysh
Copy link
Member

soltysh commented Mar 27, 2019

/retest

4 similar comments
@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

ok, it's failin because https://openshift-gce-devel.appspot.com/build/origin-ci-test/logs/branch-ci-openshift-origin-master-images/665

@deads2k deads2k closed this Mar 27, 2019
@deads2k deads2k reopened this Mar 27, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/test unit
/test images

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Mar 27, 2019

I'm pretty sure this worked, but I'll wait for a cleaner run

@deads2k deads2k merged commit f8d432d into openshift:master Mar 28, 2019
@ravisantoshgudimetla
Copy link
Contributor

Yay, finally all tests passed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@ravisantoshgudimetla ravisantoshgudimetla Awaiting requested review from ravisantoshgudimetla

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@soltysh soltysh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@deads2k @sjenning @soltysh @openshift-ci-robot @ravisantoshgudimetla