Validate CR namespaces based on consul namespaces #375

ndhanushkodi · 2020-11-01T22:16:38Z

ServiceRouters, ServiceResolvers, ServiceSplitters, and
ServiceIntentions should only have namespaces set in their spec when
consul namespaces are enabled. When consul namespaces are disabled, the
webhook now returns a validation error if these CRs have a namespace.

Previously, users could apply CRs with namespaces set even when consul
namespaces are disabled, causing them to think the namespaces might be
having an effect when they are not.

Co-authored-by: Ashwin Venkatesh ashwin@hashicorp.com

How I've tested this PR:

ran unit tests (make test)
ran acceptance tests in consul-helm using gcr.io/nitya-293720/consul-k8s-dev@sha256:80044a89e92a32dc782117b7bc9626f8aebecf2b5253a6996d141969ef81bf7b
deployed consul-k8s with image gcr.io/nitya-293720/consul-k8s-dev@sha256:80044a89e92a32dc782117b7bc9626f8aebecf2b5253a6996d141969ef81bf7b and applied the following ServiceIntention, ServiceResolver, ServiceRouter, ServiceSplitter with namespaces and verified I saw the right errors in each case.

apiVersion: consul.hashicorp.com/v1alpha1
kind: ServiceIntentions
metadata:
  name: intentions
spec:
  destination:
    name: svc1
    namespace: C
  sources:
  - name: svc2
    namespace: A
    action: allow
  - name: svc3
    namespace: B
    permissions:
    - action: allow
      http:
        methods:
        - GET
        - PUT
        pathExact: "/foo"
---
apiVersion: consul.hashicorp.com/v1alpha1
kind: ServiceResolver
metadata:
  name: resolver
spec:
  redirect:
    service: bar
    namespace: A
  failover:
    failA:
      namespace: B
---
apiVersion: consul.hashicorp.com/v1alpha1
kind: ServiceRouter
metadata:
  name: router
spec:
  routes:
    - destination:
        namespace: A
      match:
        http:
          pathPrefix: "/foo"
---
apiVersion: consul.hashicorp.com/v1alpha1
kind: ServiceSplitter
metadata:
  name: splitter
spec:
  splits:
  - namespace: A
    weight: 100
  - namespace: B
    weight: 100

Sample Error from ServiceIntention:

Error from server: error when creating "serviceintention.yaml": admission webhook "mutate-serviceintentions.consul.hashicorp.com" denied the request: serviceintentions.consul.hashicorp.com "intentions" is invalid: [spec.destination.namespace: Invalid value: "C": consul namespaces must be enabled to set destination.namespace, spec.sources[0].namespace: Invalid value: "A": consul namespaces must be enabled to set source.namespace, spec.sources[1].namespace: Invalid value: "B": consul namespaces must be enabled to set source.namespace]

How I expect reviewers to test this PR:

code review

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

hashicorp-cla · 2020-11-01T22:16:41Z

All committers have signed the CLA.

api/v1alpha1/serviceintentions_types.go

ndhanushkodi · 2020-11-02T18:19:16Z

api/v1alpha1/serviceintentions_types_test.go

-	}
-}
-
-func TestServiceIntentions_ValidateNamespaces(t *testing.T) {


These tests were slightly modified and moved into the TestServiceIntentions_Validate tests since as of this PR ValidateNamespaces is no longer its own exported method, but rather a private method called by Validate()

ndhanushkodi · 2020-11-02T18:22:32Z

api/v1alpha1/serviceintentions_types_test.go

+			expectedErrMsg: []string{
+				`namespace: Invalid value: "namespaceA": consul namespaces must be enabled to set source.namespace`,
+				`namespace: Invalid value: "namespaceB": consul namespaces must be enabled to set source.namespace`,
+				`namespace: Invalid value: "namespaceC": consul namespaces must be enabled to set source.namespace`,


Note that when there are multiple errors I did not assert on every character in the error message. Since the order is not deterministic, we don't know if spec.sources[0].namespace is going to refer to namespaceA or namespaceB or namespaceC, so I omitted that part of the assertion. From the current assertion, we will know that all 3 of these strings are in the error message, so we are asserting on multiple errors, but not every character.

See my suggestion below, since this is an array I think we can safely assert on the index. spec.source[0].namespace should hopefully always be namespaceA since order matters in these sources.

Ah thank you for this catch! Makes sense.

lkysow

Looks great! I really like the refactor moving ValidateNamespaces inside Validate and the attention to detail.

In addition to my inline comments, I wonder if we should use Consul Enterprise namespaces must be enabled instead of consul namespaces must be enabled as our error message? That way we don't have OSS folks trying to enable consul namespaces.

lkysow · 2020-11-02T20:30:56Z

api/v1alpha1/serviceresolver_types_test.go

-		expectedErrMsg string
+		input             *ServiceResolver
+		namespacesEnabled bool
+		expectedErrMsg    []string


Suggested change

expectedErrMsg []string

expectedErrMsgs []string

lkysow · 2020-11-02T20:37:21Z

api/v1alpha1/serviceresolver_types_test.go

-				require.EqualError(t, err, testCase.expectedErrMsg)
+			err := testCase.input.Validate(testCase.namespacesEnabled)
+			if len(testCase.expectedErrMsg) != 0 {
+				for _, s := range testCase.expectedErrMsg {


Suggested change

for _, s := range testCase.expectedErrMsg {

require.Error(t, err)

for _, s := range testCase.expectedErrMsg {

You need this check otherwise if err == nil then I think err.Error() will panic

(this is true for the other tests I believe)

Yup, I'll make this change to all of the tests 💯

api/v1alpha1/serviceintentions_types_test.go

api/v1alpha1/servicerouter_types_test.go

api/v1alpha1/serviceintentions_types_test.go

api/v1alpha1/servicerouter_types_test.go

api/v1alpha1/servicesplitter_types_test.go

kschoche · 2020-11-04T15:22:12Z

api/v1alpha1/serviceintentions_types_test.go

 				ObjectMeta: metav1.ObjectMeta{
 					Name: "does-not-matter",
 				},
 				Spec: ServiceIntentionsSpec{
 					Destination: Destination{
 						Name:      "dest-service",
-						Namespace: "foo",
+						Namespace: "namespaceA",


minor nit here, but I ran into this problem the other day when building a test that had an upper case letter for Namespaces in consul. Consul will automatically convert this to all lowercase letters, so if you're asserting later that the output is namespaceA (which I don't believe you are) from Consul it will not pass. I think it's best to keep our tests with lowercase namespaces as those are valid for both k8s and Consul.
What do you think?

Suggested change

Namespace: "namespaceA",

Namespace: "namespace-a",

I think it's best to keep our tests with lowercase namespaces as those are valid for both k8s and Consul.

Yeah, I agree, I'll make this change!

Are the namespaces in consul tied to the corresponding k8s namespace? Would anyone ever write a serviceintention for example with a namespace with capital letters? Or because the namespace is inherently linked between consul and k8s it will only ever be lowercase in both?

Actually capital letters are not even valid in k8s, so it would cover both!

kyle@kyles-mbp consul-helm % kubectl create namespace ABCD The Namespace "ABCD" is invalid: metadata.name: Invalid value: "ABCD": a DNS-1123 label must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character (e.g. 'my-name', or '123-abc', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?')

And I do not think anyone would ever write a service intention to a consul namespace with capitals because consul wouldn't ever have a namespace with capitals - that being said I'm not sure if it silently converts them to lowercase and returns success or would return failure in this scenario.

Got it, thanks for the explanation!

Yeah great catch!

api/v1alpha1/serviceintentions_webhook.go

lkysow

🎉

kschoche

This looks great!
I added a couple comments which aren't blockers.

ServiceRouters, ServiceResolvers, ServiceSplitters, and ServiceIntentions should only have namespaces set in their spec when consul namespaces are enabled. When consul namespaces are disabled, the webhook now returns a validation error if these CRs have a namespace. Previously, users could apply CRs with namespaces set even when consul namespaces are disabled, causing them to think the namespaces might be having an effect when they are not. Co-authored-by: Ashwin Venkatesh <ashwin@hashicorp.com>

This would make it easier for a user to understand that they need Consul Enterprise in order to enable namespaces.

* Support auto-encrypt Setting the global.tls.enableAutoEncrypt will now enable auto-encrypt for clients and servers and switch consul-k8s components that need to talk to the clients (connect injector, mesh gateway, sync catalog, and snapshot agent) to now get the CA through the API from the Consul server before they start. Optionally, allow configuring external server information to be used for HTTPS API. Currently, this is only used to retrieve client's CA when using auto-encrypt, but it could potentially be extended for other use cases (e.g. ACL bootstrapping) when the Consul server cluster is outside of k8s.

ndhanushkodi force-pushed the validate-namespaces branch from b879076 to 4e01b71 Compare November 2, 2020 17:56

ndhanushkodi commented Nov 2, 2020

View reviewed changes

ndhanushkodi marked this pull request as ready for review November 2, 2020 18:23

ndhanushkodi requested review from lkysow, a team and kschoche and removed request for a team November 2, 2020 18:30

ndhanushkodi added the theme/crds label Nov 2, 2020

lkysow reviewed Nov 2, 2020

View reviewed changes

ndhanushkodi force-pushed the validate-namespaces branch from 4e01b71 to 4a66632 Compare November 4, 2020 05:37

kschoche reviewed Nov 4, 2020

View reviewed changes

api/v1alpha1/serviceintentions_webhook.go Show resolved Hide resolved

lkysow approved these changes Nov 4, 2020

View reviewed changes

kschoche approved these changes Nov 4, 2020

View reviewed changes

ndhanushkodi force-pushed the validate-namespaces branch 2 times, most recently from e267e48 to ca08cc0 Compare November 4, 2020 18:49

ndhanushkodi and others added 2 commits November 4, 2020 11:01

Update error message to Consul Enterprise namespaces

00bc0f1

This would make it easier for a user to understand that they need Consul Enterprise in order to enable namespaces.

ndhanushkodi force-pushed the validate-namespaces branch from ca08cc0 to 00bc0f1 Compare November 4, 2020 19:02

ndhanushkodi merged commit 1474369 into master Nov 4, 2020

ndhanushkodi deleted the validate-namespaces branch November 4, 2020 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate CR namespaces based on consul namespaces #375

Validate CR namespaces based on consul namespaces #375

ndhanushkodi commented Nov 1, 2020 •

edited

Loading

hashicorp-cla commented Nov 1, 2020 •

edited

Loading

ndhanushkodi Nov 2, 2020

ndhanushkodi Nov 2, 2020

lkysow Nov 2, 2020

ndhanushkodi Nov 4, 2020

lkysow left a comment

lkysow Nov 2, 2020

lkysow Nov 2, 2020

lkysow Nov 2, 2020

ndhanushkodi Nov 4, 2020

kschoche Nov 4, 2020

ndhanushkodi Nov 4, 2020

kschoche Nov 4, 2020 •

edited

Loading

ndhanushkodi Nov 4, 2020

lkysow Nov 5, 2020

lkysow left a comment

kschoche left a comment

	for _, s := range testCase.expectedErrMsg {
	require.Error(t, err)
	for _, s := range testCase.expectedErrMsg {

Validate CR namespaces based on consul namespaces #375

Validate CR namespaces based on consul namespaces #375

Conversation

ndhanushkodi commented Nov 1, 2020 • edited Loading

hashicorp-cla commented Nov 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschoche Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

kschoche left a comment

Choose a reason for hiding this comment

ndhanushkodi commented Nov 1, 2020 •

edited

Loading

hashicorp-cla commented Nov 1, 2020 •

edited

Loading

kschoche Nov 4, 2020 •

edited

Loading