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.

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

Bug 1704874: Create ConfigMap for Registry CA #96

Merged
merged 1 commit into from May 2, 2019

Conversation

adambkaplan
Copy link
Contributor

@adambkaplan adambkaplan commented May 1, 2019

Create a ConfigMap that the openshift-controller-manager can use to hold the registry's CA.
Use the service.beta.openshift.io/inject-cabundle annotation to have the service-ca-operator dynamically inject the internal registry's certificate authority.

See https://bugzilla.redhat.com/show_bug.cgi?id=1704874

@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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 1, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 1, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2019
@adambkaplan
Copy link
Contributor Author

@bparees I think we'll want the operator to create the ConfigMap to hold the CA for the registry.
Placing it in the controller-manager's namespace seems more natural (at least I'm certain we won't run into RBAC issues accessing the ConfigMap)

@bparees
Copy link
Contributor

bparees commented May 1, 2019

@bparees I think we'll want the operator to create the ConfigMap to hold the CA for the registry.
Placing it in the controller-manager's namespace seems more natural

since it's something consumed by the controllers, that seems reasonable to me.

@adambkaplan
Copy link
Contributor Author

/retest

@adambkaplan adambkaplan changed the title [WIP] Bug 1704874: Create separate ConfigMap to hold registry CA for builds Bug 1704874: Create ConfigMap for Registry CA May 2, 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 May 2, 2019
@adambkaplan
Copy link
Contributor Author

/retest

adambkaplan added a commit to adambkaplan/origin that referenced this pull request May 2, 2019
With openshift/cluster-openshift-controller-manager-operator#96,
the openshift controller manager will be able to read the internal registry's
certificate authority from a central ConfigMap.
Updating build controller to copy the registry CA into each build's CA ConfigMap.
adambkaplan added a commit to adambkaplan/origin that referenced this pull request May 2, 2019
With openshift/cluster-openshift-controller-manager-operator#96,
the openshift controller manager will be able to read the internal registry's
certificate authority from a central ConfigMap.
Updating build controller to copy the registry CA into each build's CA ConfigMap.
@adambkaplan
Copy link
Contributor Author

ping @bparees

/cc @gabemontero @coreydaley

kind: ConfigMap
apiVersion: v1
metadata:
name: openshift-registry-ca
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the openshift-service-ca, it's not registry specific.

// Otherwise ignore the contents of the ConfigMap
modified := resourcemerge.BoolPtr(false)
existingCopy := existing.DeepCopy()
resourcemerge.EnsureObjectMeta(modified, &existingCopy.ObjectMeta, configMap.ObjectMeta)
Copy link
Contributor

Choose a reason for hiding this comment

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

how are you checking whether the service-ca value within the configmap has changed?

in our other configmaps we store a hash of the keys/values so we know if something has changed that needs to force a rollout of the configmap consumer....

Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to just add that in manageOpenShiftControllerManagerConfigMap. In fact isn't that already watching a client-ca configmap? (maybe for a differnet ca)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we are watching the client-ca ConfigMap and the serving-cert secret.
We'll still need this function to create the openshift-service-ca ConfigMap and ensure that it has the inject-cabundle annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh. you do not need to watch the openshift-service-ca configmap from here because the controller-manager itself does not care if that configmap changes. only the buildcontroller logic inside the controllermanager, which already watches the configmap itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bparees renamed ConfigMap to openshift-service-ca - ptal

Create a ConfigMap that the openshift-controller-manager can use to hold the registry's CA.
Use the service.beta.openshift.io/inject-cabundle annotation to have the service-ca-operator
dynamically inject the internal registry's certificate authority.
@bparees
Copy link
Contributor

bparees commented May 2, 2019

/lgtm

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

[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

Needs approval from an approver in each of these files:
  • OWNERS [adambkaplan,bparees]

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 6ee3581 into openshift:master May 2, 2019
adambkaplan added a commit to adambkaplan/origin that referenced this pull request May 3, 2019
With openshift/cluster-openshift-controller-manager-operator#96,
the openshift controller manager will be able to read the internal registry's
certificate authority from a central ConfigMap.
Updating build controller to copy the registry CA into each build's CA ConfigMap.
deads2k pushed a commit to openshift/openshift-controller-manager that referenced this pull request Jun 18, 2019
With openshift/cluster-openshift-controller-manager-operator#96,
the openshift controller manager will be able to read the internal registry's
certificate authority from a central ConfigMap.
Updating build controller to copy the registry CA into each build's CA ConfigMap.
sanchezl pushed a commit to sanchezl/cluster-policy-controller that referenced this pull request Sep 18, 2019
With openshift/cluster-openshift-controller-manager-operator#96,
the openshift controller manager will be able to read the internal registry's
certificate authority from a central ConfigMap.
Updating build controller to copy the registry CA into each build's CA ConfigMap.
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. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants