feat(provider/kubernetes): v2 restrict namespaces for uploading manifests #2410
Conversation
gardleopard
changed the title
|
I have tested this now and it works, except I think the default namespace handling is strange. I think maybe I cannot use that method and also that it needs to check for the ommitnamespaces and ignore all of this if no namespaces are explicitly defined. |
| @@ -93,6 +100,11 @@ public boolean validateV2Credentials(AccountCredentialsProvider provider, String | |||
| return false; | |||
| } | |||
|
|
|||
| if (!((KubernetesV2Credentials)credentials.getCredentials()).containsNamespace(namespace)) { | |||
|
Checking the omit namespace is definitely important - I don't think this should be the place to check the default namespace. I'm thinking some sort of validation should fail if someone specifies a "Default namespace" that they can't deploy to because it's not listed in the "namespaces", or omitted by the "omitNamespaces". |
gardleopard
force-pushed
the
v2_namespace_protection
branch
from
8f89cfa
to
b4048a2
Compare
gardleopard
changed the title
|
@lwander I have changed the logic and added tests for this now. I think the tests clearly shows the behaviour now. |
| @@ -115,6 +115,14 @@ public String getDefaultNamespace() { | |||
| return cachedDefaultNamespace; | |||
| } | |||
|
|
|||
| public List<String> getNamespaces() { | |||
There was a problem hiding this comment.
I would just annotated the namespaces and omitNamespaces fields with @Getter like the properties above around line 77
| protected boolean validateNamespace(String namespace, KubernetesV2Credentials credentials) { | ||
| final List<String> configuredNamespaces = credentials.getNamespaces(); | ||
| if (configuredNamespaces != null && !configuredNamespaces.isEmpty() && !configuredNamespaces.contains(namespace)) { | ||
| reject( "wrongNamespace", "namespace"); |
There was a problem hiding this comment.
Also, should the string "namespace" be replaced with the actual requested namespace?
|
|
||
| final List<String> omitNamespaces = credentials.getOmitNamespaces(); | ||
| if (omitNamespaces != null && omitNamespaces.contains(namespace)) { | ||
| reject( "omittedNamespace", "namespace"); |
| if (!validateNotEmpty("account", accountName)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!validateNotEmpty("account", namespace)) { |
| return true; | ||
| } | ||
|
|
||
| protected boolean validateNamespace(String namespace, KubernetesV2Credentials credentials) { |
There was a problem hiding this comment.
I'm thinking we should skip this (e.g. return true) when namespace is empty. If none is supplied, the default will be used which should be valid.
There was a problem hiding this comment.
in line 88 we check if the namespace is empty, so it wont reach this part of the code if a namespace is not supplied.
There was a problem hiding this comment.
Line 89 returns false, which is not correct for resources without a namespace. I've submitted #2422 to change this.
gardleopard
force-pushed
the
v2_namespace_protection
branch
from
ddbc46a
to
bb99d7a
Compare
Solves spinnaker/spinnaker#2325
Depends on #2409