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

Add support for AJP protocol #2871

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Add support for AJP protocol #2871

merged 1 commit into from
Aug 7, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Jul 29, 2018

No description provided.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 29, 2018
@aledbf aledbf added this to In Progress in 0.18.0 Jul 30, 2018
@aledbf aledbf force-pushed the ajp branch 2 times, most recently from babd52b to 80b0c2f Compare August 5, 2018 23:08
@codecov-io
Copy link

codecov-io commented Aug 5, 2018 •
edited
Loading

Codecov Report

Merging #2871 into master will decrease coverage by 0.06%.
The diff coverage is 51.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2871      +/-   ##
==========================================
- Coverage   47.65%   47.58%   -0.07%     
==========================================
  Files          76       77       +1     
  Lines        5500     5512      +12     
==========================================
+ Hits         2621     2623       +2     
- Misses       2541     2549       +8     
- Partials      338      340       +2
Impacted Files Coverage Δ
internal/ingress/types.go 0% <ø> (ø) ⬆️
internal/ingress/controller/controller.go 2.21% <0%> (-0.01%) ⬇️
internal/ingress/annotations/annotations.go 80.95% <100%> (+0.3%) ⬆️
internal/ingress/controller/template/template.go 65.56% <46.66%> (-0.69%) ⬇️
...ternal/ingress/annotations/backendprotocol/main.go 61.53% <61.53%> (ø)
internal/net/net.go 72.72% <0%> (-2.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c97a90f...7af93e0. Read the comment docs.

@aledbf aledbf changed the title WIP: Add support for AJP protocol Add support for AJP protocol Aug 5, 2018
@k8s-ci-robot k8s-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 Aug 5, 2018
@aledbf aledbf force-pushed the ajp branch 5 times, most recently from 6b512bc to 7cf4e13 Compare August 6, 2018 00:05
antoineco
antoineco reviewed Aug 6, 2018
View reviewed changes
Copy link
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Looking good!

internal/ingress/annotations/backendprotocol/main_test.go Outdated
t.Errorf("unexpected error parsing ingress with sslpassthrough")
}

// test with a valid host
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this expected to influence the test or some leftover from another test?

internal/ingress/controller/template/template.go

proxyPass := "proxy_pass"

switch location.BackendProtocol {
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 this PR is the right place to also start using the annotation in existing e2e tests (GRPC is the only one coming to my mind right now).

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ElvinEfendi
ElvinEfendi reviewed Aug 6, 2018
View reviewed changes
internal/ingress/annotations/backendprotocol/main.go
}

proto, err := parser.GetStringAnnotation("backend-protocol", ing)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

should we log the error here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No? (we are not doing that in any other annotation that requires to return a default value)

Copy link
Member

Choose a reason for hiding this comment

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

I did not mean return error, I meant log a warning just like we do at https://github.com/kubernetes/ingress-nginx/pull/2871/files#diff-42abcd9476cd9422cd55e93f503bd2e2R57

Copy link
Contributor

Choose a reason for hiding this comment

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

That would log an error every time this annotation is unset. Expect a lot of verbosity :)

Copy link
Member

Choose a reason for hiding this comment

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

oh strange that it returns error when the annotation is unset... kcco then

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same question the first time I went through that code:

ingress-nginx/internal/ingress/annotations/parser/main.go

Lines 52 to 58 in cdbb9bd

func (a ingAnnotations) parseString(name string) (string, error) {
val, ok := a[name]
if ok {
return val, nil
}
return "", errors.ErrMissingAnnotations
}

I guess @aledbf has an explanation :)

@antoineco
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco

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

@k8s-ci-robot k8s-ci-robot merged commit 828aea9 into kubernetes:master Aug 7, 2018
@aledbf aledbf deleted the ajp branch August 7, 2018 17:10
@aledbf aledbf moved this from In Progress to done in 0.18.0 Aug 7, 2018
@ElvinEfendi ElvinEfendi mentioned this pull request Aug 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElvinEfendi ElvinEfendi ElvinEfendi left review comments

@antoineco antoineco antoineco left review comments

Assignees

@ElvinEfendi ElvinEfendi

@antoineco antoineco

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
0.18.0
done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aledbf @codecov-io @antoineco @k8s-ci-robot @ElvinEfendi