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

Add support for reading Auth data from a file on disk #159

Closed
wants to merge 2 commits into from
Closed

Add support for reading Auth data from a file on disk #159

wants to merge 2 commits into from

Conversation

schancel
Copy link
Contributor

@schancel schancel commented Apr 2, 2019

This commit adds support from reading the authorization section of the
gnatsd config from a JSON file located within the nats-operator container.
This allows secrets the data to be placed on disk using sidecars like
consul-template.

Depends on #171

@schancel
Copy link
Contributor Author

schancel commented Apr 2, 2019

Untested, and needs a test. What do you recommend here @wallyqs ?

pkg/cluster/cluster.go Outdated Show resolved Hide resolved
@schancel
Copy link
Contributor Author

schancel commented Apr 2, 2019

Implements #156 ?

pkg/cluster/cluster.go Outdated Show resolved Hide resolved
@schancel
Copy link
Contributor Author

schancel commented Apr 3, 2019

Okay, if this looks good, I'll try to write some tests. Looks pretty gnarly though.

@schancel
Copy link
Contributor Author

schancel commented Apr 3, 2019

Also, should I put the sample NatsCluster file somewhere?

@wallyqs
Copy link
Member

wallyqs commented Apr 4, 2019

Thanks @schancel ! fwiw I think this approach might better than the other one of injecting all the credentials into the same config secret. The test could be a variation of this one maybe: https://github.com/nats-io/nats-operator/blob/master/test/e2e/config_reload_test.go#L100
It would be great to have an example here to include in the release notes too: https://github.com/nats-io/nats-operator/tree/master/example

@schancel
Copy link
Contributor Author

schancel commented Apr 9, 2019

@wallyqs Well, I finally got it compiling. I still can't setup the e2e tests locally due to virtualization being disabled on the hardware I'm using.

Do you have any idea what's going on here w/ the panic and failing test?

@schancel
Copy link
Contributor Author

schancel commented Apr 9, 2019

Okay it's failing on:

	// Wait for the single pod to be created.
	ctx1, fn := context.WithTimeout(context.Background(), waitTimeout)
	defer fn()
	if err = f.WaitUntilFullMeshWithVersion(ctx1, natsCluster, size, version); err != nil {
		t.Fatal(err)
	}

Specifically for my test. There must be something wrong with the manifest being provided to kubernetes.

@@ -0,0 +1,32 @@
# This is an example NatsCluster manifest which uses a 3rd party initContainer
Copy link
Member

Choose a reason for hiding this comment

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

I built a new version of the reloader and the following works for me:

# This is an example NatsCluster manifest which uses a 3rd party initContainer
# to fetch the authorization credentials from outside kubernetes.
#
# An example of this could be consul-template getting user/passwords from vault.
apiVersion: "nats.io/v1alpha2"
kind: "NatsCluster"
metadata:
  name: nats-auth-file-example
  namespace: default
spec:
  size: 1
  version: "1.4.1"

  natsConfig:
    maxPayload: 20971520

  pod:
    enableConfigReload: true
    reloaderImage: "wallyqs/nats-server-config-reloader"
    reloaderImageTag: "0.4.4-v1alpha2"
    reloaderImagePullPolicy: "IfNotPresent"

    volumeMounts:
      - name: authconfig
        mountPath: /etc/nats-config/authconfig

  auth:
    clientsAuthFile: "authconfig/auth.json"

  template:
    spec:
      initContainers:
        - name: secret-getter
          image: "busybox"
          command: ["sh", "-c", "echo 'users = [ { user: 'foo', pass: 'bar' } ]' > /etc/nats-config/authconfig/auth.json"]
          volumeMounts:
            - name: authconfig
              mountPath: /etc/nats-config/authconfig
      volumes:
        - name: authconfig
          emptyDir: {}

