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

Add support to include LoadBalancerSourceRanges in the impersonator service config #1064

Open
simox-83 opened this issue Mar 15, 2022 · 2 comments
Labels
enhancement New feature or request priority/undecided Not yet prioritized

Comments

@simox-83
Copy link

Is your feature request related to a problem? Please describe.

When using impersonation Proxy in an AWS environment, the security group associated to the ELB is wide opened. There's a way to specify the CIDR range in the service spec as mentioned here: https://aws.amazon.com/premiumsupport/knowledge-center/eks-cidr-ip-address-loadbalancer/

Describe the solution you'd like

Ideally the impersonatorconfigController should include a field to include the loadBalancerSourceRanges key in the service spec:

func (c *impersonatorConfigController) ensureLoadBalancerIsStarted(ctx context.Context, config *v1alpha1.ImpersonationProxySpec) error {

Describe alternatives you've considered

Turning off the impersonationProxy mode and create a K8s service of type LoadBalancer; grab the ELB address given by AWS and store it as External Endpoint in the credentialIssuer spec. The challenge here is to grab the ELB address as its not known upfront and everytime the service gets recreated, it changes.

Are you considering submitting a PR for this feature?

  • How will this project improvement be tested?
  • How does this change the current architecture?
  • How will this change be backwards compatible?
  • How will this feature be documented?

Additional context

Add any other context or screenshots about the feature request here.

@pinniped-ci-bot pinniped-ci-bot added enhancement New feature or request priority/undecided Not yet prioritized labels Mar 18, 2022
@margocrawf
Copy link
Contributor

The maintainer team has other feature work that we're prioritizing right now, so we're not likely to pick this up soon. We'll let you know if that changes, but in the meantime if you're interested in making a PR, we'd be happy to review it.

Here are some things we'd be looking for in a PR:

  • ytt config: When the user looks in deploy/concierge/values.yaml, they see an option to add load_balancer_source_ranges under impersonation_proxy_spec. It is unset by default but they can add a list of loadBalancerSourceRanges. When they edit that value and deploy with kapp, they see that the loadBalancerSourceRanges in the CredentialIssuer and in the Load Balancer Service that gets created.

  • CredentialIssuer CRD config: When they deploy or update the CredentialIssuer , they can specify the following:

apiVersion: config.concierge.pinniped.dev/v1alpha1
kind: CredentialIssuer
...
spec:
  impersonationProxy:
    mode: auto
    service:
      loadBalancerSourceRanges:
      - "143.231.0.0/16"
      type: LoadBalancer
...

this would add the loadBalancerSourceRanges to the Service spec

apiVersion: v1
kind: Service
...
spec:
  ...
  type: LoadBalancer
  loadBalancerSourceRanges:
  - "143.231.0.0/16"

Implementation wise, this would involve picking up the change to the CredentialIssuer inside the controller in impersonation_config.go controller, and using that to set the Load Balancer spec (there should be examples within impersonation_config.go on how we do this with other fields such as the LoadBalancerIP field.)

  • CRD config error case When the user specifies a value that isn't valid, example:
loadBalancerSourceRanges:
- "not-a-source-range"

Then an error shows up in the CredentialIssuer's status so that the user can see what's wrong.

  • unit tests for the success and error cases within impersonation_config_test.go

@nickperry
Copy link

nickperry commented Aug 12, 2022

We have the same requirement. I had to work around it by setting spec.impersonationProxy.service.type: None and setting up a separate service. We would appreciate ImpersonationProxyServiceSpec allowing for specification of loadBalancerSourceRanges. As noted by the issue reporter, the challenge is that the ELB address as its not known upfront and every time the service gets recreated, it changes.

Additionally, we need ImpersonationProxyServiceSpec to allow specification of ExternalTrafficPolicy: Local.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority/undecided Not yet prioritized
Projects
Status: No status
Development

No branches or pull requests

4 participants