*: audit operator actions to hostPath volume #1232

hasbro17 · 2017-06-28T23:26:30Z

Partially addresses #1205

The operator will now audit the following actions for a self-hosted cluster to a hostPath volume:

Create an etcd pod
Delete an etcd pod
A spec update to the cluster

To enable the auditing a hostPath volume must be mounted to the mountPath /var/tmp/ as shown below. The hostPath itself can be anything but should ideally be /var/tmp so that the operator can write to the volume even when running as the user nobody with limited write permissions.

spec:
  containers:
  - name: etcd-operator
     image: quay.io/coreos/etcd-operator:v0.4.2
        command:
        - /usr/local/bin/etcd-operator
        - --debug-logfile-path=/var/tmp/etcd-operator/debug/debug.log
    volumeMounts:
    - mountPath: /var/tmp/etcd-operator/debug
      name: debug-volume
  volumes:
  - name: debug-volume
    hostPath:
      path: /var/tmp/

The debug log file path can be specified by passing the flag debug-logfile-path to the operator.
The path specified by the flag and the mountPath should be the same.

hongchaodeng · 2017-06-28T23:45:07Z

Any examples what the audit log looks like?

hongchaodeng · 2017-06-28T23:59:06Z

pkg/cluster/audit.go

+		return nil
+	}
+
+	mountPath := "/var/tmp/etcd-operator/"


There is a OperatorRoot

this will always be mounted. so we need a flag to enable audit logging now.

actually "audit logging" might be a bad name. probably debug logging is more suitable.

@xiang90 Actually the OperatorRoot filepath is not mounted/created before the debug logger is setup.
The OperatorRoot path is created after that by the backup manager during the cluster setup by the backup manager while creating the credentials and config files from the aws secret.
https://github.com/coreos/etcd-operator/blob/master/pkg/cluster/backupstorage/s3.go#L35-L39

Although I agree an explicit flag to enable debugging is better than silently detecting the presence of the mountPath.

My only concern now is that when you mount a hostPath to /var/tmp/etcd-operator/ for debugging, and if you set up S3 backup, the credentials and config files will be written to the hostPath as well now. Even though they are deleted later when the cluster is destroyed successfully, just wondering if this is alright.

it is OK to put them under the same operator root dir. but we do not want our debug logging to be deleted by any accident.

Oh yeah the debug logging won't be deleted. Just the aws credential and config files setup by the backup manager. Alright then, I guess we're good to share the OperatorRoot for debugging.

that is fine.

hasbro17 · 2017-06-28T23:59:33Z

These logs were generated from a non-self hosted cluster creating the audit logs during testing. It was easier to perform more actions on a non-self hosted cluster to generate the audit.

I'm generating more detailed logs for a self-hosted cluster via bootkube at the moment. Will post them here once I'm done.

hongchaodeng · 2017-06-29T00:00:15Z

pkg/cluster/audit.go

+)
+
+func NewAuditLogger(cl *spec.Cluster, lg *logrus.Entry) *logrus.Logger {
+	if cl.Spec.SelfHosted == nil {


We should make this behavior explicit outside:

if selfhosted != nil { c.AuditLogger = NewAuditLogger() }

hongchaodeng · 2017-06-29T00:02:23Z

pkg/cluster/audit.go

+	return l
+}
+
+func (c *Cluster) auditPodCreation(pod *v1.Pod, podCreationErr error) {


We should provide an AuditLogger object instead of coupling it as Cluster methods.

xiang90 · 2017-06-29T04:07:48Z

pkg/cluster/audit.go

+	}
+	lg.Infof("detected the mountPath(%v): starting to audit operator actions to the mountPath", mountPath)
+
+	fileName := mountPath + cl.Metadata.Name + ".log"


use path.join

xiang90 · 2017-06-29T04:08:43Z

pkg/util/k8sutil/pod_util.go

@@ -168,3 +169,11 @@ func getPodReadyCondition(status *v1.PodStatus) *v1.PodCondition {
 	}
 	return nil
 }
