Skip to content
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

[Proposal] Pod Injection Policy #254

Merged
merged 2 commits into from
Feb 23, 2017
Merged

[Proposal] Pod Injection Policy #254

merged 2 commits into from
Feb 23, 2017

Conversation

jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Jan 5, 2017

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2017
@pmorie pmorie self-assigned this Jan 10, 2017
@pmorie
Copy link
Member

pmorie commented Jan 11, 2017

I like the expanded example, looking forward to reading more on the areas we discussed offline that need to be fleshed out.

@derekwaynecarr
Copy link
Member

I like this concept. I ask that we annotate the resultant resource so we have a record that the user input was modified as a result of the ServiceInjectionPolicy. We do something similar with LimitRange and its a useful reminder to users about why the input they provided was modified. Thoughts?

@pmorie
Copy link
Member

pmorie commented Jan 11, 2017

We do something similar with LimitRange and its a useful reminder to users about why the input they provided was modified. Thoughts?

+1 to that.

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 11, 2017 via email

emptyDir: {}
```

Pod spec after admission controller:
Copy link
Member

Choose a reason for hiding this comment

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

this is confusing. i thought the SIP would modify resources with Kind: Pod and not any resource that embedded a pod template (whose list is unknowable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this seems reasonable will update

Copy link
Member

Choose a reason for hiding this comment

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

@derekwaynecarr Is correct.

However, that said, I think another useful example would be a SIP and a ReplicaSet with a pod template whose labels match it, and then the pod that was created, showing that the pod's spec was mutated by the SIP during admission.

`Env`, `EnvFrom`, `Volumes`, and `VolumeMounts` apply to the container spec for
all containers in the pod with the specified matching `LabelSelector`.

*Why all modify all containers in a pod?*
Copy link

Choose a reason for hiding this comment

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

s/Why all/Why/


type ServiceInjectionPolicySpec struct {
Service string # Reference to the service that is being injected
LabelSelector *unversioned.LabelSelector
Copy link

Choose a reason for hiding this comment

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

For consistency with what I'm seeing on https://kubernetes.io/docs/user-guide/labels/ should this be "Selector" instead of "LabelSelector" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

The above described the behavior for explicitly defining what should be
injected into a pod.

But a helper has been discussed to automatically fill in the
Copy link
Member

Choose a reason for hiding this comment

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

I would expand on this a bit and give a more fleshed out example.

Copy link
Member

Choose a reason for hiding this comment

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

WDYT of s/helper/controller/?

Copy link
Member

Choose a reason for hiding this comment

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

Is this something that would be part of the core kubernetes, or an add-on? If it is part of core k8s we should formalize the api - e.g. annotation and controller behavior.

spec:
replicas: 2
selector:
app: website
Copy link

Choose a reason for hiding this comment

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

My k8s newbie-ness might be showing, but if the selector is "app: website" shouldn't there be a label down below with "app: website" ?

emptyDir: {}
```

ReplicaSet after admission controller modified pod spec:
Copy link
Member

Choose a reason for hiding this comment

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

This should be a pod created from the replica set, rather than the replica set itself; non-pod resources won't be mutated.

