Skip to content

Implementing Proxy CA Bundle validation.#342

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
ricardomaraschini:proxy-ca-bundle-check
Aug 9, 2019
Merged

Implementing Proxy CA Bundle validation.#342
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
ricardomaraschini:proxy-ca-bundle-check

Conversation

@ricardomaraschini
Copy link
Contributor

@ricardomaraschini ricardomaraschini commented Jul 31, 2019

This patch adds a sidecar command line parameter to the registry operator. This sidecar will be deployed with the operator and will constantly monitor for changes on the mounted config map, restarting the operator every time the certificate content changes.

It is a big PR as it includes needed updates for the following packages:

  1. github.com/prometheus/client_golang
  2. k8s.io/apiserver
  3. k8s.io/kube-openapi
  4. sigs.k8s.io/structured-merge-diff

@openshift-ci-robot openshift-ci-robot added 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
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 31, 2019
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.

Good start here - there have been a few updates in the design that need to be factored in.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2019
@bparees
Copy link
Contributor

bparees commented Aug 1, 2019

fyi there's a canonical example of doing the "watch this file for changes and then restart the operator" here:
https://github.com/openshift/cluster-kube-controller-manager-operator/pull/254/commits

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2019
@ricardomaraschini ricardomaraschini changed the title WIP - Implementing Proxy CA Bundle validation. Implementing Proxy CA Bundle validation. Aug 5, 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 Aug 5, 2019
@ricardomaraschini
Copy link
Contributor Author

/assign @adambkaplan

@ricardomaraschini
Copy link
Contributor Author

/retest

@adambkaplan
Copy link
Contributor

@ricardomaraschini two items before this is ready for review/merge:

  1. Squash your commits with the exception of the "Update dependencies" commit.
  2. Rename the update dependencies commit "bump(*):", with a short note explaining why the deps need to be updated (ex: "pulling in watchdog from library-go"). Keep this commit separate from "real" code changes.

Adding github.com/openshift/library-go/pkg/operator/watchdog to our
vendor directory.

In order to use watchdog package we need to update the following other
packages to different versions:

- github.com/prometheus/client_golang
- k8s.io/apiserver
- k8s.io/kube-openapi
- sigs.k8s.io/structured-merge-diff

The versions were copied from openshift/cluster-kube-controller-manager-operator
repository as it already includes a watchdog implementation.
@ricardomaraschini
Copy link
Contributor Author

@adambkaplan Commits squashed/renamed as requested.

@ricardomaraschini
Copy link
Contributor Author

As for reference, the deployment of the sidecar will be made as:

      - image: cluster-image-registry-operator
        imagePullPolicy: IfNotPresent
        name: cluster-image-registry-operator-watch
        volumeMounts:
        - mountPath: /path/to/certificates
          name: trusted-ca
        args:
        - --namespace=$(POD_NAMESPACE)
        - --process-name=cluster-image-registry-operator
        - --termination-grace-period=30s
        - --files=/path/to/certificate.pem
        command:
        - cluster-image-registry-operator-watch
        - file-watcher-watchdog
        env:
        - name: POD_NAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.name
        - name: POD_NAMESPACE
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace

We also need to make sure both containers share the same process namespace by specifying shareProcessNamespace: true on the Deployment. This way watcher can see the operator and kill it when necessary.

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.

A few nits.

I'd also go ahead and add the sidecar container to the image registry operator. This should watch the file /usr/share/pki/ca-trust-source/ca-bundle.pem (or watch the entire directory if that is an option).

EDIT: this can be done in the manifest YAML manifests/07-operator.yaml

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.

/approve

I'm holding this until the key name for the CA trust bundle is finalized. Otherwise looks good!

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 6, 2019
@ricardomaraschini
Copy link
Contributor Author

/retest

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.

/hold cancel

We need to land this first. I'll then update #340 so that the operator and the file watcher share the trusted-ca mount.

@openshift-ci-robot openshift-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 8, 2019
This patch implements a command line argument to make registry operator
to work as a sidecar. This sidecar monitors for changes on the
filesystems and sends a kill signal to registry operator in case of
changes on local files.

As the watchdog package uses process name when locating the right
process to kill we are creating a link to have two names for the same
binary.

This patch also adds the Trusted CA bundle config map as a dependency
when deploying the operands.

As package watchdog has a helper for Cobra command this patch also
migrates the logic in main to leverage Cobra.
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 Aug 8, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2019
@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 96ef8b6 into openshift:master Aug 9, 2019
@wking
Copy link
Member

wking commented Aug 10, 2019

Upgrade passed in CI here, but this change seems to have broken ART upgrade tests in 4.2. I've hijacked rhbz#1727080 to cover this (the CVO doesn't do a good job bubbling the cause up, so this is almost certainly different from what that Bugzilla was originally about). You may want to revert until you have time to figure out and fix whatever's wrong with the ART nightlies.

@bparees
Copy link
Contributor

bparees commented Aug 10, 2019

It looks like the dockerfile change (which added the symlink) did not get sync'd to distgit. Will need to discuss w/ ART but i'll revert this for now to get us back to green.

@bparees
Copy link
Contributor

bparees commented Aug 11, 2019

@ricardomaraschini @adambkaplan this PR needed to update Dockerfile.rhel7 in addition to Dockerfile, that is why the ART release jobs (which use the .rhel7 file) were broken by this PR even though it passed CI (which, I guess, use the plain Dockerfile).

@danehans
Copy link

fyi @ricardomaraschini @adambkaplan I took a slightly different approach for adding trusted ca support to ingress operator: openshift/cluster-ingress-operator#334

/cc @bparees

@bparees
Copy link
Contributor

bparees commented Dec 13, 2019

fyi @ricardomaraschini @adambkaplan I took a slightly different approach for adding trusted ca support to ingress operator: openshift/cluster-ingress-operator#334

@danehans by which you mean you built a filewatcher into the operator process and have the operator terminate itself when the file changes (which will cause the pod to be restarted)? That seems fine to me.

@ricardomaraschini
Copy link
Contributor Author

@danehans I like it!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants