Skip to content

Refine bus monitor #88

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.

Sign up for GitHub

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

Merged
google-prow-robot merged 22 commits into from
Jun 25, 2018
Merged

Refine bus monitor #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
362dc9c
Allow montior handler funcs to return errors
scothis Jun 12, 2018
e17286f
Filter handler funcs for channels and subscriptions on the monitored bus
scothis Jun 12, 2018
a3eb63f
Requeue failed updates
scothis Jun 13, 2018
72d39ae
Return full Channel/Subscription not *Spec
scothis Jun 13, 2018
62bd4e8
Bus arguments exposed as envvars
scothis Jun 13, 2018
baeb541
Add volumes to bus
scothis Jun 14, 2018
cef2d3b
Cleanup race conditions
scothis Jun 15, 2018
612a8e8
Split hello sample channel and subscription into separate resources
scothis Jun 15, 2018
11ef741
Revert bus arguments, use envvar instead
scothis Jun 15, 2018
9f261d1
Remove length check
scothis Jun 15, 2018
a7449c1
Cleanup Argument/Parameter relationship
scothis Jun 15, 2018
0654496
Expose attributes directly to ProvisionFunc and SubscribeFunc
scothis Jun 15, 2018
a468a1e
Define subscription params on the bus
scothis Jun 18, 2018
fb308f7
Emit events processed resources
scothis Jun 19, 2018
aa822d6
Lookup target bus before processing informer queue
scothis Jun 19, 2018
ce5debd
Drop resourceKey, normalize resource types
scothis Jun 19, 2018
61d9f85
More type alignment
scothis Jun 19, 2018
886cdb5
Merge remote-tracking branch 'upstream/master' into bus-monitor
scothis Jun 19, 2018
c2e4f4f
Move informers into monitor
scothis Jun 19, 2018
cf17add
Merge remote-tracking branch 'upstream/master' into bus-monitor
scothis Jun 21, 2018
fe854e2
Fix unknown arguments check
scothis Jun 22, 2018
24e7f7a
Merge remote-tracking branch 'upstream/master' into bus-monitor
scothis Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion config/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ metadata:
rules:
- apiGroups: ["channels.knative.dev"]
resources: ["buses", "channels", "subscriptions"]
verbs: ["get", "watch", "list"]
verbs: ["get", "watch", "list"]
- apiGroups: [""]
resources: ["events"]
verbs: ["create", "patch"]
17 changes: 15 additions & 2 deletions pkg/apis/channels/v1alpha1/bus_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,27 @@ type Bus struct {
// BusSpec (what the user wants) for a bus
type BusSpec struct {

// Parameters configuration params for the bus
Parameters *[]Parameter `json:"parameters,omitempty"`
// Parameters exposed by the bus for channels and subscriptions
Parameters *BusParameters `json:"parameters,omitempty"`

// Provisioner container definition to manage channels on the bus.
Provisioner *kapi.Container `json:"provisioner,omitempty"`

// Dispatcher container definition to use for the bus data plane.
Dispatcher kapi.Container `json:"dispatcher"`

// Volumes to be mounted inside the provisioner or dispatcher containers
Copy link
Copy Markdown
Contributor

@vaikas vaikas Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it might be better to just use a podspec if you need volumes as well? Just worried that we'll end up there but it doesn't then look like a podspec?

Copy link
Copy Markdown
Contributor Author

@scothis scothis Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I considered using a PodSpec but didn't want to expose the service account since the bus needs to act like a controller with access to the k8s api server.

Perhaps I'm just being overly paranoid with attempting to apply least privilege access control. Instead we could document that from an access control perspective applying a Bus is equivalent to applying a Pod/Deployment.

Another option would be to use a PodSpec and then overwrite the service account with the account we provide, but that may be surprising to users.

Copy link
Copy Markdown
Contributor

@vaikas vaikas Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm just curious where it's controlled who the provisioner / dispatcher runs as in your model? Same as the launcher? If so, do you foresee there being more flexibility in being able to specify a different service account to run as? If it runs with a different service account, you can then control possibly tighter with RBAC rules.

Copy link
Copy Markdown
Contributor Author

@scothis scothis Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bus controller (which is already merged), will create a service account for each bus, and binds to a preconfigured cluster role.

Volumes *[]kapi.Volume `json:"volumes,omitempty"`
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this now, I'm wondering whether it makes sense to have a single bus controller which launches long-running Deployments for provisioning and dispatching, or to have multiple bus controllers which each reconcile a subset of the Buses in the system.

In particular, if there is a bus which natively implements an HTTP event transport but uses (for example) a StatefulSet rather than a Deployment, requiring a container -> Deployment in the Bus spec would introduce an extra forwarding hop to no benefit. (Similarly, if we could stamp auth on the outgoing traffic to Google PubSub using Istio, it's possible we could avoid needing to run any Delivery pods in-cluster. Similarly, if there is a Provisioner (e.g. the PubSub provisioner) which only needs to run a few small commands to do the provisioning, it seems more efficient to run a single controller shared across multiple buses (this is similar to OpenWhisk provisioning, IIRC).

If we moved the Provision/Dispatch to controller-specific configuration, I think this would look like a BusSettings *[]Argument, ChannelOptions *[]Parameter and SubscriptionOptions *[]Parameter, possibly including a service account. This would also be more amenable to alternate integrated implementations by various providers.

WDYT?

}

// BusParameters parameters exposed by the bus
type BusParameters struct {

// Channel configuration params for channels on the bus
Channel *[]Parameter `json:"channel,omitempty"`

// Subscription configuration params for subscriptions on the bus
Subscription *[]Parameter `json:"subscription,omitempty"`
}

// BusStatus (computed) for a bus
Copy link
Copy Markdown
Member

@evankanderson evankanderson Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably include at least some basic status such as "provisioning completed" for the Bus.

Copy link
Copy Markdown
Contributor Author

@scothis scothis Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I opened #103 to track and assigned it to myself.

Expand Down
5 changes: 1 addition & 4 deletions pkg/apis/channels/v1alpha1/channel_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,8 @@ type ChannelSpec struct {
// Name of the bus backing this channel (optional)
Bus string `json:"bus`

// Arguments configuration arguments for the bus
// Arguments configuration arguments for the channel
Arguments *[]Argument `json:"arguments,omitempty"`

// Parameters configuration params for the channel
Parameters *[]Parameter `json:"parameters,omitempty"`
}

// ChannelStatus (computed) for a channel
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/channels/v1alpha1/subscription_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type SubscriptionSpec struct {
// Name of the subscriber service
Subscriber string `json:"subscriber"`

// Arguments for the channel
// Arguments for the subscription
Arguments *[]Argument `json:"arguments,omitempty"`
}

Expand Down
76 changes: 58 additions & 18 deletions pkg/apis/channels/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading