Add DataImportCron CronJobs Proxy support #2455

arnongilboa · 2022-10-30T14:47:05Z

Signed-off-by: Arnon Gilboa [email protected]

What this PR does / why we need it:
Use proxy certificates when creating DataImportCron CronJobs, so the pods won't fail on certificate issues when proxy is configured. cdi-source-update-poller was also fixed to use the proxy certificates.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Add DataImportCron CronJobs Proxy support

akalenyu

I think we will need to backport this, and I am pretty sure this is backport friendly, so that's awesome

akalenyu · 2022-10-30T14:51:31Z

pkg/controller/dataimportcron-controller.go

+		}
+		container.VolumeMounts = append(container.VolumeMounts, vm)
+		volumes = append(volumes, createConfigMapVolume(ProxyCertVolName, volName))
+	} else {


requeue on err?

why? nobody said you must have any of the Proxy params set. Similar logic to to the import controller.

akalenyu · 2022-10-30T14:55:01Z

pkg/controller/dataimportcron-controller.go

+	addEnvVarFromImportProxyConfig := func(varName string) {
+		if value, err := GetImportProxyConfig(cdiConfig, varName); err == nil {
+			addEnvVar(varName, value)
+		} else {


okay so we knowingly throw away the err from GetImportProxyConfig, just double checking we're okay with that

same answer as above.

akalenyu · 2022-10-30T14:58:57Z

tools/cdi-source-update-poller/main.go

-	digest, err := importer.GetImageDigest(url, accessKey, secretKey, certDir, insecureTLS)
+	allCertDir, err := importer.CreateCertificateDir(certDir)
+	if err != nil {
+		os.Exit(1)


maybe log the err so we can debug these (couple more of these down the file)
I see we already use log.Fatalf which prints + os.Exits in here so should be fine?

Sure, adding.

akalenyu · 2022-10-30T15:02:45Z

tools/cdi-source-update-poller/main.go

@@ -58,6 +64,8 @@ func main() {
 	if err != nil {
 		log.Fatalf("Failed BuildConfigFromFlags, kubeURL %s configPath %s: %v", kubeURL, configPath, err)
 	}
+	cfg.TLSClientConfig = rest.TLSClientConfig{Insecure: true}


hmm which change in your PR now requires this?
this is just about the cdi client that we use to get stuff (not the polling), why do we have to go insecure?

The CronJob containerized poller app runs in CDI namespace, and it tries to Get/Update() DataImportCron which is in another namespace, so when proxy is configured, Get/Update() fail on certificate issues unless we configure TLS appropriately. Maybe Insecure: true is not the best idea here, but I have no idea how to use the certs dir for setting TLSClientConfig. wdyt?

AFAIK, under the hood, it should respect proxy env vars so not sure what goes wrong

containerized-data-importer/vendor/k8s.io/client-go/rest/config.go

Lines 137 to 142 in 206c77a

// Proxy is the proxy func to be used for all requests made by this

// transport. If Proxy is nil, http.ProxyFromEnvironment is used. If Proxy

// returns a nil *URL, no proxy is used.

//

// socks5 proxying does not currently support spdy streaming endpoints.

Proxy func(*http.Request) (*url.URL, error)

Maybe need to append to TLSClientConfig instance ourselves?

ok, but what would you append?

hmm if it was erroring on x509 I think
https://github.com/kubernetes/apiserver/blob/25ccbfa75c69642e44ac9be141e3bacc7edb3879/pkg/util/webhook/client.go#L136-L140 could work, or something nicer like placing the cert in the golang "respected" path https://go.dev/src/crypto/x509/root_linux.go

Fixing that

Nice and clean
Just making sure here: the reason we don't care about proxying the k8s API calls is that it's somehow taken care of in another internal k8s layer?

We have no good reason to proxy the 2 k8s get/update calls here. Do we?

No the assumption is that the proxy is on the edge of the cluster (aka stuff that tries to go outside of the cluster). I see no use case where it makes sense to proxy traffic inside the cluster.

Yeah reading the openshift proxy docs they back that statement by calling it the cluster-wide egress proxy

akalenyu · 2022-10-30T15:03:25Z

pkg/controller/import-controller.go

@@ -1259,7 +1247,7 @@ func makeImporterContainerSpec(image, verbose, pullPolicy string) *corev1.Contai
 	}
 }

-func createProxyConfigMapVolume(certVolName, objRef string) corev1.Volume {
+func createConfigMapVolume(certVolName, objRef string) corev1.Volume {


pkg/controller/dataimportcron-controller.go

Signed-off-by: Arnon Gilboa <[email protected]>

arnongilboa · 2022-10-31T16:09:59Z

/retest

awels · 2022-11-01T12:37:44Z

pkg/controller/dataimportcron-controller.go

 						},
 					},
-					TTLSecondsAfterFinished: &ttlSecondsAfterFinished,
-					BackoffLimit:            &backoffLimit,
+					TTLSecondsAfterFinished: pointer.Int32(3),


So this was 0 before, it is now 3, any reason it changed?

tests/import_proxy_test.go

awels · 2022-11-01T16:10:00Z

tests/import_proxy_test.go

+			dvName := ""
+			Eventually(func() string {
+				cron, err = f.CdiClient.CdiV1beta1().DataImportCrons(ns).Get(context.TODO(), cronName, metav1.GetOptions{})
+				dvName = cron.Status.CurrentImports[0].DataVolumeName


Can cron.Status.CurrentImports ever have length 0? if so this will not work maybe check to ensure the length > 0 before looking up the name

Also does it make sense to combine these two eventually into one? its looking up the same object just different fields.

tests/import_proxy_test.go

awels · 2022-11-01T16:40:19Z

tools/cdi-source-update-poller/main.go

@@ -58,6 +64,8 @@ func main() {
 	if err != nil {
 		log.Fatalf("Failed BuildConfigFromFlags, kubeURL %s configPath %s: %v", kubeURL, configPath, err)
 	}
+	cfg.TLSClientConfig = rest.TLSClientConfig{Insecure: true}


Yes leaving this like this is an immediate CVE, so lets not do that. It looks like TLSClientConfig has a CAData []byte field where you should be able to load the appropriate CA data (from the directory), might have to append the standard CA bundle together with the added CA bundle.

pkg/controller/dataimportcron-controller.go

awels · 2022-11-01T18:08:09Z

tests/import_proxy_test.go

+			Expect(err).ToNot(HaveOccurred())
+
+			By("Checking the importer pod information in the proxy log to verify if the requests were proxied")
+			verifyImporterPodInfoInProxyLogs(f, url, registryUserAgent, now, BeTrue)


You can't use this function the way it is. This functions looks for the importer pod (which we know works already). We are interested in the poller working properly. So you have to either modify this function to pass in the prefix for the pod to look for, or use some other method of finding the poller pod.

Signed-off-by: Arnon Gilboa <[email protected]>

awels

looks good, one question and I would love to see some unit tests for the environment variable adding in the dataimportcron-controller

pkg/controller/dataimportcron-controller.go

awels

Three things for the unit test:

You modified the successful history and failed history counts, can you ensure they are what we expect.
You verified the env variables are set when the proxy has a value, can you also verify they are NOT set when the proxy does not have a value?
Also please verify that the extra volume is NOT set when the caProxy value is empty. It is possible to have a proxy but not need a custom CA since the CA of the proxy is signed by a known CA

Signed-off-by: Arnon Gilboa <[email protected]>

awels

/approve

kubevirt-bot · 2022-11-03T15:24:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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~~ [awels]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

awels · 2022-11-03T20:00:10Z

/cherrypick release-v1.55

kubevirt-bot · 2022-11-03T20:00:55Z

@awels: new pull request created: #2459

In response to this:

/cherrypick release-v1.55

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.

kubevirt-bot · 2022-11-03T20:01:46Z

@arnongilboa: new pull request could not be created: failed to create pull request against kubevirt/containerized-data-importer#release-v1.55 from head kubevirt-bot:cherry-pick-2455-to-release-v1.55: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for kubevirt-bot:cherry-pick-2455-to-release-v1.55."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherrypick release-v1.55
/cherrypick release-v1.49

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.

arnongilboa · 2022-11-03T20:02:46Z

/cherrypick release-v1.49

kubevirt-bot · 2022-11-03T20:03:41Z

@arnongilboa: #2455 failed to apply on top of branch "release-v1.49":

Applying: Add DataImportCron CronJobs Proxy support
Using index info to reconstruct a base tree...
M	pkg/controller/dataimportcron-controller.go
M	pkg/controller/import-controller.go
M	pkg/importer/registry-datasource.go
M	tests/dataimportcron_test.go
M	tests/import_proxy_test.go
M	tests/operator_test.go
M	tools/cdi-source-update-poller/main.go
Falling back to patching base and 3-way merge...
Auto-merging tools/cdi-source-update-poller/main.go
CONFLICT (content): Merge conflict in tools/cdi-source-update-poller/main.go
Auto-merging tests/operator_test.go
Auto-merging tests/import_proxy_test.go
Auto-merging tests/dataimportcron_test.go
Auto-merging pkg/importer/registry-datasource.go
CONFLICT (content): Merge conflict in pkg/importer/registry-datasource.go
Auto-merging pkg/controller/import-controller.go
Auto-merging pkg/controller/dataimportcron-controller.go
CONFLICT (content): Merge conflict in pkg/controller/dataimportcron-controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add DataImportCron CronJobs Proxy support
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v1.49

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.

Manual backport of kubevirt#2455 Signed-off-by: Arnon Gilboa <[email protected]>

Manual backport of #2455 Signed-off-by: Arnon Gilboa <[email protected]> Signed-off-by: Arnon Gilboa <[email protected]>

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 30, 2022

kubevirt-bot requested review from awels and maya-r October 30, 2022 14:47

kubevirt-bot added the size/L label Oct 30, 2022

akalenyu reviewed Oct 30, 2022

View reviewed changes

Add DataImportCron CronJobs Proxy support

ca4c990

Signed-off-by: Arnon Gilboa <[email protected]>

arnongilboa force-pushed the dic_cronjob_proxy_support branch from 206c77a to ca4c990 Compare October 31, 2022 11:20

awels reviewed Nov 1, 2022

View reviewed changes

CR fixes

94564d0

Signed-off-by: Arnon Gilboa <[email protected]>

awels reviewed Nov 2, 2022

View reviewed changes

pkg/controller/dataimportcron-controller.go Show resolved Hide resolved

awels reviewed Nov 3, 2022

View reviewed changes

arnongilboa force-pushed the dic_cronjob_proxy_support branch from d9f78d0 to 76b56ea Compare November 3, 2022 14:21

Add cronjob env utest

aa983d8

Signed-off-by: Arnon Gilboa <[email protected]>

arnongilboa force-pushed the dic_cronjob_proxy_support branch from 76b56ea to aa983d8 Compare November 3, 2022 15:10

awels approved these changes Nov 3, 2022

View reviewed changes

kubevirt-bot assigned awels Nov 3, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2022

kubevirt-bot merged commit eef0358 into kubevirt:main Nov 3, 2022

kubevirt-bot mentioned this pull request Nov 3, 2022

[release-v1.55] Add DataImportCron CronJobs Proxy support #2459

Merged

arnongilboa added a commit to arnongilboa/containerized-data-importer that referenced this pull request Nov 5, 2022

[release-v1.49] Add DataImportCron CronJobs Proxy support

7ae6180

Manual backport of kubevirt#2455 Signed-off-by: Arnon Gilboa <[email protected]>

arnongilboa mentioned this pull request Nov 5, 2022

[release-v1.49] Add DataImportCron CronJobs Proxy support #2462

Merged

kubevirt-bot pushed a commit that referenced this pull request Nov 10, 2022

[release-v1.49] Add DataImportCron CronJobs Proxy support (#2462)

c2d8871

Manual backport of #2455 Signed-off-by: Arnon Gilboa <[email protected]> Signed-off-by: Arnon Gilboa <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DataImportCron CronJobs Proxy support #2455

Add DataImportCron CronJobs Proxy support #2455

arnongilboa commented Oct 30, 2022 •

edited

Loading

akalenyu left a comment

akalenyu Oct 30, 2022

arnongilboa Oct 30, 2022

akalenyu Oct 30, 2022

akalenyu Oct 30, 2022

arnongilboa Oct 30, 2022

akalenyu Oct 30, 2022

arnongilboa Oct 30, 2022

akalenyu Oct 30, 2022

arnongilboa Oct 30, 2022 •

edited

Loading

akalenyu Oct 30, 2022 •

edited

Loading

arnongilboa Oct 30, 2022

akalenyu Oct 30, 2022

arnongilboa Nov 2, 2022

akalenyu Nov 2, 2022

arnongilboa Nov 2, 2022

awels Nov 3, 2022

akalenyu Nov 3, 2022

akalenyu Oct 30, 2022

arnongilboa commented Oct 31, 2022

awels Nov 1, 2022

awels Nov 1, 2022

awels Nov 1, 2022

awels Nov 1, 2022

arnongilboa Nov 2, 2022

awels left a comment

awels left a comment •

edited

Loading

awels left a comment

kubevirt-bot commented Nov 3, 2022

awels commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

arnongilboa commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

	// Proxy is the proxy func to be used for all requests made by this
	// transport. If Proxy is nil, http.ProxyFromEnvironment is used. If Proxy
	// returns a nil *URL, no proxy is used.
	//
	// socks5 proxying does not currently support spdy streaming endpoints.
	Proxy func(http.Request) (url.URL, error)

Add DataImportCron CronJobs Proxy support #2455

Add DataImportCron CronJobs Proxy support #2455

Conversation

arnongilboa commented Oct 30, 2022 • edited Loading

akalenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

akalenyu Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

awels left a comment • edited Loading

Choose a reason for hiding this comment

awels left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Nov 3, 2022

awels commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

arnongilboa commented Nov 3, 2022

kubevirt-bot commented Nov 3, 2022

arnongilboa commented Oct 30, 2022 •

edited

Loading

arnongilboa Oct 30, 2022 •

edited

Loading

akalenyu Oct 30, 2022 •

edited

Loading

awels left a comment •

edited

Loading