natsCluster.Spec.Pod = &natsv1alpha2.PodPolicy{
// Enable configuration reloading.
EnableConfigReload: true,
VolumeMounts: []v1.VolumeMount{
Copy link
Member

@wallyqs wallyqs Apr 11, 2019

Choose a reason for hiding this comment

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

The reloader image does not get build along with the test so currently have to specify the new reloader image that can be used, for example using these that I pushed:

    reloaderImage: "wallyqs/nats-server-config-reloader"
    reloaderImageTag: "0.4.5-v1alpha2"

ConfigReloadTestHelper(t, func(natsCluster *natsv1alpha2.NatsCluster, cas *v1.Secret) {
natsCluster.Spec.Auth = &natsv1alpha2.AuthConfig{
// Use the secret created above for client authentication.
ClientsAuthFile: "/authconfig/auth.json",
Copy link
Member

Choose a reason for hiding this comment

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

For the include to work it has to be mounted at /etc/nats-config/authconfig/ since the includes have to be relative to where the main config is located which is /etc/nats-config/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😱

That's a huge caveat to add to the documentation. Will definitely cause confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Agree... we'd have to cover with documentation for now.

@@ -93,6 +93,10 @@ func natsPodReloaderContainer(image, tag, pullPolicy string) v1.Container {
constants.PidFilePath,
},
}
if authFilePath != "" {
container.Command = append(container.Command, "-authfile", authFilePath)
Copy link
Member

Choose a reason for hiding this comment

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

I might have been wrong on this one... it looks like the reloader would already follow the events in the folder so if the volume is mounted underneath the main nats config file, it would signal as well in case of changes:

https://github.com/nats-io/nats-operator/blob/master/pkg/reloader/reloader.go#L94-L99

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it's not supposed to be recursively watching. (According to the documentation)

I created another PR to fix these problems with reloader independently.

Copy link
Member

Choose a reason for hiding this comment

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

That's right... it only works per folder, it is not recursive so would need to list each of the folders that should be tracked for changes.

@schancel
Copy link
Contributor Author

@wallyqs Yeah, I noticed that. I'm updating the reloader now to be a bit more generic

@wallyqs
Copy link
Member

wallyqs commented Apr 15, 2019

Once the multi-reloader branch is merged and this branch is rebased, and updating the reloader command, applying the following to the branch makes things work for me: wallyqs@35ed435

There was an issue in the reloader test that when decoding JSON it would also accidentally escape > because by default the Go encoder escapes HTML, only workaround seems to be is to setup manually the encoder to avoid escaping html...

@@ -211,11 +215,24 @@ func ConfigReloadTestHelper(t *testing.T, customizer NatsClusterCustomizerWSecre
 			},
 		},
 	}
-	// Serialize the object containing authentication data.
-	if d, err = json.Marshal(auth); err != nil {
+	// Serialize the object containing authentication data,
+	// we are using wildcard so need to unescape the HTML
+	// which the JSON encoder does by default...
+	buf := &bytes.Buffer{}
+	encoder := json.NewEncoder(buf)
+	encoder.SetEscapeHTML(false)
+	err = encoder.Encode(auth)
+	if err != nil {
+		t.Fatal(err)
+	}
+	buf2 := &bytes.Buffer{}
+	err = json.Indent(buf2, buf.Bytes(), "", "  ")
+	if err != nil {
 		t.Fatal(err)
 	}
+
 	// Create a secret containing authentication data.
+	d = buf2.Bytes()
 	if cas, err = f.CreateSecret(f.Namespace, "data", d); err != nil {
 		t.Fatal(err)
 	}

Currently the config file expects there to only be one config file
watched.  However, the nats config file can include other paths, and
we may also want to watch those.

This commit implements being able to specify an arbtirary number of
configuration files to be watched.
This commit adds support from reading the authorization section of the
gnatsd config from a JSON file located within the nats-operator container.
This allows secrets the data to be placed on disk using sidecars like
consul-template.

Depends on #171
@wallyqs
Copy link
Member

wallyqs commented Apr 29, 2019

Merged via 7ad614f

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.

2 participants