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.

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

Jump to bottom

Pod log location must validate container if provided #17886

Merged
merged 1 commit into from
Nov 30, 2015
Merged

Pod log location must validate container if provided #17886

merged 1 commit into from
Nov 30, 2015
+114 −4

Conversation

fabianofranz
Copy link
Contributor

When checking the log URL for a pod container we must validate the container name if provided, otherwise the method will return an URL that is actually not valid and results in an error when called, e.g. the server could not find the requested resource ( Pod mypod).

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 27, 2015
@fabianofranz fabianofranz mentioned this pull request Nov 27, 2015
Merged
@fabianofranz fabianofranz force-pushed the pod_logs_validate_container branch 2 times, most recently from f558dd5 to 44bf2a6 Compare November 27, 2015 18:33
@k8s-bot
Copy link

k8s-bot commented Nov 27, 2015

GCE e2e test build/test passed for commit b2f7d3a829eb1c0a7487a0dff7ec91cc1fd2e7bf.

@k8s-bot
Copy link

k8s-bot commented Nov 27, 2015

GCE e2e test build/test passed for commit 44bf2a66c40b47c06c77dae5f38d2518672997cc.

@k8s-bot
Copy link

k8s-bot commented Nov 27, 2015

GCE e2e build/test failed for commit f558dd54eec1a4f37a95d0b84d092fa598739879.

@fabianofranz fabianofranz force-pushed the pod_logs_validate_container branch from 44bf2a6 to 325c5a2 Compare November 27, 2015 20:24
@liggitt liggitt assigned liggitt and unassigned brendandburns Nov 27, 2015
@fabianofranz fabianofranz force-pushed the pod_logs_validate_container branch from 325c5a2 to bd90254 Compare November 27, 2015 20:30
@k8s-bot
Copy link

k8s-bot commented Nov 27, 2015

GCE e2e test build/test passed for commit 325c5a2eae6bc777361ab5185f175c5460ce5462.

@k8s-bot
Copy link

k8s-bot commented Nov 27, 2015

GCE e2e test build/test passed for commit bd90254c42508ee6c388683a51d8aa81e2b47ca8.

0xmichalis
0xmichalis reviewed Nov 28, 2015
View reviewed changes
pkg/registry/pod/strategy.go
@@ -240,6 +240,7 @@ func LogLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter, ct
}

// Try to figure out a container
// If a container was provided, it must be valid
container := opts.Container
if container == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind changing this to len(container) == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@liggitt
Copy link
Member

liggitt commented Nov 28, 2015

Lgtm, update with feedback from @Kargakis?

@liggitt liggitt added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Nov 29, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 29, 2015

GCE e2e build/test failed for commit bd90254c42508ee6c388683a51d8aa81e2b47ca8.

@liggitt
Copy link
Member

liggitt commented Nov 29, 2015

Build error

Error pulling image (0.3) from gcr.io/google_containers/kube-registry-proxy, Get https://storage.googleapis.com/artifacts.google-containers.appspot.com/containers/images/9b9342c134bbda6f57c95c52cafd4ae40333dd6cc0fce4bace9296f957042d22/ancestry: dial tcp 74.125.202.128:443: i/o timeout
!!! Error in /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/../build/common.sh:817
  'docker pull "${addon_path}"' exited with status 1
Call stack:
  1: /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/../build/common.sh:817 kube::release::write_addon_docker_images_for_server(...)
  2: /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/../build/common.sh:719 kube::release::package_server_tarballs(...)
  3: /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/../build/common.sh:654 kube::release::package_tarballs(...)
  4: /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/release.sh:40 main(...)
Exiting with status 1
!!! [1129 05:56:17] unable to pull or write addon image
!!! Error in /jenkins-master-data/jobs/kubernetes-pull-build-test-e2e-gce/workspace/hack/e2e-internal/../../cluster/gce/../../cluster/../build/../build/common.sh:808

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Nov 29, 2015

GCE e2e build/test failed for commit bd90254c42508ee6c388683a51d8aa81e2b47ca8.

@fabianofranz fabianofranz force-pushed the pod_logs_validate_container branch from bd90254 to afd5649 Compare November 30, 2015 01:20
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2015
@fabianofranz
Copy link
Contributor Author

Updated addressing @Kargakis comments, @liggitt needs lgtm again.

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e test build/test passed for commit afd5649.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e build/test failed for commit afd5649.

@wojtek-t
Copy link
Member

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e test build/test passed for commit afd5649.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Nov 30, 2015

GCE e2e test build/test passed for commit afd5649.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 30, 2015
Merge pull request #17886 from fabianofranz/pod_logs_validate_container
63dc126 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 63dc126 into kubernetes:master Nov 30, 2015
This was referenced Nov 30, 2015
Merged
Merged
@ikehz ikehz added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 9, 2015
@RTann RTann mentioned this pull request Oct 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@liggitt liggitt

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.