prevent slashes in object names #1847
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Drop the UPSTREAM - I don't think we really need that part, and it's up to each resource type (Go's standard library flaws are not things we hack around in core code). |
Why do we want this logic dropped into the validation of half the resources in origin, when no object in our REST API can actually have those names? It's just as invalid as an empty name. If someone wants to create resource that allows slashes in the name because it isn't going to be used in a REST API, why shouldn't they create their own one-off validation as opposed to having the common case not handle it in a common spot? |
deads2k
force-pushed
the
deads-prevent-slashes
branch
from
ba46024
to
741d271
Compare
|
Now without upstream changes. |
|
LGTM, @ncdc sign off please |
| @@ -10,13 +11,21 @@ import ( | |||
| "github.com/openshift/origin/pkg/image/api" | |||
| ) | |||
|
|
|||
| func MinimalNameValidation(name string, prefix bool) (bool, string) { | |||
There was a problem hiding this comment.
Do we not need this for every other resource type as well?
There was a problem hiding this comment.
Sort of. All the name validators need to at least limit those characters. The others I know about are more restrictive.
There was a problem hiding this comment.
I see a lot are using ValidatePodName and others are using custom funcs that handle % and / and other characters. It'd be nice to have a single common func that all validations could use to check the bare minimum, and then custom logic could be added after that. But I don't think it's required for this PR.
There was a problem hiding this comment.
It'd be nice to have a single common func that all validations could use to check the bare minimum
My first attempt as rejected because it was too broad. My second attempt failed because our main validation package would become cyclic. I decided I'd rather get enough in to stop us from failing before attempting a more consistent change again.
|
additional tests added. |
|
[test] |
|
LGTM |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1886/) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1652/) (Image: devenv-fedora_1339) |
|
transients protractor in test and integration port binding in merge. re[merge] |
|
Evaluated for origin up to 6e359df |
openshift-bot
pushed a commit
that referenced
this pull request
Apr 23, 2015
Merged by openshift-bot
This was referenced May 8, 2015
jboyd01
pushed a commit
to jboyd01/origin
that referenced
this pull request
Mar 26, 2018
…service-catalog/' changes from c3e3071633..231772fcc0 231772fcc0 origin build: add origin tooling 98af588 v0.1.11 release changes 01e2f90 v0.1.10 release changes 49af948 clear polling queue before starting new operation (openshift#1855) 252958e Refactor common serviceclass validations (openshift#1858) 68f55c6 Catalog Controller should listen on https and serve metrics over TLS secured channel (openshift#1851) 5d0f773 Refactor common broker validations (openshift#1865) eeaf285 Add NamespacedServiceBroker switch to helm chart (openshift#1864) d2c960c Add NamespacedServiceBroker Feature (openshift#1863) ef15310 Fix NamespaceScoped doc text for ns types (openshift#1862) 8d0a637 fix async deprovision retry (openshift#1832) a918a16 Update registry code from serviceclass to clusterserviceclass (openshift#1852) 4dfd13c Bump dependency on go-open-service-broker-client to 0.0.6 (openshift#1856) 958b7cd Rename SharedServicePlanSpec to CommonServicePlanSpec (openshift#1850) 426aec3 pick a better random port to listen on in integration tests (openshift#1844) 6a59ada OrphanMitigation condition and different handling of retry timeout (openshift#1789) c9b8f60 Extracting common broker spec elements into embeddable struct (openshift#1841) 3f8fab6 Extract common service class spec fields (openshift#1834) 93dab13 Support for OSB [PR#452](openservicebrokerapi/servicebroker#452). (openshift#1849) 5e1e90d [WIP] Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1847) 74f73c0 disable tests for deployment stage (openshift#1845) 82fc6e4 Revert "Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)" (openshift#1843) 94b5795 gitignore integration.test binary (openshift#1840) 014c468 Extracting common plan spec into embeddable struct (openshift#1833) 5d7041b Use k8s NewUUID method exclusively (openshift#1836) eac3f96 A new test was added after prechecks happened for last pr. (openshift#1838) 4b5d159 Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803) cc02f0e [svcat] Adding a filter to get plan. (openshift#1758) 70afb56 reset RemovedFromBroker flag on plans that are re-added by broker (openshift#1824) 712dd4a Add behavior to print deleted instance name (openshift#1806) 55505be Update catalog charts README configuration (openshift#1823) 6426c98 Controller reconciliation rework - part 2 (ServiceBinding) (openshift#1819) c606560 Integrate svcat docs with Service Catalog's (openshift#1784) e9aeeb0 Synchronize some generated code that was missed along the way (openshift#1801) a63ebf7 Fix failing test: TestReconcileServiceInstanceWithFailedCondition (openshift#1813) 07ef743 Controller reconciliation rework - part 1 (ServiceInstance) (openshift#1779) a7d602b Export the touch instance command (openshift#1809) bddb9a7 Allow retries for instances with Failed condition after spec changes (openshift#1751) a777af5 Add enhanced parameter options to provision (openshift#1785) fd1a0b9 Print deleted bindings (openshift#1799) 36d437a Adding the ability to sync a service instance (openshift#1762) 1f60676 Remove Failed condition if there was no terminal failure (openshift#1788) cd831de allow brokers to respond to getCatalog() with no services (openshift#1772) (openshift#1781) e5c37ad Add ObservedGeneration and Provisioned into ServiceInstanceStatus (openshift#1748) 9021d8b Add carolynvs to OWNERS (openshift#1780) b7643d6 Add all-namespaces flag to svcat (openshift#1782) 01e652f Use docker to interact with files made by docker (openshift#1777) REVERT: c3e3071633 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 231772fcc00be08b6b2665a39c4a3bacb0b2271f
jboyd01
pushed a commit
to jboyd01/origin
that referenced
this pull request
Mar 27, 2018
…service-catalog/' changes from c3e3071633..231772fcc0 231772fcc0 origin build: add origin tooling 98af588 v0.1.11 release changes 01e2f90 v0.1.10 release changes 49af948 clear polling queue before starting new operation (openshift#1855) 252958e Refactor common serviceclass validations (openshift#1858) 68f55c6 Catalog Controller should listen on https and serve metrics over TLS secured channel (openshift#1851) 5d0f773 Refactor common broker validations (openshift#1865) eeaf285 Add NamespacedServiceBroker switch to helm chart (openshift#1864) d2c960c Add NamespacedServiceBroker Feature (openshift#1863) ef15310 Fix NamespaceScoped doc text for ns types (openshift#1862) 8d0a637 fix async deprovision retry (openshift#1832) a918a16 Update registry code from serviceclass to clusterserviceclass (openshift#1852) 4dfd13c Bump dependency on go-open-service-broker-client to 0.0.6 (openshift#1856) 958b7cd Rename SharedServicePlanSpec to CommonServicePlanSpec (openshift#1850) 426aec3 pick a better random port to listen on in integration tests (openshift#1844) 6a59ada OrphanMitigation condition and different handling of retry timeout (openshift#1789) c9b8f60 Extracting common broker spec elements into embeddable struct (openshift#1841) 3f8fab6 Extract common service class spec fields (openshift#1834) 93dab13 Support for OSB [PR#452](openservicebrokerapi/servicebroker#452). (openshift#1849) 5e1e90d [WIP] Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1847) 74f73c0 disable tests for deployment stage (openshift#1845) 82fc6e4 Revert "Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803)" (openshift#1843) 94b5795 gitignore integration.test binary (openshift#1840) 014c468 Extracting common plan spec into embeddable struct (openshift#1833) 5d7041b Use k8s NewUUID method exclusively (openshift#1836) eac3f96 A new test was added after prechecks happened for last pr. (openshift#1838) 4b5d159 Pass correct plan ID in deprovision request (for both deleting and orphan mitigation) (openshift#1803) cc02f0e [svcat] Adding a filter to get plan. (openshift#1758) 70afb56 reset RemovedFromBroker flag on plans that are re-added by broker (openshift#1824) 712dd4a Add behavior to print deleted instance name (openshift#1806) 55505be Update catalog charts README configuration (openshift#1823) 6426c98 Controller reconciliation rework - part 2 (ServiceBinding) (openshift#1819) c606560 Integrate svcat docs with Service Catalog's (openshift#1784) e9aeeb0 Synchronize some generated code that was missed along the way (openshift#1801) a63ebf7 Fix failing test: TestReconcileServiceInstanceWithFailedCondition (openshift#1813) 07ef743 Controller reconciliation rework - part 1 (ServiceInstance) (openshift#1779) a7d602b Export the touch instance command (openshift#1809) bddb9a7 Allow retries for instances with Failed condition after spec changes (openshift#1751) a777af5 Add enhanced parameter options to provision (openshift#1785) fd1a0b9 Print deleted bindings (openshift#1799) 36d437a Adding the ability to sync a service instance (openshift#1762) 1f60676 Remove Failed condition if there was no terminal failure (openshift#1788) cd831de allow brokers to respond to getCatalog() with no services (openshift#1772) (openshift#1781) e5c37ad Add ObservedGeneration and Provisioned into ServiceInstanceStatus (openshift#1748) 9021d8b Add carolynvs to OWNERS (openshift#1780) b7643d6 Add all-namespaces flag to svcat (openshift#1782) 01e652f Use docker to interact with files made by docker (openshift#1777) REVERT: c3e3071633 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 231772fcc00be08b6b2665a39c4a3bacb0b2271f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #1844
Upstream: kubernetes/kubernetes#7129
Added code in kube to prevent bad API object names. Updated lots of image validation code to validate object meta fields and properly validate namespaced versus non-namespaced resources.
@ncdc review please?
@sosiouxme fyi