list](https://groups.google.com/forum/#!topic/kubernetes-sig-node/gijxbYC7HT8).

Other solutions include basing the container to inject based off
matching it's name to another field in the `ServiceInjectionPolicy` spec, but
Copy link
Member

Choose a reason for hiding this comment

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

its

## Constraints and Assumptions

1. New mechanisms must be made to work with controllers such as deployments and
replicasets that create pods
Copy link
Member

Choose a reason for hiding this comment

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

I think some additional qualification is needed here, and what I had had in mind is that existing controllers that create generations of pods should be made to recreate pods when a new SIP is created that would alter them.

### ServiceInjectionPolicy API object

```go
type ServiceInjectionPolicy struct {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should spell out:

  1. Validation rules for SIP
  2. Which fields are mutable, if any


### AdmissionControl Plugin: ServiceInjectionPolicy

The **ServiceInjectionPolicy** plugin introspects all incoming pod requests and
Copy link
Member

Choose a reason for hiding this comment

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

requests for pod creation, since pods are (for the purposes of SIP) immutable

#### Behavior

This will modify the containers in the pod spec. The supported changes to
`Env`, `EnvFrom`, `Volumes`, and `VolumeMounts` apply to the container spec for
Copy link
Member

Choose a reason for hiding this comment

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

Volumes are pod spec, not container spec :)

this would not scale well and would cause annoyance with configuration
management.

The resultant modified spec will be annotated to show that it was modified by
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case it's the entire pod (since the spec alone does not have metadata); we might want to track down whether there are existing annotations used to carry this information.


### AdmissionControl Plugin: ServiceInjectionPolicy

The **ServiceInjectionPolicy** plugin introspects all incoming pod creation
Copy link
Member

Choose a reason for hiding this comment

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

We should define the order of precedence when you have multiple SIPs. Seems like it should be based on creation time, oldest applied first.

Copy link
Member

Choose a reason for hiding this comment

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

Ug. Good point. The issue with creation time is that the order can't be easily adjusted. Seems like a good default, but we may need to support something more tunable eventually.

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, we can launch a beta that does a consistent order to help with debugging and stipulate that all SIPs in a namespace must be order agnostic. It might be worth mentioning that we could do some conflict detection using merge keys.

This starts to get into similar areas as kubectl apply - e.g. multiple writers to the same object with potentially conflicting writes

resources you need. This would be similar to the functionality ConfigMaps and
Secrets.

## Examples
Copy link
Member

Choose a reason for hiding this comment

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

As @duglin requested, an example that shows consuming secrets/configmaps would be great.


type ServiceInjectionPolicySpec struct {
// Service is a reference to the service being injected (optional, immutable).
Service string
Copy link
Member

Choose a reason for hiding this comment

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

Based on the service-catalog SIG discussion, I think that we can take this field out for now.

Copy link
Member

Choose a reason for hiding this comment

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

@derekwaynecarr we chatted about some use-cases you had related to resource consumption -- are those solidified at all? We are trying to determine whether this is really a generic injection policy and not intrinsically related to services.

## Proposed Changes

### ServiceInjectionPolicy API object

Copy link
Member

Choose a reason for hiding this comment

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

Things to add:

  1. Which API group is this in? I have been told by a couple API machinery peeps that extensions is not a great place to continue adding resources. Should we have a new API group for this? cc @deads2k
  2. Is this proposal describing a resource we think is alpha, beta, or GA?

Copy link
Member

Choose a reason for hiding this comment

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

IMO what we are describing noew is alpha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha!

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a new API group for this? cc @deads2k

Yeah. The cost for choosing a bad group the same as putting it in extensions and the version would be wrong, so a separate group would always be preferred. This came for storageclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really like to see this as a separate API server and avoid adding new types to the kube core. An alpha API summarized by an alpha summarizer seems very reasonable. The initializers proposal would cover the admission: #132

Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k Do you know if initializers will be ready in 1.6?

They will miss 1.6, but the generic api server is usable now and we'll be finishing its split in a couple weeks. If we separate the API servers, we'll be able to have the desired API layout. Knowing that we want to move to initializers, a temporary admission plugin can be built to load the types you need. The additional complication is minimal and it positions the API well going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to alleviate concerns about difficulty, here's a pull that creates a complete API server for running a new group: kubernetes/kubernetes#40803 . It is compatible with kubectl: kubectl get serviceinjection

Copy link
Member

Choose a reason for hiding this comment

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

Relying on initializers would mean that we could not have a working prototype until 4 months from now. As I understand it, that is not an acceptable timeline. What alternatives can you suggest to release something on 1.6?

Copy link
Member

Choose a reason for hiding this comment

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

I think a temporary admission plugin:

a temporary admission plugin can be built to load the types you need

Copy link
Contributor

Choose a reason for hiding this comment

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

Relying on initializers would mean that we could not have a working prototype until 4 months from now. As I understand it, that is not an acceptable timeline. What alternatives can you suggest to release something on 1.6?

A temporary admission plugin can be built with a client that list/watches the resources you need.


### Loose coupling between services and their consumers

Using a Service Injection Policy allows pod authors to not have to explicitly
Copy link
Member

Choose a reason for hiding this comment

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

Why is loose coupling desireable?

Spec ServiceInjectionPolicySpec
}

type ServiceInjectionPolicySpec struct {
Copy link
Member

Choose a reason for hiding this comment

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

I'm of the mind that we should make this immutable. Mutable fields introduce a ton of corner cases into any piece of the system that deals with this resource. As an example, if we make the resource mutable, does that mean we need to ripple updates to the SIP spec in a service catalog binding into the created SIP? I would rather not deal with that.

Also, it's possible to make an immutable field mutable and impossible to make a mutable field immutable from a backward-compatibility standpoint, so we could start with immutable and make fields mutable in the future if it makes sense to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

Copy link

Choose a reason for hiding this comment

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

EQ: is this just a documentation thing or is there some kind of tag we add to the resource/properties that allows for our api server to handle/check this for us?

// VolumeMounts defines the collection of VolumeMount to inject (optional, mutable).
VolumeMounts []VolumeMount
}
```
Copy link
Member

Choose a reason for hiding this comment

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

I would do a subsection just devoted to validations


## Constraints and Assumptions

1. New mechanisms must be made to work with controllers such as deployments and
Copy link
Member

Choose a reason for hiding this comment

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

Existing controllers that create pods should recreate their pods when a new Service Injection Policy is added that would effect them.

I am not sure this is always true. Imagine a new SIP is created that targets all Pods, this would then restart all Pods in the namespace. The impact could be limited by adding a PodDisruptionBudget, but that part of the design would need to be further discussed. May be we should defer this and leave it as a possible future work.


## Use Cases

1. As a user, I want to be able to describe a way that pods should be injected
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is a mouthful ;)


