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

MCO-1273: OCB respects proxy configuration in Controller Config #4599

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RishabhSaini
Copy link
Contributor

@RishabhSaini RishabhSaini commented Sep 18, 2024

Closes: MCO-1273

- What I did
Added support for proxy configuration specified in the Controller Config to the buildah-build scripts

- How to verify it

  1. Add a cluster wide proxy like
oc get proxy/cluster -o yaml | yq
apiVersion: config.openshift.io/v1
kind: Proxy
metadata:
  creationTimestamp: "2024-09-19T13:45:39Z"
  generation: 1
  name: cluster
  resourceVersion: "546"
  uid: b9890313-569c-4083-8f1d-a97eb4bb42e3
spec:
  httpProxy: http://username:passwd@ip:port/
  httpsProxy: http://username:passwd@ip:port/
  trustedCA:
    name: ""
status:
  httpProxy: http://username:passwd@ip:port/
  httpsProxy: http://username:passwd@ip:port/
  noProxy: .cluster.local,localhost
  1. Add a network policy to only allow egress through the proxy

  2. Trigger a build by creating a MOSC (Containerfile can contain rpm-ostree install cowsay)

  3. MOSB succeeds

- Description for the changelog

OCB respects proxy configuration in Controller Config

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 18, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 18, 2024

@RishabhSaini: This pull request references MCO-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Closes: MCO-1273

- What I did

- How to verify it

- Description for the changelog

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 openshift-eng/jira-lifecycle-plugin repository.

@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 Sep 18, 2024
Copy link
Contributor

openshift-ci bot commented Sep 18, 2024

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-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 18, 2024

@RishabhSaini: This pull request references MCO-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Closes: MCO-1273

- What I did

- How to verify it

- Description for the changelog

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Sep 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RishabhSaini
Once this PR has been reviewed and has the lgtm label, please assign dkhater-redhat for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@RishabhSaini
Copy link
Contributor Author

/test all

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 18, 2024

@RishabhSaini: This pull request references MCO-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Closes: MCO-1273

- What I did
Added support for proxy configuration specified in the Controller Config to the buildah-build scripts

- How to verify it

- Description for the changelog

OCB respects proxy configuration in Controller Config

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 openshift-eng/jira-lifecycle-plugin repository.

@RishabhSaini
Copy link
Contributor Author

/test all

@RishabhSaini RishabhSaini marked this pull request as ready for review September 18, 2024 20:28
@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 Sep 18, 2024
@RishabhSaini
Copy link
Contributor Author

/test unit

@RishabhSaini
Copy link
Contributor Author

/test e2e-hypershift

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 19, 2024

@RishabhSaini: This pull request references MCO-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Closes: MCO-1273

- What I did
Added support for proxy configuration specified in the Controller Config to the buildah-build scripts

- How to verify it

  1. Add a cluster wide proxy like
oc get proxy/cluster -o yaml | yq
apiVersion: config.openshift.io/v1
kind: Proxy
metadata:
 creationTimestamp: "2024-09-19T13:45:39Z"
 generation: 1
 name: cluster
 resourceVersion: "546"
 uid: b9890313-569c-4083-8f1d-a97eb4bb42e3
spec:
 httpProxy: http://username:passwd@ip:port/
 httpsProxy: http://username:passwd@ip:port/
 trustedCA:
   name: ""
status:
 httpProxy: http://username:passwd@ip:port/
 httpsProxy: http://username:passwd@ip:port/
 noProxy: .cluster.local,localhost
  1. Add a network policy to only allow egress through the proxy

  2. Trigger a build by creating a MOSC

  3. MOSB succeeds

- Description for the changelog

OCB respects proxy configuration in Controller Config

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 openshift-eng/jira-lifecycle-plugin repository.

@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Sep 19, 2024

While working on this issue I realized Sergio wrote:

it is not enough to use the proxy to build the image, but we need to use the /etc/pki/ca-trust/source/anchors/openshift-config-user-ca-bundle.crt file to be able to reach the yum repositories, since rpm-ostree will complain about an intermediate certificate (this one of the https proxy) being self-signed.

rpm-ostree install uses dnf that requires custom-ca-certificates when using a https proxy. These certs are present in the Proxy Custom Resource as a link to a config map. Hence the custom user certificate needs to be added to the /etc/pki/ca-trust/source/anchors/ so that dnf can successfully trust/use them. However, currently the ControllerConfig only contains ProxyStatus in its spec which does not contain any reference to the custom-cert config map.

Hence the solution I see is to have a Informer/Lister for the Proxy CR as a part of the Build Controller. It will have an Update/Add/DeleteFunc that tracks the provided TrustedCA. It gets the user defined certs from the linked ConfigMap, creates a file and then within the ContainerFile template COPY /tmp/user-defined.cert /etc/pki/ca-trust/source/anchors/openshift-config-user-ca-bundle.crt.

@yuqi-zhang
Copy link
Contributor

Just to clarify, this is the same "/etc/pki/ca-trust/source/anchors/openshift-config-user-ca-bundle.crt" file that we would write to disk, correct? If so, it should live in the controllerconfig, as a field called AdditionalTrustBundle.

If so we can probably just leverage that instead of having to add an additional configmap watcher. I am curious if:

  1. you can specify a proxy without having to provide a userCA (sounds like no?)
  2. whether that configmap reference can be to another cert instead

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 20, 2024

@RishabhSaini: This pull request references MCO-1273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Closes: MCO-1273

- What I did
Added support for proxy configuration specified in the Controller Config to the buildah-build scripts

- How to verify it

  1. Add a cluster wide proxy like
oc get proxy/cluster -o yaml | yq
apiVersion: config.openshift.io/v1
kind: Proxy
metadata:
 creationTimestamp: "2024-09-19T13:45:39Z"
 generation: 1
 name: cluster
 resourceVersion: "546"
 uid: b9890313-569c-4083-8f1d-a97eb4bb42e3
spec:
 httpProxy: http://username:passwd@ip:port/
 httpsProxy: http://username:passwd@ip:port/
 trustedCA:
   name: ""
status:
 httpProxy: http://username:passwd@ip:port/
 httpsProxy: http://username:passwd@ip:port/
 noProxy: .cluster.local,localhost
  1. Add a network policy to only allow egress through the proxy

  2. Trigger a build by creating a MOSC (Containerfile can contain rpm-ostree install cowsay)

  3. MOSB succeeds

- Description for the changelog

OCB respects proxy configuration in Controller Config

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 openshift-eng/jira-lifecycle-plugin repository.

@RishabhSaini
Copy link
Contributor Author

/test bootstrap-unit

@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Sep 25, 2024

What is the elegant way of adding a CA root certificate to the unprivileged Buildah Container?

The problem of respecting the proxy configuration in on-cluster builds here is two fold:

  1. When buildah tries to build the user defined containerfile passed through the MachineConfigOS to produce the new RHCOS image

The image-build container for the buildah pod, needs to contain the correct configuration for HTTP_PROXY, HTTPS_PROXY, NO_PROXY to be able to run the buildah bud. While doing this it will give a proxyconnect tcp: tls: failed to verify certificate: x509: certificate signed by unknown authority if the user-defined certs are not added to the /etc/pki/source/ca-trust. Hence to the image-build container we need to supply the proxy config and the user defined certs to the /etc/pki and be able to run the update-ca-trust at runtime. This will add the user certs to the /etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt and make the source trustable. This needs to happen in the buildah-build.sh script

  1. When in the user defined ContainerFile, rpm-ostree install userNeededRPM. Here too the proxy configs and the certs will be needed. A COPY certs.crt /etc/pki/source/ca-trust/anchor/certs.crt and RUN update-ca-trust should do the job here in the ContainerFile.template

