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

feat: Support extra listeners on NLBs #529

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhuntwork
Copy link
Contributor

@jhuntwork jhuntwork commented Jul 26, 2022

The zalando.org/aws-nlb-extra-listeners annotation accepts a JSON
string that describes a list of extra listen/target ports to add to an
NLB. These will be routed to pods matching a specific label in the same
namespace as the ingress. As such, this depends on the AWS CNI mode
feature.

@szuecs
Copy link
Member

szuecs commented Jul 27, 2022

Thanks @jhuntwork , I think because of vacations and some pressing issues we won't be able to review your PR until September.
I hope it's fine and not too much blocking!

@jhuntwork
Copy link
Contributor Author

@szuecs Thanks for the note! I think I was hoping for a quicker review than that, but I understand... vacations are important! Looking forward to the feedback when it comes.

@jhuntwork jhuntwork force-pushed the master branch 2 times, most recently from 7394ef1 to dc5436f Compare August 19, 2022 14:10
@szuecs
Copy link
Member

szuecs commented Aug 25, 2022

@jhuntwork can you please rebase on current master, I had to drop the golint linter in order to make tests work again.

README.md Show resolved Hide resolved
aws/cf.go Outdated
@@ -230,6 +254,7 @@ func createStack(svc cloudformationiface.CloudFormationAPI, spec *stackSpec) (st
cfParam(parameterIpAddressTypeParameter, spec.ipAddressType),
cfParam(parameterLoadBalancerTypeParameter, spec.loadbalancerType),
cfParam(parameterHTTP2Parameter, fmt.Sprintf("%t", spec.http2)),
cfParam(parameterCascadeParameter, fmt.Sprintf("%t", spec.cascade)),
Copy link
Member

Choose a reason for hiding this comment

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

this will change the CF stack as far as I understand, so it will retrigger an update.
@mikkeloscar @AlexanderYastrebov I guess it's fine to do in this case, right?

if spec.cascade {
listener = ExtraListener{ListenProtocol: "TCP", ListenPort: 443, TargetPort: 443, cascade: true}
}
template.AddResource(httpsTargetGroupName, newTargetGroup(spec, parameterTargetGroupTargetPortParameter, listener))
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 do the code path with less changes on current CF stacks?
I would imagine it would be fine like:

extraListener := ExtraListener{}
if spec.cascade {
    listener = ExtraListener{ListenProtocol: "TCP", ListenPort: 443, TargetPort: 443, cascade: true}
    template.AddResource(httpsTargetGroupName, newTargetGroup(spec, parameterTargetGroupTargetPortParameter, extraListener))
} else {
    template.AddResource(httpsTargetGroupName, newTargetGroup(spec, parameterTargetGroupTargetPortParameter))
}

and later only use extraListener in case we have if spec.cascade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you still have to pass in the parameter to the function? Which is why it's just initialized as empty first before the if statement.

aws/cf_template.go Outdated Show resolved Hide resolved
@szuecs
Copy link
Member

szuecs commented Aug 25, 2022

@jhuntwork can you please split the 2 features into 2 separate PRs? It will be easier to review.
Another request is to add tests that cover the new feature in some way.

@jhuntwork
Copy link
Contributor Author

@jhuntwork can you please split the 2 features into 2 separate PRs? It will be easier to review. Another request is to add tests that cover the new feature in some way.

Sure, I can do that. I needed both to be able to handle my specific use case, but happy to split them apart. And agreed, test coverage would be good. Thanks for the feedback so far.

@jhuntwork jhuntwork changed the title feat: Advanced options for NLBs feat: Support extra listeners on NLBs Sep 15, 2022
@jhuntwork
Copy link
Contributor Author

@szuecs This is reduced to just the first feature now and also rebased on latest master. I'm still working on a couple of tests, so those will be coming soon, hopefully.

@jhuntwork
Copy link
Contributor Author

@szuecs Updated with some tests. Should be ready for review again, please.

@jhuntwork
Copy link
Contributor Author

Hi @szuecs. Is there something more I need to do to get this reviewed?

@szuecs
Copy link
Member

szuecs commented Nov 21, 2022

@jhuntwork the feature enables to have git like interfaces right?
The extra listener would be for ssh for example. Do I get this right or do you see any other relevant use case?
This week is cyberweek, so likely next week. And I will try to review with @AlexanderYastrebov

@webframp
Copy link

webframp commented Mar 8, 2023

Thanks for sharing the update @szuecs, looking forward to this getting merged eventually.

@szuecs
Copy link
Member

szuecs commented Mar 8, 2023

You can see the progress in our test strategy #587

@szuecs
Copy link
Member

szuecs commented Apr 22, 2023

@jhuntwork can you rebase and create a golden file test?

@jhuntwork
Copy link
Contributor Author

@jhuntwork can you rebase and create a golden file test?

Thanks, will do!

@jhuntwork
Copy link
Contributor Author

Just coming back to this after a long while. Rebased and will now work on the goldenfile and tests.

The `zalando.org/aws-nlb-extra-listeners` annotation accepts a JSON
string that describes a list of extra listen/target ports to add to an
NLB. These will be routed to pods matching a specific label in the same
namespace as the ingress. As such, this depends on the AWS CNI mode
feature.

Signed-off-by: Jeremy Huntwork <[email protected]>
@jhuntwork
Copy link
Contributor Author

The tests are working, but there's just a couple things more I want to refactor. I'll submit a few more changes and remove the Draft status once it's in better shape.

@jhuntwork
Copy link
Contributor Author

@szuecs I was thinking maybe some parts could use a refactor, but I might just need to get some of your input on this again now before changing anything else. Do you have time to review?

@@ -655,8 +665,33 @@ func (a *Adapter) UpdateTargetGroupsAndAutoScalingGroups(stacks []*Stack, proble
a.TargetCNI.TargetGroupCh <- targetTypesARNs[elbv2.TargetTypeEnumIp]
}

// run through any target groups with ALB targets and register all ALBs
for _, tg := range targetTypesARNs[elbv2.TargetTypeEnumAlb] {
Copy link
Member

Choose a reason for hiding this comment

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

What's about NLBs?

func (a *Adapter) getRegisteredTargets(tgARN string) ([]string, error) {
tgh, err := a.elbv2.DescribeTargetHealth(&elbv2.DescribeTargetHealthInput{TargetGroupArn: &tgARN})
if err != nil {
log.Errorf("unable to describe target health %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

log.Errorf("unable to describe target health %v", err) -> log.Errorf("Failed to describe target health %v", err)

@@ -1102,36 +1139,56 @@ func nonTargetedASGs(ownedASGs, targetedASGs map[string]*autoScalingGroupDetails
return nonTargetedASGs
}

func (a *Adapter) getRegisteredTargets(tgARN string) ([]string, error) {
tgh, err := a.elbv2.DescribeTargetHealth(&elbv2.DescribeTargetHealthInput{TargetGroupArn: &tgARN})
Copy link
Member

Choose a reason for hiding this comment

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

&tgARN is correct but we should use aws.String(tgARN)

}
registeredTargets := make([]string, len(tgh.TargetHealthDescriptions))
for i, target := range tgh.TargetHealthDescriptions {
registeredTargets[i] = *target.Target.Id
Copy link
Member

Choose a reason for hiding this comment

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

*target.Target.Id is correct but we should use aws.StringValue(target.Target.Id) instead

func (a *Adapter) registerAndDeregister(new []string, old []string, tgARN string) error {
toRegister := difference(new, old)
if len(toRegister) > 0 {
log.Info("Registering CNI targets: ", toRegister)
Copy link
Member

Choose a reason for hiding this comment

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

better use formatstring

}
toDeregister := difference(old, new)
if len(toDeregister) > 0 {
log.Info("Deregistering CNI targets: ", toDeregister)
Copy link
Member

Choose a reason for hiding this comment

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

better use formatstring

log.Debugf("Looking for tags on %s", aws.StringValue(tg.TargetGroupArn))
out, err := elbv2svc.DescribeTags(&elbv2.DescribeTagsInput{ResourceArns: []*string{tg.TargetGroupArn}})
if err != nil {
log.Errorf("cannot describe tags on target group: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

We always start uppercase for logs "Failed to describe...".

Copy link
Member

@szuecs szuecs Dec 22, 2023

Choose a reason for hiding this comment

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

Does it make sense to continue here, because otherwise you work with empty string below the else.
Or omit setting the label with empty string.
Wdyt?

}
if arn, ok := o[outputHTTPTargetGroupARN]; ok {
arns = append(arns, arn)
for k, v := range o {
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think it's better to have the 2 map lookups instead of a range loop over the whole map.
Do we have more than these 2 map lookups, now?

@@ -473,6 +504,13 @@ func mapToManagedStack(stack *cloudformation.Stack) *Stack {
if key == ingressOwnerTag {
ownerIngress = value
}

if key == extraListenersTag {
decodedListeners, _ := base64.StdEncoding.DecodeString(value)
Copy link
Member

Choose a reason for hiding this comment

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

you should use err and return early if there is one

if key == extraListenersTag {
decodedListeners, _ := base64.StdEncoding.DecodeString(value)
if err := json.Unmarshal(decodedListeners, &extraListeners); err != nil {
return &Stack{}, err
Copy link
Member

Choose a reason for hiding this comment

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

should not this return nil?

}
}
return true
})
if err != nil {
return nil, fmt.Errorf("findManagedStacks failed to list stacks: %w", err)
}
if len(errors) > 0 {
return nil, fmt.Errorf("mapToManagedStacks returned errors: %v", errors)
Copy link
Member

Choose a reason for hiding this comment

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

%v to %w

if err != nil {
errors = append(errors, err)
}
stacks = append(stacks, stack)
Copy link
Member

Choose a reason for hiding this comment

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

we would here either add empty stack (&Stack{}) or nil. We could think of a continue in the error path instead.
I am not sure if we need empty stacks or nil stacks for later use, maybe for deletion or new creation.

s := stackOutput{outputLoadBalancerARN: want}
got := s.lbARN()
if want != got {
t.Errorf("unexpected result. wanted %+v, got %+v", want, got)
Copy link
Member

Choose a reason for hiding this comment

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

want and got should be string so %s

return nil, errors.New("extra listeners are only supported on NLBs")
}
if err := json.Unmarshal([]byte(rawlisteners), &extraListeners); err != nil {
return nil, fmt.Errorf("unable to parse aws-nlb-extra-listeners annotation: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

in fmt.Errorf always use %w for passing err.

return nil, fmt.Errorf("unable to parse aws-nlb-extra-listeners annotation: %v", err)
}
for idx, listener := range extraListeners {
if listener.ListenProtocol != "TCP" && listener.ListenProtocol != "UDP" && listener.ListenProtocol != "TCP_UDP" {
Copy link
Member

Choose a reason for hiding this comment

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

maybe nicer to switch:

switch listener.ListenProtocol {
case "TCP","UDP","TCP_UDP":
// ok
default:
  return nil, error..
}

"sync"
"time"

log "github.com/sirupsen/logrus"
"github.com/zalando-incubator/kube-ingress-aws-controller/aws"
Copy link
Member

@szuecs szuecs Dec 22, 2023

Choose a reason for hiding this comment

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

this should not be a dependency in the kubernetes package
We should convert it into aws.CNIEnpoint in aws/adapter.go or in worker.go

@szuecs
Copy link
Member

szuecs commented Dec 22, 2023

@jhuntwork I did my review. In general try to have no "aws" in "kubernetes" package and also the other way around.
We build a business object "Ingress" for the business logic for aws.

@szuecs
Copy link
Member

szuecs commented Dec 22, 2023

Maybe split the pr into more than one. One refactoring (type introduction can be one) and the actual extraListener/extraTg.

@jhuntwork
Copy link
Contributor Author

Thanks for the feedback! I'll see about making it smaller, refactoring in prep for the feature.

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.

4 participants