Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dynamically secure RouteGroups using Kubernetes TLS Secrets #2814
Changes from all commits
93f2431
c71636b
c358be5
4553fe7
5c5bca5
5e99692
99ac3bf
4aca418
f175dd0
bccbca3
2b529a3
ded3e16
04e8061
0daaddf
73aac01
090e10b
5795c51
9bf60a0
31348fc
d9671ac
2ceba75
0e82071
6750482
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ofctx.routeGroup.Spec.UniqueHosts()
.compareStringList
returns all items fromtls.Hosts
that are also part ofctx.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:
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