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

CFE-755: Support cluster-wide egress proxy injection #103

Merged

Conversation

thejasn
Copy link
Contributor

@thejasn thejasn commented Feb 13, 2023

Description

This PR adds egress proxy injection support to the operator. Proxy environment variables will now be propagated to the operand from the operator deployment. This PR also provides an option to provide custom trusted CA certificates.

  • pkg/controller/deployment/cert_manager_cainjector_deployment.go, pkg/controller/deployment/cert_manager_controller_set.go, pkg/controller/deployment/cert_manager_controller_deployment.go and pkg/controller/deployment/cert_manager_webhook_deployment.go : Fix informers arg names and pass trusted CA configmap at runtime.
  • pkg/controller/deployment/deployment_overrides.go : Add 2 new deployment hooks to inject proxy env variables and inject trusted CA configmap as a volume.
  • pkg/controller/deployment/generic_deployment_controller.go : Add configmap informers to re-sync deployments (watch configmaps in the operand namespace).

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 13, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2023
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023
@thejasn thejasn force-pushed the feature/proxy-injection branch 2 times, most recently from 7b7beb2 to 6fe3276 Compare February 14, 2023 11:43
@thejasn thejasn changed the title WIP: Support proxy injection CFE-755: Support proxy injection Feb 14, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 14, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 14, 2023

@thejasn: This pull request references CFE-755 which is a valid jira issue.

In response to this:

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.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 15, 2023

@thejasn: This pull request references CFE-755 which is a valid jira issue.

In response to this:

Description

This PR adds egress proxy injection support to the operator. Proxy environment variables will now be propagated to the operand from the operator deployment. This PR also provides an option to provide custom trusted CA certificates.

  • pkg/controller/deployment/cert_manager_cainjector_deployment.go, pkg/controller/deployment/cert_manager_controller_set.go, pkg/controller/deployment/cert_manager_controller_deployment.go and pkg/controller/deployment/cert_manager_webhook_deployment.go : Fix informers arg names and pass trusted CA configmap at runtime.
  • pkg/controller/deployment/deployment_overrides.go : Add 2 new deployment hooks to inject proxy env variables and inject trusted CA configmap as a volume.
  • pkg/controller/deployment/generic_deployment_controller.go : Add configmap informers to re-sync deployments (watch configmaps in the operand namespace).

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.

@thejasn thejasn marked this pull request as ready for review February 15, 2023 09:21
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 15, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 15, 2023

@thejasn: all tests passed!

Full PR test history. Your PR dashboard.

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.

@thejasn
Copy link
Contributor Author

thejasn commented Feb 16, 2023

/cc @xingxingxia

@xingxingxia
Copy link
Contributor

xingxingxia commented Feb 16, 2023

I'm executing below to try pre-merge testing:

launch openshift/cert-manager-operator#103 aws,proxy

If stuck due to whatever, will label directly to unblock merge, and test after merge.
@thejasn , one suggestion: it is better to always have Dev peer's /lgtm label first, as openshift other components repos' PRs do, before qe test starts involved, in case later Dev peer review will introduce new code updates : )

@thejasn thejasn changed the title CFE-755: Support proxy injection CFE-755: Support cluster-wide egress proxy injection Feb 16, 2023
@xingxingxia
Copy link
Contributor

xingxingxia commented Feb 16, 2023

launch openshift/cert-manager-operator#103 aws,proxy failed due to unknown readon.
Then used this approach to pre-merge test: executed launch openshift/cert-manager-operator#103, got the env, pulled down the catalog image, pushed to my quay.io repo, then used QE's Jenkins Flexy-install job to install a disconnected env with https_proxy, and installed this catalog from my quay.io repo, then installed cert-manager-operator from it. Done pre-merge testing with this https_proxy env:

[xxia@2023-02-16 20:36:57 CST my]$ oc get po -n cert-manager-operator
NAME                                                        READY   STATUS    RESTARTS   AGE
cert-manager-operator-controller-manager-76cf859cb7-qwvr7   2/2     Running   0          111s
[xxia@2023-02-16 20:37:10 CST my]$ oc get po -n cert-manager-operator cert-manager-operator-controller-manager-76cf859cb7-qwvr7 -o yaml 
...
    env:
    - name: HTTP_PROXY
      value: http://<user>:<password>@10.0.152.122:3128
    - name: HTTPS_PROXY
      value: https://<user>:<password>@10.0.152.122:3130
    - name: NO_PROXY
      value: .cluster.local,.svc,10.0.0.0/16,10.128.0.0/14,127.0.0.1,172.30.0.0/16,api-int.<cluster-domain>,localhost,test.no-proxy.com

