-
Notifications
You must be signed in to change notification settings - Fork 316
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
Support Consul namespaces for service-intentions #362
Conversation
|
||
// defaultingPatches returns the patches needed to set fields to their | ||
// defaults. | ||
func (v *ServiceIntentionsWebhook) defaultingPatches(err error, svcIntentions *ServiceIntentions) ([]jsonpatch.Operation, error) { |
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.
This only returns patches in consul ent but I didn't short-circuit in case we add defaulting logic for oss later.
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.
This comment just stopped me from mentioning we should short circuit it 😛
"k8s.io/api/admission/v1beta1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
"sigs.k8s.io/controller-runtime/pkg/webhook/admission" | ||
) | ||
|
||
func TestValidateServiceIntentions_Create(t *testing.T) { | ||
func TestHandle_ServiceIntentions_Create(t *testing.T) { |
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.
These tests actually call Handle()
unlike the other webhook tests that do call Validate()
// Test that we return patches to set Consul namespace fields to their defaults. | ||
// This test also tests OSS where we expect no patches since OSS has no | ||
// Consul namespaces. | ||
func TestHandle_ServiceIntentions_Patches(t *testing.T) { |
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.
I added a new test rather than adding permutations to the existing tests because I thought that would make them too complicated.
svcIntentions.Default() | ||
defaultingPatches, err := v.defaultingPatches(err, &svcIntentions) | ||
if err != nil { | ||
return admission.Errored(http.StatusInternalServerError, err) |
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.
Not sure if this should be 500 or 400. If there's an error, it will happen every time but the error could be due to the input or our internal defaulting logic.
It would be an input issue if the json unmarshal failed but I think that's not possible since it would have to be legal json to be posted to the webhook I believe?
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.
You're right. If malformed JSON makes it here, it's probably because the API server messed up in some weird way. I'm OK with a 500 here, given i'd expect the API server to return a 400
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.
LOVE IT!
|
||
// defaultingPatches returns the patches needed to set fields to their | ||
// defaults. | ||
func (v *ServiceIntentionsWebhook) defaultingPatches(err error, svcIntentions *ServiceIntentions) ([]jsonpatch.Operation, error) { |
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.
This comment just stopped me from mentioning we should short circuit it 😛
svcIntentions.Default() | ||
defaultingPatches, err := v.defaultingPatches(err, &svcIntentions) | ||
if err != nil { | ||
return admission.Errored(http.StatusInternalServerError, err) |
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.
You're right. If malformed JSON makes it here, it's probably because the API server messed up in some weird way. I'm OK with a 500 here, given i'd expect the API server to return a 400
67315e6
to
1e90da5
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.
Looks great! I tested it out on a cluster too with both single namespace and namespace mirroring settings.
destinationNamespace: "", | ||
mirroring: false, | ||
prefix: "", | ||
sourceNamespace: "bar", |
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.
the source namespace doesn't change between test cases, so I think we could probably remove that from params.
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.
@ashwin-venkatesh this looks good. I can't approve my own PRs :D
LOL..ill approve it for you ;) |
- Use Defaulting to ensure the correct destination namespace is set on the spec.destination of the config entry. Additionally, explicity set the destination namespace on the service intention config entry. Utilize this namespace to ensure the config entry is managed in the expected Consul namespace.
- Do not fail if empty string value is provided for exposePath.Protocol. Consul defaults it to the correct value.
31e50e4
to
9ce9fab
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.
🦊
Changes proposed in this PR:
Fix support for service-intentions and Consul enterprise. Previously the defaults we were setting for the namespace fields weren't getting applied to the object in the webhook because we weren't returning any patches. When the entry got into the controller, its destination.namespace field was
""
. We would then attempt to create a namespace with name""
which would fail. In addition, if the namespace creation didn't error out, we would create the config entry in the namespace"default"
instead of what we wanted to be the default namespace (e.g. if mirroring we wanted the namespace to equal the kube namespace).In this PR, we reject requests that have source.namespace or destination.namespace specified and consul namespaces are disabled.
Additionally, if a destination namespace is explicitly specified, and namespaces are enabled, we create the service intention in the specified destination namespace.
In case the destination namespace is not specified and namespaces are enabled, we use our existing logic to determine the consul namespace, depending on mirroring, mirror-prefix and destination consul namespace values.
This PR allows validates that atleast one source value is set on an intention.
It also ensures that exposePaths.protocol does not fail is
""
is set as it's value as that defaulting is managed by Consul.How I've tested this PR:
Acceptance tests are passing Test controller against consul:1.9.0-beta1 consul-helm#645 using this image. Previously they were failing for Consul enterprise.
Closes #367
How I expect reviewers to test this PR:
Checklist: