-
Notifications
You must be signed in to change notification settings - Fork 346
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
Dynamically secure RouteGroups using Kubernetes TLS Secrets #2814
Conversation
57c00c5
to
8059893
Compare
// and adds the TLS secret to the registry if a match is found. | ||
func (r *routeGroups) addRouteGroupTLS(ctx *routeGroupContext, tls *definitions.RouteTLSSpec) { | ||
// Host in the tls section need to explicitly match the host in the RouteGroup | ||
hostlist := compareStringList(tls.Hosts, ctx.routeGroup.Spec.UniqueHosts()) |
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 think what we want here is to check if tls.Hosts
is a subset of ctx.routeGroup.Spec.UniqueHosts()
.
compareStringList
returns all items from tls.Hosts
that are also part of ctx.routeGroup.Spec.UniqueHosts()
.
So it reduces the hostlist.
Did you do this by intention and I miss something?
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.
We only want to add certs which have a match from from tls.Hosts and the routeGroup hosts, so reducing the list is desired. If there are no matches we log and return, but if there are matches, we use the reduced list to add the matching tls certs to the registry.
We could log the non-matching hosts in tls.hosts to help debugging a miss configuration. What do you think?
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 logging makes sense.
I wonder in case of an update that is applied wrong, what we want to have as behavior. For example a timeline like this:
- rg foo has tls.hosts foo.example, bar.example and there is a referenced secret that has only these two names foo.example, bar.example as hosts in SAN
- rg was updated to add baz.example and remove bar.example in tls.hosts
The current behavior would drop bar.example.
Should this better stop updating it, such that bar.example is not replaced by baz.example ?!
I am thinking of migrations, but can not come up with an idea how this could break less with either case, so from my side we can also just log it.
@AlexanderYastrebov wdyt?
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 this should be a part of validator and invalid routegroup should be rejected. We already use validator in webhook and dataclient.
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.
The same is for Ingress - we plan to add ingress validator to dataclient, see #2757
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 this something we could handle in a separate PR to keep the scope limited? Happy to work on getting in validator for both Ingress and RouteGroup.
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.
Lets have it like it is now (log mismatch) and add tls validation for ingress and routegroup as a future improvement (makes sense after we land #2757).
@MustafaSaber FYI
dataclients/kubernetes/testdata/ingressV1/tls/tls-no-secret.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
TLS secrets is optional in the IngressV1 spec, using similar config for RouteGroups. Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
72b728d
to
0daaddf
Compare
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
f385e7d
to
73aac01
Compare
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
This is the error:
Just pass certregistry.CertRegistry as pointer in ingress and routegroup case |
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
// and adds the TLS secret to the registry if a match is found. | ||
func (r *routeGroups) addRouteGroupTLS(ctx *routeGroupContext, tls *definitions.RouteTLSSpec) { | ||
// Host in the tls section need to explicitly match the host in the RouteGroup | ||
hostlist := compareStringList(tls.Hosts, ctx.routeGroup.Spec.UniqueHosts()) |
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 this should be a part of validator and invalid routegroup should be rejected. We already use validator in webhook and dataclient.
dataclients/kubernetes/testdata/ingressV1/tls/tls-host-mismatch.log
Outdated
Show resolved
Hide resolved
dataclients/kubernetes/testdata/ingressV1/tls/tls-missing-host.log
Outdated
Show resolved
Hide resolved
// and adds the TLS secret to the registry if a match is found. | ||
func (r *routeGroups) addRouteGroupTLS(ctx *routeGroupContext, tls *definitions.RouteTLSSpec) { | ||
// Host in the tls section need to explicitly match the host in the RouteGroup | ||
hostlist := compareStringList(tls.Hosts, ctx.routeGroup.Spec.UniqueHosts()) |
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.
The same is for Ingress - we plan to add ingress validator to dataclient, see #2757
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
dataclients/kubernetes/testdata/routegroups/tls/tls-missing-host.log
Outdated
Show resolved
Hide resolved
Please add missing newlines at the end of test fixtures #2814 (comment) and fix log level #2814 (comment) LGTM but I think we should land szuecs/routegroup-client#16 first. |
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
Why? |
But we generate CRD there. I think we should settle all changes in the client, then copy CRD here, then we can land client PR. |
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
1dc62ce
to
6750482
Compare
👍 |
1 similar comment
👍 |
thanks for your work! |
I tagged the merged PR commit with https://github.com/szuecs/routegroup-client/releases/tag/v0.23.0 |
See changes * szuecs/routegroup-client#31 * szuecs/routegroup-client#32 It restores a link to the origin removed by #2814 to make it clear that CRD is the copy. Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
See changes * szuecs/routegroup-client#31 * szuecs/routegroup-client#32 It restores a link to the origin removed by #2814 to make it clear that CRD is the copy. Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Closes #2811