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

Builds - Mount trusted CA for cluster proxies #12

Merged
merged 1 commit into from Aug 2, 2019
Merged

Builds - Mount trusted CA for cluster proxies #12

merged 1 commit into from Aug 2, 2019

Conversation

gabemontero
Copy link
Contributor

@gabemontero gabemontero commented Jul 31, 2019 •
edited

@openshift/openshift-team-developer-experience fyi

/assign @adambkaplan
/assign @bparees

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2019
@gabemontero
Copy link
Contributor Author

/test e2e-aws-builds

@gabemontero
Copy link
Contributor Author

but PTAL @adambkaplan @bparees even with WIP label

@gabemontero
Copy link
Contributor Author

hmmm .... can we not run e2e-aws-builds out of OCM ?

@gabemontero
Copy link
Contributor Author

we cannot: https://github.com/openshift/release/blob/master/ci-operator/config/openshift/openshift-controller-manager/openshift-openshift-controller-manager-master.yaml

I'll be submitting a openshift/release PR to fix that @adambkaplan @bparees

@gabemontero gabemontero mentioned this pull request Jul 31, 2019
Merged
@gabemontero
Copy link
Contributor Author

e2e-aws errors were oauth/router flakes

/test e2e-aws

adambkaplan
adambkaplan reviewed Jul 31, 2019
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

@gabemontero a few things came out of the arch call this AM:

  1. We should mount to /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
  2. We will always want to mount this since the new controller will be injecting the trust CA for the entire cluster, not just the proxy. Per @smarterclayton setting up the cluster CA may become an orthogonal configuration to proxies, as some customers want firm control over which CAs they trust.

/cc @bparees

pkg/build/controller/strategy/util.go Outdated Show resolved Hide resolved
pkg/build/controller/strategy/util.go Outdated Show resolved Hide resolved
pkg/build/controller/build/build_controller.go Outdated Show resolved Hide resolved
pkg/build/controller/build/build_controller.go Outdated Show resolved Hide resolved
@gabemontero
Copy link
Contributor Author

@adambkaplan I pushed some responses to your initial comments in a new commit

I deferred for now on the "always create the config map" point since it seemed like you were waiting for confirmation from @bparees

@gabemontero gabemontero changed the title WIP: Builds - Mount trusted CA for cluster proxies Builds - Mount trusted CA for cluster proxies Jul 31, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2019
@gabemontero
Copy link
Contributor Author

OK @adambkaplan @bparees always created the CA

relevant code snippets commented out pending arrival of injector controller

squashed commits

renamed ProxyCA to GlobalCA and the like

I have the one question on potential additional scaffolding that could be done based whether injection key names are known.

PTAL

@gabemontero
Copy link
Contributor Author

and the e2e-aws-builds jobs is getting triggered now

@adambkaplan
Copy link
Contributor

/approve

I'd like to see e2e-aws-builds succeed before staging this

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019
@gabemontero
Copy link
Contributor Author

e2e-aws-build passed

@gabemontero
Copy link
Contributor Author

/hold

temporarily until I get some of the inject key reference prototype in the commented out section

@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 Jul 31, 2019
@gabemontero
Copy link
Contributor Author

ok placeholder added in commented out section

/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 Jul 31, 2019
adambkaplan
adambkaplan approved these changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan 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 Jul 31, 2019
@gabemontero
Copy link
Contributor Author

terraform flake on e2e-aws

/test e2e-aws

@gabemontero
Copy link
Contributor Author

/hold

they renamed the annotation per @adambkaplan

@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 Jul 31, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2019
@gabemontero
Copy link
Contributor Author

/hold cancel

annotation updated @adambkaplan please re-lgtm

@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 Jul 31, 2019
@gabemontero
Copy link
Contributor Author

auth / mem related flakes e2e-aws-builds

/test e2e-aws-builds

@gabemontero
Copy link
Contributor Author

failed to acquire resource flakes discussed in forum-testplatform

/test e2e-aws-builds

@adambkaplan please re-post the lgtm thx

@gabemontero
Copy link
Contributor Author

e2e-aws-build avoided flakes ... this is ripe for the re posting the lgtm @adambkaplan

@adambkaplan
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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

@openshift-merge-robot openshift-merge-robot merged commit 80f039f into openshift:master Aug 2, 2019
@gabemontero gabemontero deleted the proxy-ca branch August 2, 2019 15:46
stlaz pushed a commit to stlaz/openshift-controller-manager that referenced this pull request Jul 29, 2021
@openshift-merge-robot
Merge pull request openshift#12 from deads2k/toleration
9722a8d 
tolerate any taint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan adambkaplan approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@soltysh soltysh Awaiting requested review from soltysh

@bparees bparees Awaiting requested review from bparees

Assignees

@adambkaplan adambkaplan

@bparees bparees

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gabemontero @adambkaplan @openshift-ci-robot @bparees @openshift-merge-robot