inject proxy env vars into requesting workload resources by deads2k · Pull Request #224 · openshift/cluster-version-operator

deads2k · 2019-07-18T14:40:50Z

set .metadata.annotations["config.openshift.io/inject-proxy"]="container1,initcontainer2"
to inject into specified container names

deads2k · 2019-07-18T14:41:06Z

@ironcladlou @bparees fyi

bparees · 2019-07-18T14:57:39Z

/cc @abhinavdahiya @danehans

openshift-ci-robot · 2019-07-18T14:57:40Z

@bparees: GitHub didn't allow me to request PR reviews from the following users: danehans.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @abhinavdahiya @danehans

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.

bparees · 2019-07-18T14:58:55Z

also /cc @adambkaplan @dmage @cdaley as this would be relevant to the registry operator if it were implemented, since it is currently managing its own proxy env config (inconveniently).

smarterclayton · 2019-07-23T14:52:05Z

+
+	// if proxy injection is requested, get the proxy values and use them
+	// TODO You'll want a mutation helper that takes a pointer to a pod spec so you can wire it to other workload resources
+	if containerNamesString := deployment.Annotations["config.openshift.io/inject-proxy"]; len(containerNamesString) > 0 {


Not terrible.

Which operators would use this? The ones that need all of their traffic to go through the proxy, including service network (I assume status.noProxy must ALWAYS contain service network or you cause an outage?)

auth operator for example....

the noProxy will be provided by network operator which should set these (I asked him same question during arch call ;-)

The image registry operator may use this. We already have similar logic inside of our reconciliation loop. The expectation is that noProxy covers the API server. And our operator needs to interact with external services like S3.

the noProxy will be provided by network operator which should set these

That's correct. I plan to enforce the default noProxy values as part of openshift/cluster-network-operator#245

@danehans the registry operator communicates to cloud apis even when the cluster is not running on that particular cloud.

@bparees the registry operator is then responsible for augmenting the cluster-wide noProxy with additional no proxy values, correct? This is the WIP implementation of the cloud metadata ip.

@danehans I don't know if that is desirable, particularly if a customer wants their baremetal cluster to have all outbound traffic go through their proxy (and yet still use s3 or other cloud provider storage as their backend).

@bparees the registry operator is then responsible for augmenting the cluster-wide noProxy with additional no proxy values, correct?

? My point is that the registry operator has a need to use the proxy configuration when it is talking to the cloudapis that exist outside the cloudprovider the cluster is running on. NoProxy is irrelevant.

@dmage was observing that the registry operator has a use for this proxy env injection. You said they didn't need it because they are talking to cloud apis, but that is incorrect. They are talking to cloudapis, but not cloudapis from the cluster's cloud provider, therefore they will need to go through the proxy (assuming the customer's network topology requires it, which would be our assumption since it's external traffic like any other).

If the customer does not want their AWS api calls going through the proxy (and assuming their network topology doesn't require it), the admin would have to add the AWS api endpoints to their NoProxy value.

I don't know if that is desirable, particularly if a customer wants their baremetal cluster to have all outbound traffic go through their proxy (and yet still use s3 or other cloud provider storage as their backend).

@adambkaplan the cloud metadata ip will not be set for vsphere, baremetal and non platform types.

smarterclayton · 2019-07-23T14:55:50Z

/hold

danehans · 2019-07-24T17:40:35Z

@deads2k @sttts I'm in favor of this approach.

danehans · 2019-07-29T17:41:31Z

@deads2k @sttts is this PR moving forward or just a reference implementation that operators can follow?

smarterclayton · 2019-07-30T13:40:03Z

@abhinavdahiya have you looked at this PR yet?

deads2k · 2019-07-31T12:47:11Z

We've agreed to go this route.

/hold cancel

abhinavdahiya · 2019-07-31T14:04:07Z

How do we handle a case where an operator wants to remove the envs from it's containers as part of the update??

abhinavdahiya · 2019-07-31T14:05:31Z


-	"k8s.io/klog"
-
+	v1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"


nit configv1

deads2k · 2019-07-31T14:51:40Z

How do we handle a case where an operator wants to remove the envs from it's containers as part of the update??

It's hard to imagine where that's useful. We can build a special value into the annotation if it comes up.

set .metadata.annotations["config.openshift.io/inject-proxy"]="container1,initcontainer2" to inject into specified container names

abhinavdahiya · 2019-07-31T15:53:23Z

How do we handle a case where an operator wants to remove the envs from it's containers as part of the update??

It's hard to imagine where that's useful. We can build a special value into the annotation if it comes up.

sure.

I think this get's other operators unblocked for time being. i'll take a todo to add some docs and any fixes needed for 4.1 to 4.2 upgrade.

Thanks!

/lgtm

openshift-ci-robot · 2019-07-31T15:53:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, deads2k

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:

~~OWNERS~~ [abhinavdahiya]

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

abhinavdahiya · 2019-07-31T15:54:22Z

/hold

abhinavdahiya · 2019-07-31T17:47:27Z

/hold cancel

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 18, 2019

openshift-ci-robot requested review from crawford and wking July 18, 2019 14:40

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2019

openshift-ci-robot requested a review from abhinavdahiya July 18, 2019 14:57

deads2k closed this Jul 22, 2019

deads2k reopened this Jul 22, 2019

smarterclayton reviewed Jul 23, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2019

stlaz mentioned this pull request Jul 30, 2019

deployment: add env vars from the Proxy object openshift/cluster-authentication-operator#155

Merged

deads2k force-pushed the proxy branch from ab05a37 to 958a0f2 Compare July 31, 2019 12:44

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 Jul 31, 2019

deads2k changed the title ~~[WIP] demonstrate proxy injection~~ inject proxy env vars into requesting workload resources Jul 31, 2019

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 Jul 31, 2019

deads2k force-pushed the proxy branch from 958a0f2 to 986f1e4 Compare July 31, 2019 12:45

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2019

abhinavdahiya reviewed Jul 31, 2019

View reviewed changes

Comment thread lib/resourcebuilder/apps.go

abhinavdahiya reviewed Jul 31, 2019

View reviewed changes

Comment thread lib/resourcebuilder/podspec.go

inject proxy env vars into workload resources

ce10cd0

set .metadata.annotations["config.openshift.io/inject-proxy"]="container1,initcontainer2" to inject into specified container names

deads2k force-pushed the proxy branch from 986f1e4 to ce10cd0 Compare July 31, 2019 15:03

openshift-ci-robot assigned abhinavdahiya Jul 31, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2019

abhinavdahiya reviewed Jul 31, 2019

View reviewed changes

Comment thread lib/resourcebuilder/podspec.go

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2019

openshift-merge-robot merged commit c9cb9c6 into openshift:master Jul 31, 2019

deads2k mentioned this pull request Jul 31, 2019

ask for proxy env vars to be injected for OIDC communication openshift/cluster-authentication-operator#166

Merged

danehans mentioned this pull request Aug 8, 2019

Adds cvo opt-in annotation for proxy openshift/cluster-network-operator#281

Closed

bparees mentioned this pull request Aug 10, 2019

Updating name of container to inject proxy information into openshift/cluster-image-registry-operator#362

Merged

danehans mentioned this pull request Nov 7, 2019

Bug 1752725: Log into kibana console get 504 Gateway Time-out The server didn't respond in time. when http_proxy enabled openshift/cluster-logging-operator#255

Merged

deads2k mentioned this pull request Jan 12, 2021

csi: add configobserver controller and hook helpers openshift/library-go#976

Merged


		"k8s.io/klog"

		v1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"

Conversation

deads2k commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Jul 18, 2019

Uh oh!

bparees commented Jul 18, 2019

Uh oh!

openshift-ci-robot commented Jul 18, 2019

Uh oh!

bparees commented Jul 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Jul 23, 2019

Uh oh!

danehans commented Jul 24, 2019

Uh oh!

danehans commented Jul 29, 2019

Uh oh!

smarterclayton commented Jul 30, 2019

Uh oh!

deads2k commented Jul 31, 2019

Uh oh!

Uh oh!

abhinavdahiya commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deads2k commented Jul 31, 2019

Uh oh!

abhinavdahiya commented Jul 31, 2019

Uh oh!

openshift-ci-robot commented Jul 31, 2019

Uh oh!

abhinavdahiya commented Jul 31, 2019

Uh oh!

Uh oh!

abhinavdahiya commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

deads2k commented Jul 18, 2019 •

edited

Loading