-
Notifications
You must be signed in to change notification settings - Fork 16
Conversation
546e97d
to
c7a7de6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks decent to me, two quick questions:
- About the way we're adding flags, see below, and
- About whether or not we want to handle patching existing namespaces with this additional policy
// If not, create it. | ||
var aclConfig capi.NamespaceACLConfig | ||
// If the namespace does not, create it with default cross-namespace-policy. | ||
crossNamespacePolicy, err := getOrCreateCrossNamespacePolicy(client, partitionInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to update an existing namespace with the policy in case it was created without? I'm thinking probably not since users could create the namespace themselves with a different set of policies that still allow for cross-namespace service resolution and maybe some other stuff they're interested in.
But, if we do I think we'll probably need to do something like append this policy onto an existing namespace and then CAS the namespace if the policy wasn't previously there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I don't think we want to patch existing namespaces for that reason you mentioned: they're not managed by us and might give broader permissions than the operator wants (it could be a bit of a footgun, but if the operator is manually creating namespaces I'd expect them to also be managing the permissions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to make a corresponding change in the Helm chart to set the environment variables. Feel free to link it back here.
so @andrewstucki it looks like there might already be an env variable in the helm chart to handle that (should've checked |
doesn't exist, this mirrors functionality in consul-k8s
implementation in consul-k8s
about why we need to inspect the error when attempting to create the default ACL
ec8ba84
to
5495997
Compare
Changes proposed in this PR:
When new namespace is created we should automatically apply the
cross-namespace-policy
policy to the newly created namespace to ensure cross namespace communication is allowed.Fixes #510
How I've tested this PR:
<YOUR ENTERPRISE KEY>
with a key for enterprise consul in thedeploy.sh
filemake docker/dev
on the main branch (without these changes)deploy.sh
scriptcurl 0:30602 -vv --header 'Host: nginx.green.daskt.nsbug.it'
and you should see a503
error along withno healthy upstream
Manage Namespaces
from theNamespaces
drop down you will see thegreen
namespace does not have thecross-namespace-policy
attached to itmake docker/dev
again to build the image with the changes from this branchreload.sh
script from the gistcurl 0:30602 -vv --header 'Host: nginx.green.daskt.nsbug.it'
and you should see a response with a bunch of html (you may see a "connection reset by peer" error, if so give things a few seconds to reconcile and try again)Manage Namespaces
from theNamespaces
drop down you will see thegreen
namespace now has thecross-namespace-policy
attached to itHow I expect reviewers to test this PR:
Code Review
Can run the above steps
Checklist: