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.

Sign up for GitHub

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

Jump to bottom

Introduce GRPC_PROXY EnvVar Support #2364

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Introduce GRPC_PROXY EnvVar Support #2364

merged 1 commit into from
Sep 28, 2021

Conversation

awgreene
Copy link
Member

Introduce the ability to specify a dial context for GRPC connections.

@openshift-ci
Copy link

openshift-ci bot commented Sep 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene

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

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 17, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2021
@awgreene
Copy link
Member Author

Please take a look @kevinrizza @estroz

tylerslaton
tylerslaton reviewed Sep 17, 2021
View reviewed changes
Copy link
Contributor

@tylerslaton tylerslaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions! Looks good.

pkg/controller/registry/grpc/source.go Outdated Show resolved Hide resolved
pkg/controller/registry/grpc/source.go Show resolved Hide resolved
@awgreene awgreene force-pushed the grpc-proxy branch 2 times, most recently from 5ccf427 to c0e0e4f Compare September 17, 2021 21:24
@awgreene
Copy link
Member Author

/retest

njhale
njhale reviewed Sep 20, 2021
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

design questions

pkg/controller/registry/grpc/source.go Show resolved Hide resolved
@awgreene
Copy link
Member Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2021
timflannagan
timflannagan reviewed Sep 28, 2021
View reviewed changes
go.mod Show resolved Hide resolved
@awgreene
Copy link
Member Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2021
@njhale
Copy link
Member

njhale commented Sep 28, 2021

/hold

looks like some related unit tests are failing.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
njhale
njhale reviewed Sep 28, 2021
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late review here, but spotted a few things of note:

pkg/controller/registry/grpc/source.go
func (s *SourceStore) Add(key registry.CatalogKey, address string) (*SourceConn, error) {
_ = s.Remove(key)

conn, err := grpc.Dial(address, grpc.WithInsecure())
conn, err := grpcConnection(address)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we abstract the dial logic away? maybe take a Dailer as a field of SourceStore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we pursue this option wouldn't the connection to the proxy need to remain open indefinitely?

pkg/controller/registry/grpc/source.go
Comment on lines +126 to +129
HTTPProxy: getGRPCProxyEnv(),
HTTPSProxy: getGRPCProxyEnv(),
NoProxy: getEnvAny("NO_PROXY", "no_proxy"),
CGI: os.Getenv("REQUEST_METHOD") != "",
Copy link
Member

@njhale njhale Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We should probably memoize these so we don't fetch them for every dial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable but would make the values static, which likely isn't a problem given how deployments and pods work.

@njhale
Copy link
Member

njhale commented Sep 28, 2021

/lgtm
/hold cancel

looks like the unit test may have been on a stale commit

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021
@awgreene
Introduce GRPC_PROXY EnvVar Support
31c7815 
Introduce the ability to specify a dial context for GRPC connections.

Signed-off-by: Alexander Greene <greene.al1991@gmail.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2021
@kevinrizza
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2021
@openshift-merge-robot openshift-merge-robot merged commit afc0848 into operator-framework:master Sep 28, 2021
@openshift-cherrypick-robot
Copy link

@awgreene: #2364 failed to apply on top of branch "release-4.9":

Applying: Introduce GRPC_PROXY EnvVar Support
Using index info to reconstruct a base tree...
M	go.mod
M	pkg/controller/registry/grpc/source.go
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging pkg/controller/registry/grpc/source.go
CONFLICT (content): Merge conflict in pkg/controller/registry/grpc/source.go
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Introduce GRPC_PROXY EnvVar 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:

/cherry-pick release-4.9

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.

timflannagan added a commit to timflannagan/operator-lifecycle-manager that referenced this pull request Oct 5, 2021
@timflannagan
Revert "Introduce GRPC_PROXY EnvVar Support (operator-framework#2364)"
17d0b76 
This reverts commit afc0848.
timflannagan added a commit to timflannagan/operator-lifecycle-manager that referenced this pull request Oct 6, 2021
@timflannagan
Revert "Introduce GRPC_PROXY EnvVar Support (operator-framework#2364)"
54021c5 
This reverts commit afc0848.
@njhale njhale mentioned this pull request Oct 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhale njhale njhale left review comments

@timflannagan timflannagan timflannagan left review comments

@tylerslaton tylerslaton tylerslaton left review comments

Assignees

@njhale njhale

@kevinrizza kevinrizza

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@awgreene @kevinrizza @njhale @openshift-cherrypick-robot @timflannagan @tylerslaton @openshift-merge-robot