### Loose coupling between services and their consumers

Using a Service Injection Policy allows pod authors to not have to explicitly
Copy link
Member

Choose a reason for hiding this comment

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

s/pod/pod or podtemplate/

@pmorie
Copy link
Member

pmorie commented Feb 22, 2017

I totally get the ServiceBroker idea. The part of this that rubs me wrong is the implicitness of it. If I want to use my CloudSpanner instance, I should really say so. It shouldn't just happen magically.

If you don't want to use it, it's entirely optional. If you don't use it, you'll have to reference everything in every pod spec you want to consume it in.

Signed-off-by: Jess Frazelle <acidburn@google.com>
@jessfraz
Copy link
Contributor Author

everything has been updated PTAL, thanks!

@pwittrock
Copy link
Member

@thockin

I totally get the ServiceBroker idea. The part of this that rubs me wrong is the implicitness of it. If I want to use my CloudSpanner instance, I should really say so. It shouldn't just happen magically.

+1 #176 Is a proposal to offer something more along these lines.

@pmorie
Copy link
Member

pmorie commented Feb 23, 2017

@thockin

I totally get the ServiceBroker idea. The part of this that rubs me wrong is the implicitness of it. If I want to use my CloudSpanner instance, I should really say so. It shouldn't just happen magically.

In the pre-kubernetes versions of OpenShift, we had a feature for our image-analog that was a more limited version of PIP. We found that people really like not having to worry about writing links explicitly and just having them injected. As Brian has articulated very well, having implicit mechanisms reduces the complexity of descriptors users write a great deal.

I swear that we have had this discussion before and I won you over. I cannot find it now.

Going back to your concern:

If I want to use my CloudSpanner instance, I should really say so. It shouldn't just happen magically.

We can have an escape hatch annotation that you can place on a pod to exempt it from being acted on by PIP. That way, if you have a workload you absolutely positively do not want to be subject to this type of mutation, you can express that.

I do and don't agree with the spooky at a distance analogy. Yes, it is action at a distance, but so is everything any admission controller does. For example, the service account token volume is basically a subset of this idea.

