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

Issue #696 - Support apps with static namespaces in resources #842

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented Nov 29, 2018

After PR is merged application might have resources with 'hardcoded' namespaces. This partially solved issue #696

@alexmt alexmt force-pushed the 696-hardcoded-namespaces branch 3 times, most recently from 45c05a8 to 8d204d6 Compare November 29, 2018 00:33
@jessesuen
Copy link
Member

jessesuen commented Nov 29, 2018

I haven't yet looked at the code, but before I forget, do we prevent the following attack:

In git, I have a ClusterRole with a metadata.namespace value (even though ClusterRoles don't need namespace). The namespace is in the whitelisted namespaces in the project, but ClusterRoles are not whitelisted.

The expected behavior is that the create ClusterRole should be rejected.

@alexmt alexmt force-pushed the 696-hardcoded-namespaces branch from 8d204d6 to fb9602f Compare November 29, 2018 18:58
@alexmt
Copy link
Collaborator Author

alexmt commented Nov 29, 2018

@jessesuen thanks for reminding about project permissions check. The attack which you described is prevented ( we are explicetly if resource is namespaced or not using discovery api ).

I forgot to add destination namespace check. This is fixed now. Please review PR

@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3a9196c). Click here to learn what that means.
The diff coverage is 52.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #842   +/-   ##
=========================================
  Coverage          ?   30.75%           
=========================================
  Files             ?       47           
  Lines             ?     6893           
  Branches          ?        0           
=========================================
  Hits              ?     2120           
  Misses            ?     4451           
  Partials          ?      322
Impacted Files Coverage Δ
controller/state.go 4.76% <0%> (ø)
util/kube/kube.go 31.91% <0%> (ø)
controller/sync_hooks.go 20.11% <10.52%> (ø)
controller/cache/cache.go 3.73% <21.73%> (ø)
controller/cache/cluster.go 78.37% <60%> (ø)
controller/sync.go 56.13% <84.21%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9196c...fb9602f. Read the comment docs.

if api, ok := c.apis[gvk]; ok && !api.Namespaced {
return false
}
return true
Copy link
Member

Choose a reason for hiding this comment

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

Is there any problem here that if we don't see the GVK in the cluster, that we unconditionally consider it as a namespaced? e.g. will deploying an app for the first time that contains a cluster-scoped CRD + cluster-scoped CRD object work okay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. working improving it in next PR

@alexmt alexmt merged commit 6dede28 into argoproj:master Nov 29, 2018
@alexmt alexmt deleted the 696-hardcoded-namespaces branch November 30, 2018 17:35
@peterbosalliandercom
Copy link

Should this be working on role and rolebindings? I have set a namespace hardcoded into the resource and still it pickes up the application namespace...

@peterbosalliandercom
Copy link

I have added the following pod, it should be running in namespace demo1 but it runs in demo (the application destination namespace).
apiVersion: v1
kind: Pod
metadata:
name: myapp-pod
labels:
app: myapp
spec:
containers:

  • name: myapp-container
    image: busybox
    command: ['sh', '-c', 'echo Hello Kubernetes! && sleep 3600']

@peterbosalliandercom
Copy link

My source pod.yaml was:
apiVersion: v1
kind: Pod
metadata:
name: myapp-pod
labels:
app: myapp
namespace: demo1
spec:
containers:

  • name: myapp-container
    image: busybox
    command: ['sh', '-c', 'echo Hello Kubernetes! && sleep 3600']

@peterbosalliandercom
Copy link

Is this the same issue as described in my previous posts? 483872a

@jessesuen
Copy link
Member

Yes possibly. Are you using kustomize?

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

Successfully merging this pull request may close these issues.

4 participants