Skip to content

Add Additional Trusted CAs and Registry CA to Build Pods #21614

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
Merged
openshift-merge-robot merged 1 commit into from
Jan 22, 2019
Merged

Add Additional Trusted CAs and Registry CA to Build Pods #21614

openshift-merge-robot merged 1 commit into from
Jan 22, 2019

Conversation

@adambkaplan
Copy link
Contributor

@adambkaplan adambkaplan commented Dec 4, 2018 •
edited
Loading

  • Enhance the build controller to watch and update the
    map of additional trusted certificate authorities copied
    into build pods.
  • Mount all trusted CAs in the map to new build pods for buildah
    under /etc/docker/certs.d
  • Mount service cert signing CA with the internal registry's domain

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 4, 2018
adambkaplan
adambkaplan commented Dec 4, 2018
View reviewed changes
Copy link
Contributor Author

@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.

/assign @bparees

May also need input from @openshift/sig-auth wrt. updating the trust store in the build pod.

pkg/build/controller/build/build_controller.go
AddFunc: c.configMapAdded,
UpdateFunc: c.configMapUpdated,
DeleteFunc: c.configMapDeleted,
})
Copy link
Contributor Author

@adambkaplan adambkaplan Dec 4, 2018

Choose a reason for hiding this comment

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

Watch Build.config.openshift.io and openshift-config ConfigMap resources.

pkg/build/controller/build/build_controller.go
}
bc.additionalTrustedCAData = caData
}
defer bc.controllerConfigQueue.ShutDown()
Copy link
Contributor Author

@adambkaplan adambkaplan Dec 4, 2018

Choose a reason for hiding this comment

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

Instead of mounting the additional trusted CA bundle, we populate the additional CAs map from a separate control loop.

@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 5, 2018

/retest

@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 5, 2018

/hold

Need to fix mounting in the build pod

@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 Dec 5, 2018
adambkaplan
adambkaplan commented Dec 5, 2018
View reviewed changes
@adambkaplan adambkaplan mentioned this pull request Dec 5, 2018
Merged
mrogers950
mrogers950 reviewed Dec 5, 2018
View reviewed changes
bparees
bparees reviewed Dec 5, 2018
View reviewed changes
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 6, 2018
@adambkaplan adambkaplan force-pushed the ca-mounting branch 2 times, most recently from 23ef169 to e75386f Compare December 6, 2018 20:31
@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 6, 2018

/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 Dec 6, 2018
@adambkaplan adambkaplan mentioned this pull request Dec 6, 2018
Merged
@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 6, 2018

@bparees @smarterclayton with openshift/builder#29, builds will be able to pull from/push to image streams. I think this is the last blocker to moving the extended build tests to 4.0 clusters.

@adambkaplan adambkaplan changed the title Watch Additional Trusted CAs from build.config CRD Add Additional Trusted CAs and Registry CA to Build Pods Dec 7, 2018
@sanchezl
Copy link
Contributor

sanchezl commented Dec 7, 2018

/test e2e-aws

@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 7, 2018

/hold

e2e-aws will fail until openshift/builder#31 lands

@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 Dec 7, 2018
@adambkaplan adambkaplan force-pushed the ca-mounting branch 2 times, most recently from d06eb84 to 2bac879 Compare December 14, 2018 16:06
@adambkaplan
Copy link
Contributor Author

adambkaplan commented Dec 14, 2018

/hold cancel

@bparees as soon as the builder image updates, this should pass tests 😃

@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 Dec 14, 2018
bparees
bparees requested changes Dec 14, 2018
View reviewed changes
pkg/cmd/openshift-controller-manager/controller/interfaces.go Outdated
},
},
KubernetesInformers: kexternalinformers.NewSharedInformerFactory(kubeClient, defaultInformerResyncPeriod),
OpenshiftConfigInformers: kexternalinformers.NewSharedInformerFactoryWithOptions(kubeClient, defaultInformerResyncPeriod, kexternalinformers.WithNamespace("openshift-config")),
Copy link
Contributor

@bparees bparees Dec 14, 2018

Choose a reason for hiding this comment

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

