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

GCE/GKE "pre-shared" TLS cert #291

Merged
merged 12 commits into from
Mar 7, 2017
Merged

GCE/GKE "pre-shared" TLS cert #291

merged 12 commits into from
Mar 7, 2017

Conversation

tonglil
Copy link
Contributor

@tonglil tonglil commented Feb 16, 2017 •
edited
Loading

What this PR does / why we need it:

  • Ingress can reference to an external TLS cert
  • Ingress does not use TLS secret if a reference exists (pending validation code)
  • unit test to create an Ingress with the annotation

Which issue this PR fixes:
Closes #45.
Related to kubernetes/kubernetes#41593.

Special notes for your reviewer:

  • there is a TODO item on naming of this annotation key.

Release note:

@k8s-reviewable
Copy link

This change is Reviewable

@tonglil tonglil mentioned this pull request Feb 16, 2017
Closed
2 tasks
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 45.762% when pulling a99720f on tonglil:gce-tls into a41ee3f on kubernetes:master.

@bprashanth
Copy link
Contributor

Great, thanks for the pr!
Did you actually move past this issue: #267?

@tonglil
Copy link
Contributor Author

tonglil commented Feb 16, 2017

No I did not, still waiting for a time to dig deeper into that.

For this one, I ssh into the master of the e2e to change the image pulled.

@bprashanth bprashanth self-assigned this Feb 16, 2017
@bprashanth
Copy link
Contributor

@nicksardo this is the pr for #45 (comment)

@tonglil
Copy link
Contributor Author

tonglil commented Feb 27, 2017

Anything I can do to help the review process/bump this?

@nicksardo
Copy link
Contributor

Will be looking at this in a day or two @tonglil. Sorry for the delay.

@nicksardo
Copy link
Contributor

@tonglil, we want to get this merged within the next few hours. In order for this to happen, can you remove the change to the vendored k8s. Move the annotation to controller/utils.go and set as ingress.gcp.kubernetes.io/pre-shared-cert. @thockin doesn't believe we need validation in the kubernetes repo as this is a GCP specific. If other ingress controllers adopt similar functionality, we can address the annotation/validation then. We won't be addressing the e2e tests in #41593 until after 1.6 ships.

After performing a final test, ping me. Thanks

tonglil
tonglil commented Mar 7, 2017
View reviewed changes
controllers/gce/controller/utils.go
// manage this certificate, it is the users responsibility to create/delete it.
// In GCP, the Ingress controller assigns the SSL certificate with this name
// to the target proxies of the Ingress.
preSharedCertKey = "ingress.gcp.kubernetes.io/pre-shared-cert"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to raise to light the mismatch in formatting between annotations (if this is still the right way to go).

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussing annotation naming conventions with Tim, I rather use the correct format moving forward. We can add modify docs later to highlight the disparity.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 46.303% when pulling 3cc0583 on tonglil:gce-tls into edb9b00 on kubernetes:master.

@nicksardo nicksardo self-assigned this Mar 7, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 46.303% when pulling 3cc0583 on tonglil:gce-tls into edb9b00 on kubernetes:master.

@nicksardo
Copy link
Contributor

/lgtm

Your final test was successful?

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

tonglil commented Mar 7, 2017

Yes, the functionality remained the same (minus validation of config).

@nicksardo nicksardo merged commit e1d1445 into kubernetes:master Mar 7, 2017
@tonglil
Copy link
Contributor Author

tonglil commented Mar 7, 2017

Thanks, I'll make a ticket for the documentation!

@tonglil tonglil deleted the gce-tls branch March 7, 2017 21:58
@tonglil tonglil restored the gce-tls branch March 7, 2017 21:58
@tonglil tonglil deleted the gce-tls branch March 7, 2017 22:18
This was referenced Mar 7, 2017
Merged
Closed
kdada pushed a commit to caicloud/ingress that referenced this pull request Apr 13, 2017
@tonglil @kdada
GCE/GKE "pre-shared" TLS cert (kubernetes#291)
3cb0912 
* add allow-named-tls annotation

* works for setting tls

* fix logs (mostly)

* add ssl cert annotation

* return an error when cert not found

* use annotation if specified, otherwise use spec

* add TODO on naming

* use the annotation key from k8s

* add unit test for HTTPS LB w/ cert annotation

* refactor logic and check for error

* move annotation to controller package

* remove todo for function naming
@nicksardo nicksardo mentioned this pull request Jun 23, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicksardo nicksardo nicksardo left review comments

Assignees

@nicksardo nicksardo

@bprashanth bprashanth

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Leverage user supplied secrets directly from cloudprovider
7 participants
@tonglil @k8s-reviewable @coveralls @bprashanth @nicksardo @aledbf @k8s-ci-robot