The problem is in the first case where the update-ca-trust will not be able to run at runtime since it needs sudo privileges and the image-build container is unprivileged

system wide trust store
buildah-build: Add the mounted additional-trust-bundle to the build
context of buildah
build_controller: Stored Additional Trust Bundle as a ConfigMap
image_build_request: Add the Additional Trust Bundle Config Map as
a volume to the Buildah pod and mount it

When using a cluster wide Proxy Configuration, the trustedCA needs to be
added to the system wide trust store in /etc/pki/ca-trust/source, hence
enabling a use case where self-signed certificates are used to download
a package from a YUM repo. It will be successfully validated by DNF
using the proxy.
@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Sep 26, 2024

Within the Cluster Network Operator, exists logic within the ProxyConfig Controller to essentially merge the system trust bundles and proxy trust bundles into one ConfigMap with the name trusted-ca-bundle withing the namespace openshift-config-managed. And the recommended way to distributing the merged trusted bundle across the cluster is through the ConfigMap CA Injector.
It solves the above problem, by just creating an empty ConfigMap within the MCO Namespace with the appropriate labels config.openshift.io/inject-trusted-cabundle = true so the configmap_ca_injector controller can eventually populate our config map with the trusted bundle. We can then mount it in the Buildah Pod as a volume from a config map directly into /etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt

The question now remain is when should this config map with the labels be created so the Cluster Network Operator can help populate it.
If we resort to doing this within the Build Controller (with or without the ephemeral-build-object label key in the configMap) we might not leave enough time for the Cluster Network Operator to inject the Trusted CA Bundle or make changes
Hence my suggestion is to do it when syncing the ControllerConfig after it is ensured that a user-ca-bundle exists in the Proxy CR. The operator at this point can just make a trusted-ca-bundle within the MCO namespace, making it easy for any other MCO owned Controller to use (ex. Build Controller). It helps prevent any potential bugs with the trusted CA bundles not created within time for the Buildah Pod to consume.

@jlebon
Copy link
Member

jlebon commented Sep 27, 2024

The question now remain is when should this config map with the labels be created so the Cluster Network Operator can help populate it.

Hmm, is there a reason why we just wouldn't always want it (i.e. make it a baked manifest)? We technically don't need it if on-cluster layering isn't in use, but (1) we're moving towards always relying on OCL, and (2) it's harmless if it's not used.

@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Sep 27, 2024

@jlebon If the Manifest is baked in to the MCO Image, if the Proxy CR is edited to modify the cert, would the manifest be needed to change as well?

Another caveat that I have noticed:
The merged (proxy + system) trusted CA bundles that are injected by Cluster Network Operator, are in the format to be consumed only by the /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem and NOT by the /etc/pki/ca-trust/extracted/openssl/ca-bundle.trust.crt.
Since the trusted .trust.crt format requires certificates to contain the PEM headers which the ConfigMap CA Injector does not provide

ca-bundle.trust.crt
-----BEGIN TRUSTED CERTIFICATE-----
-----END TRUSTED CERTIFICATE-----

vs

tls-ca-bundle.pem
-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----

Hence using the ConfigMap CA Injector only allows modifying the tls-ca-bundle.pem and not ca-bundle.trust.crt. Both of which might be needed to contain the proxy root ca certs, when establishing an egress connection (buildah bud/push/rpm-ostree install)

This is a problem that update-ca-trust-unpriv solves due to COSA privileges

Copy link
Contributor

openshift-ci bot commented Sep 27, 2024

@RishabhSaini: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure-ovn-upgrade-out-of-change 621f441 link false /test e2e-azure-ovn-upgrade-out-of-change
ci/prow/e2e-gcp-op-techpreview 621f441 link false /test e2e-gcp-op-techpreview
ci/prow/e2e-vsphere-ovn-upi-zones 621f441 link false /test e2e-vsphere-ovn-upi-zones

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants