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

Use authbind to bind privileged ports #2894

Merged
merged 1 commit into from
Aug 5, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Aug 3, 2018

What this PR does / why we need it:

Removes the attempt to listen ports in the ingress controller to check if a port is available with a simple connection check and adds authbind as mechanism to listen on privileged ports

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 3, 2018
ln.Close()
return true
defer conn.Close()
return false
Copy link
Member

Choose a reason for hiding this comment

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

this function is being called in cmd//nginx/flags.go when Nginx has not started yet, which results in for the dial to fail. And consequently


F0803 14:42:19.694324       5 main.go:72] Port 80 is already in use. Please check the flag --http-port

@aledbf aledbf force-pushed the authbind branch 2 times, most recently from 800d5b5 to 5aa30ed Compare August 4, 2018 01:42
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2018
@codecov-io
Copy link

codecov-io commented Aug 4, 2018

Codecov Report

Merging #2894 into master will increase coverage by 0.01%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2894      +/-   ##
==========================================
+ Coverage   47.58%   47.59%   +0.01%     
==========================================
  Files          76       76              
  Lines        5485     5490       +5     
==========================================
+ Hits         2610     2613       +3     
- Misses       2540     2541       +1     
- Partials      335      336       +1
Impacted Files Coverage Δ
internal/ingress/controller/util.go 27.77% <0%> (ø) ⬆️
cmd/nginx/main.go 6.57% <0%> (ø) ⬆️
internal/ingress/controller/nginx.go 11.68% <100%> (+0.39%) ⬆️
internal/ingress/metric/collectors/controller.go 90.67% <100%> (ø) ⬆️
internal/net/net.go 72.72% <100%> (ø) ⬆️
internal/ingress/metric/collectors/socket.go 79.27% <60%> (-0.54%) ⬇️
internal/ingress/controller/config/config.go 98.27% <0%> (-0.02%) ⬇️

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 9b3207d...b148f11. Read the comment docs.

@aledbf aledbf force-pushed the authbind branch 2 times, most recently from b09e161 to 80c14c9 Compare August 4, 2018 16:14
@@ -741,7 +741,10 @@ func configureDynamically(pcfg *ingress.Configuration, port int) error {
backends := make([]*ingress.Backend, len(pcfg.Backends))

for i, backend := range pcfg.Backends {
service := &apiv1.Service{Spec: backend.Service.Spec}
var service *apiv1.Service
if backend.Service != nil {
Copy link
Member

@ElvinEfendi ElvinEfendi Aug 5, 2018

Choose a reason for hiding this comment

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

What did necessitate this? Is this fixing an actual bug? Maybe add a test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Running e2e tests I saw some errors when Service is nil trying to access the Spec field

return nil, err
}

err = os.Chmod(socket, 0777)
Copy link
Member

@ElvinEfendi ElvinEfendi Aug 5, 2018

Choose a reason for hiding this comment

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

Is this needed so Nginx can connect to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

@ElvinEfendi
Copy link
Member

please squash commits, then it's good to go 👍

@ElvinEfendi
Copy link
Member

/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 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants