-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[GLBC] Support backside re-encryption #519
Conversation
Changes Unknown when pulling d7eaa26 on nicksardo:backside-reencrypt into ** on kubernetes:master**. |
I am doing this in #463 and also migrating to client-go. |
kubernetes/kubernetes#43644 was merged after 1.6.0 release. After talking with some folks here, it may be possible to cherry pick into 1.6.1. |
994fda9
to
13b2d7b
Compare
@nicksardo why not adding this as a field in Ingress? |
@aledbf Makes more sense as a descriptor for the Service object. Probably not a common case, but there may be users with multiple ingress objs referencing a single service. It also touches the subject of service discovery which was a proposal from long ago. Entities other than ingress controllers may want descriptive information about services. kubernetes/kubernetes#41678 |
👍 |
b1851de
to
5f53f0b
Compare
06f03c4
to
94da993
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs changes LGTM
controllers/gce/backends/backends.go
Outdated
glog.Infof("Creating backend for %d instance groups, port %v named port %v", | ||
len(igs), port, namedPort) | ||
be, err = b.create(igs, namedPort, b.namer.BeName(port)) | ||
glog.Infof("Creating backend for %d instance groups, port %v named port %v", len(igs), p.Port, namedPort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add V(1 or 2) I think?
controllers/gce/backends/backends.go
Outdated
|
||
pProto := utils.GetHTTPScheme(p.Encrypted) | ||
if be.Protocol != pProto { | ||
glog.Infof("Updating backend protocol %v from %v to %v", pName, be.Protocol, pProto) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
glog.V
controllers/gce/backends/backends.go
Outdated
} | ||
pool := b.snapshotter.Snapshot() | ||
for port := range pool { | ||
p, err := strconv.Atoi(port) | ||
p, err := strconv.ParseInt(port, 10, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the port 16-bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe existing code is following the compute library's use of int64 for Port. https://godoc.org/google.golang.org/api/compute/v1#NamedPort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I misunderstood - are you suggesting we assert that the port can fit in 16bits? We could but it would be unnecessary as the string is generated from within the same package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline, probably should use strconv.ParseInt(port, 10, 16)
to be consistent with port being uint16_t
controllers/gce/backends/backends.go
Outdated
healthPath := p.Handler.HTTPGet.Path | ||
// GCE requires a leading "/" for health check urls. | ||
if !strings.HasPrefix(healthPath, "/") { | ||
healthPath = fmt.Sprintf("/%v", healthPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
healthPath = "/" + healthPath, seems heavyweight to use Sprintf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, was just moving, but I'll make the change.
controllers/gce/backends/backends.go
Outdated
hc.RequestPath = healthPath | ||
hc.Host = host | ||
hc.Description = "kubernetes L7 health check from readiness probe." | ||
// set a low health threshold and a high failure threshold. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment seems like it should be attached to healthchecks.DefaultHealthCheckInterval instead of here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore LGTM -- thought change was only docs
…view comments; rename protocol constants
c42b7c0
to
6af52e7
Compare
controllers/gce/backends/backends.go
Outdated
} | ||
pool := b.snapshotter.Snapshot() | ||
for port := range pool { | ||
p, err := strconv.Atoi(port) | ||
p, err := strconv.ParseInt(port, 10, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed offline, probably should use strconv.ParseInt(port, 10, 16)
to be consistent with port being uint16_t
// borked, service level outages will get detected sooner | ||
// by kube-proxy. | ||
// DefaultHealthCheckInterval defines how frequently a probe runs | ||
DefaultHealthCheckInterval = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to 60 * time.Second, then the duration is more self-documenting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this would be desirable. However, the compute api takes int64 of seconds. If we made this a time.Duration, we would need to int64(DefaultHealthCheckInterval.Seconds())
which isn't ideal.
Should we settle for renaming to DefaultHealthCheckIntervalSeconds
?
// DefaultUnhealthyThreshold defines the threshold of failure probes that declare a backend "unhealthy" | ||
DefaultUnhealthyThreshold = 10 | ||
// DefaultTimeoutSeconds defines the timeout of each probe | ||
DefaultTimeoutSeconds = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
wantHC.RequestPath = h.defaultPath | ||
|
||
if existingHC.Protocol() != hc.Protocol() { | ||
glog.Infof("Updating health check %v because it has protocol %v but need %v", existingHC.Name, existingHC.Type, hc.Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.V(2)
// TODO: reconcile health checks, and compare headers interval etc. | ||
// Currently Ingress doesn't expose all the health check params | ||
// natively, so some users prefer to hand modify the check. | ||
glog.Infof("Unexpected request path on health check %v, has %v want %v, NOT reconciling", | ||
name, hc.RequestPath, wantHC.RequestPath) | ||
glog.Infof("Unexpected request path on health check %v, has %v want %v, NOT reconciling", hc.Name, existingHC.RequestPath, hc.RequestPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.V(2)
|
||
service = *as.(*api_v1.Service) | ||
for _, sp := range service.Spec.Ports { | ||
svcPort = sp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move down into the if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me explain my thought process on this styling choice. Ideally, svcPort
is a pointer so nil can represent not found. However, service.Spec.Ports[N]
is a struct which is why svcPort
is a struct. You cannot take the address of sp
because it's changing with every loop iteration. The other options are:
- Declare a new struct for each iteration and instantiate with value of sp, then take address of that variable if port is matched. Then you can check the pointer for existence, but you're not saving any lines of codes.
- Move
svcPort=sp
intoif
and check if ServicePort is a zero-value struct. Of course,if svcPort.Port != 0
is a bit of an eyesore.
IMO, the current implementation and first alternative are equally fine and a matter of preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
controllers/gce/backends/backends.go
Outdated
return "", err | ||
} | ||
if probe != nil { | ||
glog.V(1).Infof("Applying httpGet settings of readinessProbe to health check on port %+v", sp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V(2)
controllers/gce/backends/backends.go
Outdated
@@ -231,7 +286,7 @@ func (b *Backends) Add(port int64) error { | |||
// Delete deletes the Backend for the given port. | |||
func (b *Backends) Delete(port int64) (err error) { | |||
name := b.namer.BeName(port) | |||
glog.Infof("Deleting backend %v", name) | |||
glog.Infof("Deleting backend service %v", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.V(2)
controllers/gce/backends/backends.go
Outdated
len(igs), port, namedPort) | ||
be, err = b.create(igs, namedPort, b.namer.BeName(port)) | ||
glog.V(1).Infof("Creating backend for %d instance groups, port %v named port %v", len(igs), p.Port, namedPort) | ||
be, err = b.create(igs, namedPort, hcLink, p.Protocol, pName) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for be
to be nil when it gets to here? Is this going to be a problem for the defer'd function above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely possible be
could be added to the snapshot as nil
. However, this is as expected. I double-checked that all code fetching items from snapshotter
are looking at the key and not at the backend-service value.
controllers/gce/backends/backends.go
Outdated
} | ||
|
||
if be.Protocol != string(p.Protocol) || existingHCLink != hcLink { | ||
glog.Infof("Updating backend protocol %v (%v) for change in protocol (%v) or health check", pName, be.Protocol, string(p.Protocol)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.V(2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@nicksardo I see this is merged, how long until I see this in GKE? This will be quite relevant in our decision to either use the GCE load balancer or nginx for our soft launch. |
#609 warrants a release of the GLBC and version bump in the manifest file for all release branches. Could land in 1.6.3 or 1.6.4 and GKE usually updates on patches about a week or two after release. I'll ping you from the version bump PR so you can track. |
This feature was released in v0.9.3 which is included in K8s 1.6.3. @ibawt |
@nicksardo you are my favourite person in the world :O . 1.6.3 lines up nicely with our schedule |
@ibawt and others, Have any of you had a chance to use this feature yet? Any comments? Thanks |
@nicksardo yes, currently running HTTPS backends on two separate( read fairly large) deployments. I also tried to swap the service port, ie from 80 to 443 to indicate at least at a glance that it will be https, that part didn't work so hot. If I left it to 80 everything went fine, I even changed the targetPort. It looked something was doing an equality check, I was going deep dive into but haven't had a chance. So tl;dr works great, thanks a bunch. The HC updating shook some goblins out that we should have done correctly anyways. One thing I didn't find in the docs is what should I be using for the cert pair on the service backend? I ended up using the same cert as the ingress. Can we use different ones? How does the verification work? Can we use self signed if we provide the root? |
Enables proxying traffic to HTTPS k8s services.
Fixes #494 - Support HTTPS backend
Fixes #475 - RPS per instance instead of instance group
Requires:
Surfacing Service Annotation
Ideally, the service annotation parsing function is moved here: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/service/annotations.go . However, the
ingress
repo consumes theclient-go
api and k8s still consumes the local api. @.caesarxuchao mentions that all types will be moved out of k8s and client-go (kubernetes/kubernetes#44065) in a few weeks. He suggests we place the annotation function locally until things change.Overview of Changes
compute.HttpsHealthCheck
sHealthCheck
healthchecks
package and interface to separate the instantiation of a health check (New...
) from creating it in the cloud (`Add...).GCETranslator
presence fromhealthchecks
tobackends
healthchecks
backends
to create HTTPScompute.BackendServices
if encrypted=trueTest Cases