+
+func GetReadablePodSpec(pod *v1.Pod) (string, error) {


PodSpecToPrettyJSON

hasbro17 · 2017-07-11T20:50:24Z

@xiang90 @hongchaodeng Made the changes requested. The debug logger is now a separate object that appends to a log file for the cluster.
To turn on the debug logging the following need to be true:

The cluster must be self hosted
A volume must be mounted at the path /var/tmp/etcd-operator/debug
The etcd-operator flag debug must be set to true

The pod spec is not printed for pods added/deleted because that makes the output too verbose.
An update in the cluster spec is logged since that helps make sense of the other actions(pod creation/deletion) that the etcd-operator logs. However that can be removed if not neeeded.

Logs for a 4 master node self hosted cluster. The following actions were performed:

Resize the cluster from 1->4 pods after its creation
Delete one pod manually
Resize the cluster from 4->3 pods
The messages prepended with ########## were added by me to indicate certain events like the operator restarting.

xiang90 · 2017-07-11T21:21:30Z

The pod spec is not printed for pods added/deleted because that makes the output too verbose.

i think it is still worth to add even though it can be verbose.

xiang90 · 2017-07-11T21:23:05Z

cmd/operator/main.go

@@ -72,6 +73,7 @@ var (

 func init() {
 	flag.BoolVar(&analyticsEnabled, "analytics", true, "Send analytical event (Cluster Created/Deleted etc.) to Google Analytics")
+	flag.BoolVar(&debug.DebugEnabled, "debug", false, "Enable debug logging of operator actions to a hostPath volume mounted at /var/tmp/etcd-operator/")


enable-debug-file (also you can make this arg to take a actual file path. default can be /var/tmp/etcd-operator/debug.log)

operator probably should not know about hostpath thing. it just writes logs to somewhere.

well, actually enable-selfhosted-debug-log is better.

xiang90 · 2017-07-11T21:26:03Z

pkg/debug/debug_logger.go

+
+	logger := logrus.WithField("pkg", "debug")
+
+	mountPath := path.Join(constants.OperatorRoot, "debug")


we can remove this. operator does not have to know anything about the host path mount.

it just logs to what is configured by the deployment when selfhosted-debug-log is enabled.

xiang90 · 2017-07-11T21:26:45Z

pkg/debug/debug_logger.go

+	mountPath := path.Join(constants.OperatorRoot, "debug")
+	_, err := os.Stat(mountPath)
+	if os.IsNotExist(err) {
+		logger.Errorf("No volumed mounted at mountPath(%v): debug logging will not be performed: %v", mountPath, err)


call createDirAll instead here.

hasbro17 · 2017-07-12T18:01:13Z

More verbose logs which print pod spec for every pod created.

The operator will now write the debug logs to a default path /var/tmp/etcd-operator/debug/debug.log or to a custom path specified by the user via the flag debug-logfile-path

xiang90 · 2017-07-12T19:30:32Z

lgtm after addressing the flag merge issue discussed offline. thanks!

hasbro17 · 2017-07-12T21:11:24Z

@hongchaodeng please take a final look. I've tested it manually on a self hosted cluster.
The debug logging will now be enabled if:

Cluster is self-hosted
And the flag debug-logfile-path is set

xiang90 · 2017-07-13T01:19:08Z

cmd/operator/main.go

@@ -72,6 +73,7 @@ var (

 func init() {
 	flag.BoolVar(&analyticsEnabled, "analytics", true, "Send analytical event (Cluster Created/Deleted etc.) to Google Analytics")
+	flag.StringVar(&debug.DebugFilePath, "debug-logfile-path", "", "path where the debug logfile will be written, recommended to be under: /var/tmp/etcd-operator/debug/ to avoid any issue with lack of write permissions")


mention that it is only for self hosted cluster?

hongchaodeng

lgtm

xiang90 · 2017-07-13T18:36:27Z

pkg/cluster/cluster.go

+			c.debugLogger.LogMessage(fmt.Sprintf("pod (%s) not found while trying to delete it", name))
+		}
+	}
+	if c.cluster.Spec.SelfHosted != nil && c.debugLogger != nil {


can you enable/disable the logger during the initialization? then we do not need to check this everywhere.

Or just make it a function?
Like isDebugLoggerEnabled() ?

hongchaodeng · 2017-07-13T19:11:25Z

pkg/cluster/cluster.go

+	}
+}
+
+func (c *Cluster) IsDebugLoggerEnabled() bool {


Why is this public?

hongchaodeng reviewed Jun 28, 2017

View reviewed changes

hongchaodeng reviewed Jun 29, 2017

View reviewed changes

xiang90 reviewed Jun 29, 2017

View reviewed changes

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from c065c7f to 794bc81 Compare July 11, 2017 20:38

xiang90 reviewed Jul 11, 2017

View reviewed changes

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from 794bc81 to 23701d4 Compare July 12, 2017 17:59

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from 23701d4 to da8966f Compare July 12, 2017 21:03

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch 2 times, most recently from 4b74b32 to 33d360c Compare July 12, 2017 23:47

xiang90 reviewed Jul 13, 2017

View reviewed changes

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch 4 times, most recently from bbb8a9b to 10d9823 Compare July 13, 2017 18:22

hongchaodeng reviewed Jul 13, 2017

View reviewed changes

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from 10d9823 to 0900c9f Compare July 13, 2017 18:30

xiang90 reviewed Jul 13, 2017

View reviewed changes

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from 0900c9f to 6a81898 Compare July 13, 2017 18:58

hongchaodeng reviewed Jul 13, 2017

View reviewed changes

pkg/cluster/cluster.go Outdated

}

}

func (c *Cluster) IsDebugLoggerEnabled() bool {

Copy link

Member

hongchaodeng Jul 13, 2017

Choose a reason for hiding this comment

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

Why is this public?

debug: log operator actions to hostPath volume

060f2c0

hasbro17 force-pushed the haseeb/on-disk-log-for-sh-cluster branch from 6a81898 to 060f2c0 Compare July 13, 2017 19:13

hongchaodeng merged commit c946e30 into coreos:master Jul 13, 2017

hasbro17 deleted the haseeb/on-disk-log-for-sh-cluster branch July 13, 2017 19:24

hasbro17 mentioned this pull request Jul 17, 2017

*: enable debug logging for etcd-operator by default coreos/tectonic-installer#1425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: audit operator actions to hostPath volume #1232

*: audit operator actions to hostPath volume #1232

hasbro17 commented Jun 28, 2017 •

edited

Loading

hongchaodeng commented Jun 28, 2017

hongchaodeng Jun 28, 2017

xiang90 Jun 29, 2017

xiang90 Jun 29, 2017

hasbro17 Jun 30, 2017

xiang90 Jun 30, 2017

hasbro17 Jun 30, 2017

xiang90 Jun 30, 2017

hasbro17 commented Jun 28, 2017

hongchaodeng Jun 29, 2017

hongchaodeng Jun 29, 2017

xiang90 Jun 29, 2017

xiang90 Jun 29, 2017

hasbro17 commented Jul 11, 2017

xiang90 commented Jul 11, 2017

xiang90 Jul 11, 2017

xiang90 Jul 11, 2017

xiang90 Jul 11, 2017

xiang90 Jul 11, 2017

hasbro17 commented Jul 12, 2017 •

edited

Loading

xiang90 commented Jul 12, 2017

hasbro17 commented Jul 12, 2017

xiang90 Jul 13, 2017

hongchaodeng left a comment

xiang90 Jul 13, 2017

hongchaodeng Jul 13, 2017

hongchaodeng Jul 13, 2017


		logger := logrus.WithField("pkg", "debug")

		mountPath := path.Join(constants.OperatorRoot, "debug")

*: audit operator actions to hostPath volume #1232

*: audit operator actions to hostPath volume #1232

Conversation

hasbro17 commented Jun 28, 2017 • edited Loading

hongchaodeng commented Jun 28, 2017

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

hasbro17 commented Jun 28, 2017

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

hasbro17 commented Jul 11, 2017

xiang90 commented Jul 11, 2017

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

hasbro17 commented Jul 12, 2017 • edited Loading

xiang90 commented Jul 12, 2017

hasbro17 commented Jul 12, 2017

Choose a reason for hiding this comment

hongchaodeng 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

hasbro17 commented Jun 28, 2017 •

edited

Loading

hasbro17 commented Jul 12, 2017 •

edited

Loading