-
Notifications
You must be signed in to change notification settings - Fork 281
Add support for multiple Pub/Sub notification configurations #2135
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,8 @@ type Config struct { | |
| backend string | ||
| fsRoot string | ||
| event EventConfig | ||
| events eventConfigFlag | ||
| parsedEvents []notification.EventManagerOptions | ||
| bucketLocation string | ||
| LogLevel slog.Level | ||
| } | ||
|
|
@@ -60,6 +62,105 @@ type EventConfig struct { | |
| list []string | ||
| } | ||
|
|
||
| // eventConfigFlag is a flag.Value that accumulates repeated -event.config | ||
| // values. Each invocation of Set appends the raw string; validation and | ||
| // conversion into EventManagerOptions happens in Config.validate. | ||
| type eventConfigFlag []string | ||
|
|
||
| func (f *eventConfigFlag) String() string { | ||
| if f == nil { | ||
| return "" | ||
| } | ||
| return strings.Join(*f, ", ") | ||
| } | ||
|
|
||
| func (f *eventConfigFlag) Set(value string) error { | ||
| *f = append(*f, value) | ||
| return nil | ||
| } | ||
|
|
||
| // parseEventConfig parses a single -event.config value into an | ||
| // EventManagerOptions. The expected format is: | ||
| // | ||
| // bucket=<name>;project=<project>;topic=<topic>[;events=<e1>,<e2>][;prefix=<prefix>] | ||
| // | ||
| // bucket, project, and topic are required. events defaults to finalize if not | ||
| // specified. prefix is optional. | ||
| func parseEventConfig(raw string) (notification.EventManagerOptions, error) { | ||
| var opts notification.EventManagerOptions | ||
| var hasBucket, hasProject, hasTopic, hasEvents bool | ||
|
|
||
| for _, field := range strings.Split(raw, ";") { | ||
| parts := strings.SplitN(field, "=", 2) | ||
| if len(parts) != 2 { | ||
| return opts, fmt.Errorf("invalid event config field %q: expected key=value format", field) | ||
| } | ||
| key := strings.TrimSpace(parts[0]) | ||
| val := strings.TrimSpace(parts[1]) | ||
|
|
||
| switch key { | ||
| case "bucket": | ||
| opts.Bucket = val | ||
| hasBucket = true | ||
| case "project": | ||
| opts.ProjectID = val | ||
| hasProject = true | ||
| case "topic": | ||
| opts.TopicName = val | ||
| hasTopic = true | ||
| case "events": | ||
| notifyOn, err := eventListToNotifyOn(strings.Split(val, ",")) | ||
| if err != nil { | ||
| return opts, err | ||
| } | ||
| opts.NotifyOn = notifyOn | ||
| hasEvents = true | ||
| case "prefix": | ||
| opts.ObjectPrefix = val | ||
| default: | ||
| return opts, fmt.Errorf("unknown event config key %q", key) | ||
| } | ||
| } | ||
|
|
||
| if !hasBucket { | ||
| return opts, fmt.Errorf("event config missing required field \"bucket\"") | ||
| } | ||
| if !hasProject { | ||
| return opts, fmt.Errorf("event config missing required field \"project\"") | ||
| } | ||
| if !hasTopic { | ||
| return opts, fmt.Errorf("event config missing required field \"topic\"") | ||
| } | ||
| if !hasEvents { | ||
| opts.NotifyOn = notification.EventNotificationOptions{Finalize: true} | ||
| } | ||
| return opts, nil | ||
| } | ||
|
|
||
| // eventListToNotifyOn validates a list of event name strings and maps them to an | ||
| // EventNotificationOptions struct. | ||
| func eventListToNotifyOn(events []string) (notification.EventNotificationOptions, error) { | ||
| var notifyOn notification.EventNotificationOptions | ||
| if len(events) == 0 { | ||
| return notifyOn, fmt.Errorf("events list must not be empty") | ||
| } | ||
| for _, e := range events { | ||
| switch strings.TrimSpace(e) { | ||
| case eventFinalize: | ||
| notifyOn.Finalize = true | ||
| case eventDelete: | ||
| notifyOn.Delete = true | ||
| case eventMetadataUpdate: | ||
| notifyOn.MetadataUpdate = true | ||
| case eventArchive: | ||
| notifyOn.Archive = true | ||
| default: | ||
| return notifyOn, fmt.Errorf("%q is an invalid event", strings.TrimSpace(e)) | ||
| } | ||
| } | ||
| return notifyOn, nil | ||
| } | ||
|
|
||
| // Load parses the given arguments list and return a config object (and/or an | ||
| // error in case of failures). | ||
| func Load(args []string) (Config, error) { | ||
|
|
@@ -84,6 +185,7 @@ func Load(args []string) (Config, error) { | |
| fs.StringVar(&cfg.event.bucket, "event.bucket", "", "if not empty, only objects in this bucket will generate trigger events") | ||
| fs.StringVar(&cfg.event.prefix, "event.object-prefix", "", "if not empty, only objects having this prefix will generate trigger events") | ||
| fs.StringVar(&eventList, "event.list", eventFinalize, "comma separated list of events to publish on cloud function URl. Options are: finalize, delete, and metadataUpdate") | ||
| fs.Var(&cfg.events, "event.config", "notification configuration in the format: bucket=<bucket>;project=<project>;topic=<topic>[;events=<event1>,<event2>][;prefix=<prefix>]. Can be specified multiple times for multiple configurations. events defaults to finalize. Supported events: finalize, delete, metadataUpdate, archive") | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a warning or something like that when the user provides both flags? (like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can, though I think that might require more invasive changes (which I don't necessarily mind making) because there currently isn't a good way to differentiate between Would returning an error (e.g., in config validation) in this case instead of logging a warning and using the new flag's values be acceptable to you? Or would you prefer I add a warning to the existing approach? |
||
| fs.StringVar(&cfg.bucketLocation, "location", "US-CENTRAL1", "location for buckets") | ||
| fs.StringVar(&cfg.CertificateLocation, "cert-location", "", "location for server certificate") | ||
| fs.StringVar(&cfg.PrivateKeyLocation, "private-key-location", "", "location for private key") | ||
|
|
@@ -160,7 +262,19 @@ func (c *Config) validate() error { | |
| return fmt.Errorf("port-http %d is too high, maximum value is %d", c.PortHTTP, math.MaxUint16) | ||
| } | ||
|
|
||
| return c.event.validate() | ||
| if err := c.event.validate(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, raw := range c.events { | ||
| opts, err := parseEventConfig(raw) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| c.parsedEvents = append(c.parsedEvents, opts) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (c *EventConfig) validate() error { | ||
|
|
@@ -230,6 +344,7 @@ func (c *Config) ToFakeGcsOptions(logger *slog.Logger, scheme string) fakestorag | |
| AllowedCORSHeaders: c.allowedCORSHeaders, | ||
| Writer: &slogWriter{logger: logger, level: slog.LevelInfo}, | ||
| EventOptions: eventOptions, | ||
| EventConfigs: c.parsedEvents, | ||
| BucketsLocation: c.bucketLocation, | ||
| CertificateLocation: c.CertificateLocation, | ||
| PrivateKeyLocation: c.PrivateKeyLocation, | ||
|
|
||
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.
not super concerning given how fake-gcs-server is used, but
bucket=;project=;topic=passes validation because the has* booleans get set regardless of value content.NewPubsubEventManagerthen sees emptyProjectID/TopicNameand silently creates a no-op manager. Can you update the code to validate that those values are not empty?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! Should be resolved in a4601ab.