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

ContainerPort protocol is a key field, which makes it required for server-side apply #130

Closed
bouk opened this issue Nov 27, 2019 · 29 comments
Assignees

Comments

@bouk
Copy link

bouk commented Nov 27, 2019

Hi,

I was playing around with server-side apply and hit into a problem. Say I have a pod like this (very reduced case of course):

# pod.yml
apiVersion: v1
kind: Pod
metadata:
  name: test
spec:
  containers:
  - name: test 
    ports:
    - containerPort: 1234

If I run kubectl apply -f pod.yml --server-side I get the following error:

Error from server: failed to create typed patch object: .spec.containers[name="test"].ports: element 0: associative list with keys has an element that omits key field "protocol"

This is unexpected, because ContainerPort specifies the protocol as an optional field, which defaults to TCP. I traced this error back to this repository, so I figured this is where it could be resolved.

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.2", GitCommit:"c97fe5036ef3df2967d086711e6c0c405941e14b", GitTreeState:"clean", BuildDate:"2019-10-15T19:18:23Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.3", GitCommit:"b3cbbae08ec52a7fc73d334838e18d17e8512749", GitTreeState:"clean", BuildDate:"2019-11-16T01:01:59Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}
@bouk
Copy link
Author

bouk commented Nov 28, 2019

I think keeping this restriction in will hamper the adoption of server-side applies greatly, for example the ingress-nginx config also misses the 'protocol' ports in its config: https://github.com/kubernetes/ingress-nginx/blob/master/deploy/static/provider/cloud-generic.yaml

cc @lavalamp

bouk added a commit to bouk/ingress-nginx that referenced this issue Nov 28, 2019
kubectl apply --server-side currently doesn't work with Port specs that
are missing protocol:
kubernetes-sigs/structured-merge-diff#130 so
we should always specify it.
@apelisse
Copy link
Contributor

apelisse commented Dec 2, 2019

Thanks @bouk, we're aware of this bug and we do want to fix it, thanks.

We have #83 that partially fixes it (still trying to figure out the best solution).

/assign @jennybuckley

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 31, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/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.

@apelisse
Copy link
Contributor

apelisse commented May 1, 2020

/reopen

We care about this more than ever.

@k8s-ci-robot k8s-ci-robot reopened this May 1, 2020
@k8s-ci-robot
Copy link
Contributor

@apelisse: Reopened this issue.

In response to this:

/reopen

We care about this more than ever.

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.

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/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.

@bouk
Copy link
Author

bouk commented May 31, 2020

/reopen

@k8s-ci-robot k8s-ci-robot reopened this May 31, 2020
@k8s-ci-robot
Copy link
Contributor

@bouk: Reopened this issue.

In response to this:

/reopen

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.

towolf pushed a commit to towolf/ingress-nginx that referenced this issue Jun 16, 2020
kubectl apply --server-side currently doesn't work with Port specs that
are missing protocol:
kubernetes-sigs/structured-merge-diff#130 so
we should always specify it.
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/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.

@bouk
Copy link
Author

bouk commented Jun 30, 2020

/reopen

@k8s-ci-robot
Copy link
Contributor

@bouk: Reopened this issue.

In response to this:

/reopen

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.

@bouk
Copy link
Author

bouk commented Jun 30, 2020

/remove-lifecycle rotten

@apelisse
Copy link
Contributor

apelisse commented Nov 9, 2020

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 9, 2020
@rocketraman
Copy link

Despite kubernetes/ingress-nginx#4791, the nginx controller still does not specify the protocol in all its configurations, so this still prevents --server-side from being used. See for example: https://github.com/kubernetes/ingress-nginx/blob/master/charts/ingress-nginx/templates/controller-service-webhook.yaml.

@apelisse
Copy link
Contributor

Should be fixed in 1.20!

@codablock
Copy link

As I've deployed my first 1.20.2 cluster today, I retried server-side applying a Service without the protocol field being set and I still have the same issue. @apelisse Do you have a link to the PR that fixed this in 1.20?

@liggitt
Copy link

liggitt commented Feb 16, 2021

this was narrowly fixed for container port in 1.20. the service port default is addressed in master in kubernetes/kubernetes@dab5112

@apelisse
Copy link
Contributor

the service port default is addressed in master in kubernetes/kubernetes@dab5112

@liggitt What do you think about backporting this to 1.20?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants