Re: [kubernetes/community] Proposal: optional service links (#1249)
Brian Grant
cc @kubernetes/sig-api-machinery-api-reviews
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Brian Grant
We may need a special case for just the apiserver-related variables.
Andy Goldstein
@ncdc commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> + +Possible issues with injected service links are + +* Accidental coupling. +* Incompatibilities with container images that no longer utilize service links and explicitly fail at startup time if certain service links are defined. +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes
typo: interfere
Tim Hockin
are by this proposal). The question is whether we want a parallael flag
for them. But for now, I'll argue not.
On Thu, Oct 26, 2017 at 1:32 PM, Brian Grant <notifi...@github.com>
wrote:
> We may need a special case for just the apiserver-related variables.
>
> —
> Reply to this email directly, view it on GitHub
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFVgVKs63Mzad3lk3nApnt641Fkyn_nTks5swOxugaJpZM4QBrHD>
> .
Jonas Kongslund
@kongslund commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.
No objections.
Jonas Kongslund
Thanks for comments so far. Any other remarks?
Michael Grosser
@stp-ip commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +
+In `kubelet_pods.go`, the value of that field is passed along to the function `getServiceEnvVarMap` where it is used to decide which selector should be used for the `serviceLister`. Current behavior is `labels.Everything()`. In case `disableServiceLinks` is true then only the `kubernetes` service in the `kl.masterServiceNamespace` should be injected.
+
+```
+func (kl *Kubelet) getServiceEnvVarMap(ns string, disableServiceLinks bool) (map[string]string, error) {
+ ...decide on selector
+}
+```
+
+### Client/Server Backwards/Forwards compatibility
+
+Pods that do not have the field set will assume a value of false.
+
+## Alternatives considered
+
+An alternative is to add support for explicit service links, e.g. by applying a label selector map with a default behavior of including everything.
Starting with a way to disable the current behavior and considering extending via selector typ behavior might be nice, but definitely needs a few specific examples, where that might be necessary from a k8s perspective.
Tim Hockin
@kongslund I think you're on-target. You have a week before code freeze for 1.9 :)
Jonas Kongslund
Thanks for heads up, @thockin.
So does everyone think that a boolean flag is sufficient for now?
Tim Hockin
I think that's fine. Let's get it working so we can take a concrete look at it. Deltas from there are not so terrible to contemplate.
Clayton Coleman
@smarterclayton commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false.
The field should be a pointer to a boolean, default to true if nil, and have a positive value (enableServiceEnvVars).
Clayton Coleman
@smarterclayton commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfer with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `disableServiceLinks` of type boolean. Default value is false. + +In `kubelet_pods.go`, the value of that field is passed along to the function `getServiceEnvVarMap` where it is used to decide which selector should be used for the `serviceLister`. Current behavior is `labels.Everything()`. In case `disableServiceLinks` is true then only the `kubernetes` service in the `kl.masterServiceNamespace` should be injected.
If this omits KUBERNETES_SERVICE_HOST then in cluster config will break. That impacts sidecars like istio.
It’s not clear to me that we should omit those variables. If we do, we may have to make this a container field, not pod field.
Tim Hockin
Is this abandoned?
Michal Rostecki
Does that proposal need to be merged before I continue the work on my implementation? If yes, @kongslund, are you up to finishing the work on proposal?
I'm sorry that I started the implementation, but I didn't see the proposal before and I just started working on the issue...
Jonas Kongslund
@mrostecki The proposal is a starting point and already has good feedback from the community. It does not need to be merged before you can provide an implementation but you should incorporate the contents of the proposal or start a discussion if you think something should be different.
Jonas Kongslund
@kongslund pushed 1 commit.
- f9237cf Incorporated feedback in the comments
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: kongslund
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton
Assign the PR to them by writing /assign @smarterclayton in a comment when ready.
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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Brian Grant
Brian Grant
@bgrant0607 commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
This may need to be disableServiceLinks, so that the existing behavior would be in effect if it were the zero value.
cc @erictune
Jonas Kongslund
@kongslund commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
Either is fine with me. However, there seems to be a preference for enable instead of disable cf. #1249 (comment)
Clayton Coleman
@smarterclayton commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
Brian, as a pointer zero value would be interpreted as existing behavior by older clients - can you articulate a bit more what you’d be concerned about as a bool ptr?
Michal Rostecki
@mrostecki commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
Even if we go with enable, we can set its default value to true, so I don't see any backwards compatibility issues there.
Brian Grant
Brian Grant
Brian Grant
@bgrant0607 commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
Sorry for delay. I do want this to happen.
If we make it a pointer and omitempty, will both true and false values be explicitly encoded as long as the field is non-null? I want to ensure that podspecs without the field can be unambiguously interpreted.
Also, doing this will add a field to every Pod and every pod template rendered. It will change podspec hashes, but that should be ok. It will make all pods more verbose, but maybe more users will become aware of the field and use it.
If we could, it would be great to not apply the default in pod templates. In addition to not changing hashes, this would make it feasible to set the field via admission control.
Clayton Coleman
@smarterclayton commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
Now that we use versioned clients leaving fields unset on the wire has fewer downsides - a client already has to test existence, so not applying the default on the apiserver is reasonable (effectively how runAsUser behaves). We definitely want to define the default, but we don’t have to fill it except at point of use. The biggest risk is clients inconsistently interpreting the default, which our current “always default” strategy minimizes. But as the apply discussion shows, it’s not necessarily an advantage for end users.
Maybe explicit defaulting is something a client should be able to request from the server, although we could only really change that behavior for existing fields until a v2.
Daniel Smith
@lavalamp commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
I don't think that's the case, to what in the apply discussion are you referring?
Client-applied defaults are ~impossible to ever change in our system, even over version boundaries.
Brian Grant
@bgrant0607 commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
I still think that defaults explicitly manifested by the server is the right thing. It just needs to be done where the resources are actually reified, not in templates.
DuJun
I meet the same issue when try to create more than 6000 services in my environment - Pod failed to start due to "too long parameters". I don't think this "one size fits all" approach works for me because if we disable the service ENV injection, users may have no way to know some service-related environment variables, e.g. SERVICE_PORT.
Michal Rostecki
@mrostecki commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
So, do we have some agreement there? Are we OK with the field named disableServiceLinks, pointer, omitempty, with the default false value on the server side? That seems to address @bgrant0607's doubts - it will preserve the old behavior by default and would not require to define that field.
Tim Hockin
@m1093782566 SRV DNS exists for that, if you don't know the port number.
In theory we could add yet another downward API here, but I'd need to see a serious use case for it.
Tim Hockin
@thockin commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
enable is probably a better API. I think that's what Brian was saying - pointers make this possible.
Any serialized form of an object which has this value specified will retain that value. Anything that has this value omitted will get nil which will be defaulted to whatever the versioned default is.
Right?
So if you serialize it in v1, when you read it back it will deserialize and default. If you write it back as v2, that v1 default is still retained...
Am I missing something?
Tim Hockin
I still very much want to see this fixed, if you want to carry it forward @m1093782566
DuJun
SRV DNS exists for that, if you don't know the port number.
Yeah, DNS SRV record works for me.
If we reach the agreement about the proposal, I can carry it forward. I can send a PR if no one has objections...
Tim Hockin
I feel like we sort of agreed on a single bool to enable service env which defaults to true.
It looks like @mrostecki is awake on https://github.com/kubernetes/kubernetes/pull/60206/files so maybe this is almost done already?
Brian Grant
I would still like to get this proposal merged, to capture the discussion.
Tim Hockin
owner :)
On Fri, Aug 24, 2018 at 11:51 AM Brian Grant <notifi...@github.com>
wrote:
> I would still like to get this proposal merged, to capture the discussion.
>
> —
> Reply to this email directly, view it on GitHub
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFVgVEu_g4DbwuSK5AqNDXupwJmsSGQ3ks5uUEsYgaJpZM4QBrHD>
Michal Rostecki
@mrostecki commented on this pull request.
In contributors/design-proposals/node/optional-service-links.md:
> +* Performance penalty in starting up pods [for namespaces with many services](https://github.com/kubernetes/kubernetes/issues/1768#issuecomment-330778184). + +## Proposal + +Make it possible for a user to disable injection of service links into containers of a pod by adding a disable flag to the pod's spec. Make the default value false in order to stay backwards compatible with the v1 API. Make an exception for the `kubernetes` service in the master namespace so that it will always get injected. + +## User Experience + +### Use Cases + +* As a user, I want to be able to disable service link injection since the injected environment variables interfere with a Docker image that I am trying to run on Kubernetes +* As a user, I want to be able to disable service link injection since I don't need it and it takes increasingly longer time to start pods as services are added to the namespace. + +## Implementation + +`PodSpec` is extended with an additional field, `enableServiceLinks`. The field should be a pointer to a boolean and default to true if nil.
OK, I agree. Let's go with enable. I will change that in my implementation.
bradhoekstra
This feature has been merged in k/k: kubernetes/kubernetes#68754
Tim Hockin
@bradhoekstra - could you take the contents here and update them as a KEP? We should have done that in the first place...
Then we can close this
bradhoekstra
KEP PR created here: #2709
bradhoekstra
The KEP for this feature is merged.
/close
k8s-ci-robot
@bradhoekstra: You can't close an active issue/PR unless you authored it or you are a collaborator.
In response to this:
The KEP for this feature is merged.
/close
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.
Tim Hockin
Closed #1249.