GeneratedNames: fix generated names not being RFC 1123 compliant #2800
Conversation
knative-prow-robot
added
the
size/M
|
Welcome @odacremolbap! It looks like this is your first PR to knative/eventing 🎉 |
knative-prow-robot
added
the
needs-ok-to-test
|
Hi @odacremolbap. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @evankanderson |
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
|
The following is the coverage report on the affected files.
|
|
/retest |
| @@ -116,15 +116,20 @@ func ToDNS1123Subdomain(name string) string { | |||
| // The name's length will be short enough to be valid for K8s Services. | |||
| func GenerateFixedName(owner metav1.Object, prefix string) string { | |||
There was a problem hiding this comment.
This method should be deleted and use https://github.com/knative/pkg/blob/master/kmeta/names.go#L39
There was a problem hiding this comment.
thanks @n3wscott
GenerateFixedName is used by
- EventType
- APIServerSource ReceiveAdapter
- Subscriptions
- PingSource ReceiveAdapter
I'll broaden the scope of this PR to exceed the initial Trigger-Subscription issue and replace all uses of GenerateFixedName
There was a problem hiding this comment.
We need to be careful when changing how names are generated. Will kmeta.ChildName always generate the same result as GenerateFixedName?
There was a problem hiding this comment.
Yes, the point of kmeta.ChildName is to do just this and be dns compliant. it is used in serving to make revisions. @vagababov
There was a problem hiding this comment.
I think is slightly different. So you need to be certain it's backward compatible, but our names are guaranteed valid k8s object names (which implies DNS valid name).
There was a problem hiding this comment.
What is the scope of the backward-incompatible change if we don't drop GenerateFixedName, and make only the suggested changes in this PR instead? Do we need a migration strategy for that too?
There was a problem hiding this comment.
For all cases it looks for an object by the generated name, and if it doesn't exists it will create it.
Users would end up with duplicated deployments (receive adapters) and subscriptions.
We don't need a migration strategy merging this PR, it is backwards compatible but for cases where current implementation generates invalid DNS names and fails.
My take on this would be merging this, it is fixing current issue, no matter if we get rid of GenerateFixedName. Then open a follow up issue, to discuss how to remove GeneratedFixedNames.
There was a problem hiding this comment.
The difference between these cases and serving though is that the listed cases:
EventType
APIServerSource ReceiveAdapter
Subscriptions
PingSource ReceiveAdapter
Do not use the generated name to do name resolution. If you do 2. it seems like it will be fine operationally with a big gotchya: we use that name to query the api server for the resource the controller is attempting to make and if we find it and don't own it, then it is an error. If we find it, we update. If we don't find it we create it. The last point will cause duplicates of all sub-resources.
How do you like 3. with a twist: query with ChildName first, if not found, query for GenerateFixedName. If found with GenerateFixedName then delete it and treat is as the same case as not found from ChildName.
This will cause a moment of downtime and the subscriptions will drop state if they have it...
There was a problem hiding this comment.
That's the pattern I was thinking for case 3, there is no way of changing object meta without re-creating so that's it.
At first there doesn't seem to be an easy way of abstracting that double checks for GeneratedFixedName and ChildName existence but I'll try to come up with the a solution that doesn't spread much code around. I'll work on a different PR and park/remove this one if that's ok with you.
There was a problem hiding this comment.
My take on this would be merging this, it is fixing current issue, no matter if we get rid of GenerateFixedName. Then open a follow up issue, to discuss how to remove GeneratedFixedNames.
I agree, if there's no backward incompatible change here, then deprecating GenerateFixedName is out of scope for this PR. I created #2842 to track the deprecation. Please continue the conversation there :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grantr, odacremolbap 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 |
knative-prow-robot
added
the
approved
|
/retest |
Fixes #2753
Regarding the original issue kubernetes names are DNS-1123 compliant so are Trigger names, no need to add extra validation.
This PR removes any dot that will be prefixing to the
dash + UIDstring to avoid the issue.Although double dashes are DNS-1123 compliant they do not look pretty; the PR also avoids adding a dash to the concatenation when the prefix already begins with one.
Release Note