They are same as oc get proxy -o yaml.

[xxia@2023-02-16 20:35:04 CST my]$ oc get po -n cert-manager
NAME                                      READY   STATUS    RESTARTS   AGE
cert-manager-6f96c79f9b-mzn4v             1/1     Running   0          31s
cert-manager-cainjector-674cfc967-8qwdw   1/1     Running   0          67s
cert-manager-webhook-5fd545966f-2q6f6     1/1     Running   0          69s

[xxia@2023-02-16 20:40:35 CST my]$ for i in cert-manager-6f96c79f9b-mzn4v cert-manager-cainjector-674cfc967-8qwdw cert-manager-webhook-5fd545966f-2q6f6 ; do oc get po $i -n cert-manager -o yaml > pod-cluster-bot-pr103-$i.yaml ; done
[xxia@2023-02-16 20:40:43 CST my]$ vi -p pod-cluster-bot-pr103-cert-manager-*.yaml
...
    env:
    - name: HTTPS_PROXY
      value: https://<user>:<password>@10.0.152.122:3130
    - name: HTTP_PROXY
      value: http://<user>:<password>@10.0.152.122:3128
    - name: NO_PROXY
      value: .cluster.local,.svc,10.0.0.0/16,10.128.0.0/14,127.0.0.1,172.30.0.0/16,api-int.<cluster-domain>,localhost,test.no-proxy.com
...
    - name: http_proxy
      value: http://<user>:<password>@10.0.152.122:3128
    - name: https_proxy
      value: https://<user>:<password>@10.0.152.122:3130
    - name: no_proxy
      value: .cluster.local,.svc,10.0.0.0/16,10.128.0.0/14,127.0.0.1,172.30.0.0/16,api-int.<cluster-domain>,localhost,test.no-proxy.com

In all 3 pods, there are both upper case and lower case proxy env vars ^^

[xxia@2023-02-16 20:42:15 CST my]$ oc -n cert-manager-operator patch subscription cert-manager-operator --type='merge' -p '{"spec":{"config":{"env":[{"name":"TRUSTED_CA_CONFIGMAP_NAME","value":"trusted-ca"}]}}}'
[xxia@2023-02-16 20:42:58 CST my]$ oc get po -n cert-manager-operator
NAME                                                       READY   STATUS    RESTARTS   AGE
cert-manager-operator-controller-manager-7b4bc96d9-wmtft   2/2     Running   0          68s
[xxia@2023-02-16 20:44:19 CST my]$ oc get po -n cert-manager
NAME                                       READY   STATUS    RESTARTS   AGE
cert-manager-85b7988d98-8mqpx              1/1     Running   0          30s
cert-manager-cainjector-744c47c447-fhntr   1/1     Running   0          30s
cert-manager-webhook-54bbfd9797-z8k29      1/1     Running   0          30s
[xxia@2023-02-16 20:44:33 CST my]$ for i in cert-manager-85b7988d98-8mqpx cert-manager-cainjector-744c47c447-fhntr cert-manager-webhook-54bbfd9797-z8k29 ; do oc get po $i -n cert-manager -o yaml > pod-cluster-bot-pr103-after-ce-injection-$i.yaml ; done
[xxia@2023-02-16 20:45:22 CST my]$ vi -p pod-cluster-bot-pr103-after-ce-injection-cert-manager-*
  volumes:
  - configMap:
      defaultMode: 420
      name: trusted-ca
    name: trusted-ca
...
    volumeMounts:
    - mountPath: /etc/pki/tls/certs/cert-manager-tls-ca-bundle.crt
      name: trusted-ca
      subPath: ca-bundle.crt

In all 3 pods, there is proxy ca injected ^^

Then test functions of self-signed issuer, CA issuer, ACME http01 solver issuer with external server https://acme-v02.api.letsencrypt.org/directory which can't be accessed by cert-manager if the proxy stuff is not injected, and certificates, they work well.

So adding qe label:
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 16, 2023
@TrilokGeer
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thejasn, TrilokGeer

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

@thejasn
Copy link
Contributor Author

thejasn commented Feb 17, 2023

Fixes CM-37

@thejasn
Copy link
Contributor Author

thejasn commented Feb 17, 2023

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 17, 2023
@thejasn
Copy link
Contributor Author

thejasn commented Feb 17, 2023

/cc @davemulford

@davemulford
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 17, 2023
@openshift-merge-robot openshift-merge-robot merged commit bb98523 into openshift:master Feb 17, 2023
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. docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants