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

update the egress rules to allow loadBalancer IP on consumer side #289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rewantsoni
Copy link
Member

The consumer needs to access the loadbalancer IP, hence we need to put the rule in the allow list

@rewantsoni rewantsoni requested a review from bindrad May 31, 2023 10:01
@rewantsoni rewantsoni force-pushed the egress branch 2 times, most recently from 86a7c3c to 17147b9 Compare May 31, 2023 11:02
if r.DeploymentType == consumerDeploymentType {

storageProviderEndpointURL := r.addonParams[storageProviderEndpointKey]
storageProviderEndpointHost := strings.Split(storageProviderEndpointURL, ":")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This might panic if the split is yielding an empty array in the case of a malformed URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a condition to return a error if the split is yielding an empty string

if r.DeploymentType == consumerDeploymentType {

storageProviderEndpointURL := r.addonParams[storageProviderEndpointKey]
storageProviderEndpointHost := strings.Split(storageProviderEndpointURL, ":")[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This might panic if the split is yielding an empty array in the case of a malformed URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a condition to return a error if the split is yielding an empty string

Comment on lines 1607 to 1609
ip := net.ParseIP(storageProviderEndpointHost)

if ip == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you wanted != and not ==.
Also, let's not pollute the func scope if we don't have to.

Suggested change
ip := net.ParseIP(storageProviderEndpointHost)
if ip == nil {
if ip := net.ParseIP(storageProviderEndpointHost); ip == nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to check if the provided storageProviderEndpointHost is a nodeIP or a hostName, we want to allow the loadBalancer Endpoint HostName, we can ignore if we have nodeIP as it is already reachable.


ip := net.ParseIP(storageProviderEndpointHost)

if ip == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you wanted != and not ==.
Also, let's not pollute the func scope if we don't have to.

Suggested change
if ip == nil {
if ip := net.ParseIP(storageProviderEndpointHost); ip == nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to check if the provided storageProviderEndpointHost is a nodeIP or a hostName, we want to allow the loadBalancer Endpoint HostName, we can ignore if we have nodeIP as it is already reachable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants