Generate unique, stable pubsub topic and subscription IDs#458
Conversation
With the gcppubsub bus it was possible to create conflicting topics and subscription if two (or more) clusters used the same Bus, Channel and Subscription names. This would result in the misdelivery of messages between the clusters. Now the UID of the Bus/ClusterBus that owns the topic/subscription is a factor in the identifier. The Bus UID will be stable durring The lifetime of the Bus, but unique between individual resources. Fixes knative#253
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: scothis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
knative-prow-robot
added
the
size/M
| } | ||
|
|
||
| bus := &CloudPubSubBus{ | ||
| ref: ref, |
There was a problem hiding this comment.
what is ref in this context? It looks like it is being passed in but I would assume a ref to a bus would point to the object that is being created with &CloudPubSubBus{...}
|
|
||
| func (b *CloudPubSubBus) topicID(channel buses.ChannelReference) string { | ||
| return fmt.Sprintf("channel-%s-%s-%s", b.ref.Name, channel.Namespace, channel.Name) | ||
| return fmt.Sprintf("channel-%s-%s-%s", channel.Namespace, channel.Name, b.id) |
There was a problem hiding this comment.
is it intended to flip the order of this? it use to be general bus to specific channel.
| os.Getenv("BUS_NAME"), | ||
| os.Getenv("BUS_NAMESPACE"), | ||
| ) | ||
| busUID := os.Getenv("BUS_UID") |
There was a problem hiding this comment.
what happens if there is no BUS_UID?
|
|
||
| // busId generates a short unique identifier for a bus from the Bus's UID. | ||
| // This value is used as a component of the topic and subscription IDs | ||
| func busId(uid string) string { |
There was a problem hiding this comment.
There is no test added for this method. Please add a test to make sure this does not make the same busID for similar UIDs
|
|
||
| // busId generates a short unique identifier for a bus from the Bus's UID. | ||
| // This value is used as a component of the topic and subscription IDs | ||
| func busId(uid string) string { |
There was a problem hiding this comment.
In go style, busId should be busID
|
|
||
| bus := &CloudPubSubBus{ | ||
| ref: ref, | ||
| id: busId(busUID), |
There was a problem hiding this comment.
rather than storing the id like this I wonder if it might be cleaner to set a topic prefix field that is generated from the id and our special strings (like channel- and then topicID becomes
func (b *CloudPubSubBus) topicID(channel buses.ChannelReference) string {
return fmt.Sprintf("%s-%s-%s", b.topicPrefix, channel.Namespace, channel.Name)
}
And then we are free to change how we generate that prefix, and the code does not have to change each time.
There was a problem hiding this comment.
It seems like we should store the generated UID in the status so we can recover it in the future.
Additionally, storing the topic ID means that we don't get thrown off later if we change the busID function.
Since this is the old Bus code, it's probably not worth updating the status to support this, but we should probably include a runtime.RawExtension in the status of the new Channel to support recording controller choices like this.
|
@scothis I think this should be closed; all files that are touched in this PR got nuked 🔥 |
|
/close |
|
@matzew: Closing this PR. DetailsIn response to this:
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. |
remove unneeded patch
…knative#458) * Fix reporting index of the failed step The warning had a wrong index: {"level":"info","ts":"2023-11-13T14:00:03.816770975Z","caller":"sender/services.go:207","msg":"Sending step event #16691 to \"http://sut-kn-channel.eventing-e2e0.svc.cluster.local\""} {"level":"warn","ts":"2023-11-13T14:00:03.821102525Z","caller":"sender/services.go:102","msg":"Could not send step event 16690, retrying (1): Post \"http://sut-kn-channel.eventing-e2e0.svc.cluster.local\": dial tcp 172.30.99.91:80: connect: connection refused"} * Upgrade tests account for last event being interrupted Co-authored-by: Martin Gencur <mgencur@redhat.com>
5 participants
With the gcppubsub bus it was possible to create conflicting topics and
subscription if two (or more) clusters used the same Bus, Channel and
Subscription names. This would result in the misdelivery of messages
between the clusters.
Now the UID of the Bus/ClusterBus that owns the topic/subscription is a
factor in the identifier. The Bus UID will be stable durring The
lifetime of the Bus, but unique between individual resources.
Fixes #253
Release Note