OpenShiftConfigKubernetesInformers (NamespaceClientTypeInformer)

pkg/build/controller/build/build_controller.go
}

func (bc *BuildController) configMapAdded(obj interface{}) {
configMap, ok := obj.(*corev1.ConfigMap)
Copy link
Contributor

@bparees bparees Dec 14, 2018

Choose a reason for hiding this comment

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

all these functions should be checking if event is for the configmap name we care about (whatever build config currently points to)

Copy link
Contributor Author

@adambkaplan adambkaplan Dec 14, 2018

Choose a reason for hiding this comment

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

@bparees updated by caching the ConfigMap name on the build controller. Better off pulling that data directly from the lister?

Copy link
Contributor

@bparees bparees Dec 14, 2018

Choose a reason for hiding this comment

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

caching it on the controller seems fine because the controller is going to get restarted if it ever changes, right?

Copy link
Contributor

@bparees bparees Dec 15, 2018

Choose a reason for hiding this comment

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

I don't think it will restart - the controller manager's operator doesn't watch this ConfigMap.

well the configmap is moot, this is about the configmap reference field in the build config cluster resource, but you're right, i keep forgetting we redesigned this.

But basically the build controller needs to be watching the build config resource so that when things like the AdditionalCAConfigMapNameRef field is changed, the controller sees the name change. The build controller can store that value whenever it sees it change, it doesn't need to get it from anywhere.

It's possible i'm confused about what you were originally asking.

@adambkaplan
Copy link
Contributor Author

adambkaplan commented Jan 14, 2019

@bparees fixed conflicts.

@bparees
Copy link
Contributor

bparees commented Jan 14, 2019

/lgtm

@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2019
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2019

/retest

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

8 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 14, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 15, 2019

/retest

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

@adambkaplan
Copy link
Contributor Author

adambkaplan commented Jan 15, 2019

/hold

aws-builds consistently failing. Needs investigation.

@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 Jan 15, 2019
@coreydaley
Copy link

coreydaley commented Jan 21, 2019

/retest

@bparees
Copy link
Contributor

bparees commented Jan 21, 2019

/hold cancel

i think we've cleaned up the build failures, 🤞

@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 Jan 21, 2019
@coreydaley
Copy link

coreydaley commented Jan 22, 2019

Looks like there was a change in openshift/api/config/v1/types.go from ConfigMapReference to ConfigMapNameReference

@adambkaplan
Watch Additional Trusted CAs from build.config CRD
a92eec2 
* Enhance the build controller to watch and update the
map of additional trusted certificate authorities copied
into build pods.
* Mount all trusted CAs in the map to new build pods for buildah
under /etc/docker/certs.d
* Mount service cert signing CA with the internal registry's domain

JIRA-ID: [DEVEXP-154](https://jira.coreos.com/browse/DEVEXP-154)
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2019
@adambkaplan
Copy link
Contributor Author

adambkaplan commented Jan 22, 2019

@coreydaley @bparees rebased & fixed the test that used configv1.ConfigMapReference

@bparees
Copy link
Contributor

bparees commented Jan 22, 2019

/lgtm

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

openshift-ci-robot commented Jan 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 22, 2019 •
edited
Loading

@adambkaplan: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-gcp ffd3578 link /test e2e-gcp
ci/prow/e2e-gcp-serial ffd3578 link /test e2e-gcp-serial
ci/prow/e2e-gcp-builds ffd3578 link /test e2e-gcp-builds
ci/prow/launch-aws ffd3578 link /test launch-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@coreydaley
Copy link

coreydaley commented Jan 22, 2019

/retest

level=fatal msg="waiting for openshift-console URL: context deadline exceeded"

@openshift-merge-robot openshift-merge-robot merged commit 035bd49 into openshift:master Jan 22, 2019
@adambkaplan adambkaplan mentioned this pull request Feb 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bparees bparees bparees requested changes

@csrwng csrwng Awaiting requested review from csrwng

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

+1 more reviewer

@mrogers950 mrogers950 mrogers950 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@adambkaplan @sanchezl @bparees @openshift-bot @wking @coreydaley @openshift-ci-robot @mrogers950 @openshift-merge-robot