@bgrant0607 @smarterclayton @thockin I would really like to have this proposal accepted so that we can publish something alpha in 1.6. Does adding the escape hatch annotation make it more palatable? This is a very important part of the story for service catalog and we have invested quite a bit of time on the concept. I would like to have folks begin using this at 1.6 so we can incorporate feedback into a more mature version in the 1.7 timeframe.

@thockin
Copy link
Member

thockin commented Feb 23, 2017 via email

@derekwaynecarr
Copy link
Member

@thockin - this does mutate the resultant pod spec upon creation and records via annotation that it did it.

@pmorie
Copy link
Member

pmorie commented Feb 23, 2017

If I understand correctly this is not mutating the pod specs,
just the runtime behavior.

It does mutate - see the examples section. I am going to merge - we can update the proposal to refine articulation of the motivation if necessary.

Thanks a lot for this work @jessfraz !

@pmorie pmorie merged commit 2b56d8b into kubernetes:master Feb 23, 2017
@jessfraz jessfraz deleted the sip branch February 23, 2017 06:34
@erictune
Copy link
Member

erictune commented Feb 23, 2017 via email

@pmorie
Copy link
Member

pmorie commented Feb 23, 2017

@erictune How about an 'injection' group? I get that we don't want a ton of single-resource groups, but when there's no fitting group, it seems like we should create a new one.

@erictune
Copy link
Member

Some thoughts on naming:

  • The word "Policy" makes me think about something that says what you cannot do. This is more about setting values you don't know you need to set. "Environment" and "Defaults" seem like more appropriate terms to use.
  • "PodInjection" sounds like it injects pods, which it doesn't.
  • What about "PodDefaults" or "PodEnvironment".

@jessfraz
Copy link
Contributor Author

Defaults kinda makes it seem like it would be a "default" tho

@bgrant0607
Copy link
Member

I agree with @erictune that ServiceInjection made more sense than PodInjection. I called this configurable defaulting in kubernetes/kubernetes#17097 -- it's selector-scoped default settings for pods. How about PodSettings, PodProperties, PodAttributes, or the like?

As for group, I agree with @pmorie that we do already have too many groups. Why does ImagePolicy have its own group? I asked PodDisruptionBudget to use a "policy" group with the expectation that other policies would go there.

We could create a "settings" group for APIs like PIP, LimitRange, and MetadataPolicy that modify other resources in a straightforward way (as opposed to autoscaling, which is reactive/predictive).

@jessfraz
Copy link
Contributor Author

so should we vote?

@jessfraz jessfraz mentioned this pull request Feb 23, 2017
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Mar 2, 2017
Automatic merge from submit-queue (batch tested with PRs 41931, 39821, 41841, 42197, 42195)

Admission Controller: Add Pod Preset

Based off the proposal in kubernetes/community#254

cc @pmorie @pwittrock 

TODO:
- [ ] tests



**What this PR does / why we need it**: Implements the Pod Injection Policy admission controller

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
Added new Api `PodPreset` to enable defining cross-cutting injection of Volumes and Environment into Pods.
```
@tamalsaha
Copy link
Member

@jessfraz , is there any chance that this will also allow adding side car container on the fly in future?

@pmorie
Copy link
Member

pmorie commented Mar 7, 2017

@tamalsaha containers and init containers are definitely on the table

@tamalsaha
Copy link
Member

@pmorie good to know. Any chance that will make in 1.6? Also will the extensible admission controller feature make in 1.6?

@pwittrock
Copy link
Member

@tamalsaha Definitely not in 1.6.

ruebenramirez pushed a commit to ruebenramirez/community that referenced this pull request Apr 22, 2017
[Proposal] Pod Injection Policy
shyamjvs pushed a commit to shyamjvs/community that referenced this pull request Sep 22, 2017
…s-secondary-for-1.7-release

Propose Caleb Miles as member of 1.7 release team
MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021
[Proposal] Pod Injection Policy
danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet