Fix and enable apache test on SLES and CentOS 7 (#1166)

integration_test/metadata/integration_metadata.go

@@ -49,6 +49,9 @@ type ExpectedMetric struct {
 	Representative bool `yaml:"representative,omitempty" validate:"excluded_with=Optional,excluded_with=Platform"`
 	// Exclusive metric to a particular kind of platform.
 	Platform string `yaml:"platform,omitempty" validate:"excluded_with=Representative,omitempty,oneof=linux windows"`
+	// A list of platforms that this metric is not available on.
+	// Examples: centos-7,debian-10. Not valid are linux,windows.
+	UnavailableOn []string `yaml:"unavailable_on,omitempty" validate:"excluded_with=Representative"`
 }
 
 type LogFields struct {
@@ -57,6 +60,9 @@ type LogFields struct {
 	Type        string `yaml:"type" validate:"required"`
 	Description string `yaml:"description" validate:"excludesall=‘’“”"`
 	Optional    bool   `yaml:"optional,omitempty"`
+	// A list of platforms that this field is not available on.
+	// Examples: centos-7,debian-10.
+	UnavailableOn []string `yaml:"unavailable_on,omitempty"`
 }
 
 type ExpectedLog struct {

integration_test/third_party_apps_data/applications/apache/exercise

@@ -1,9 +1,17 @@
 set -e
 
+DOCUMENT_ROOT=/var/www/html
+
+source /etc/os-release
+if [[ "${ID}" == sles || "${ID}" == opensuse-leap ]]; then
+  DOCUMENT_ROOT=/srv/www/htdocs
+fi
+
 # Create a file that apache can't read.
-touch /var/www/html/forbidden.html
-chmod o-r /var/www/html/forbidden.html
+touch "${DOCUMENT_ROOT}"/forbidden.html
+chmod o-r "${DOCUMENT_ROOT}"/forbidden.html
 
 # Then request that file through apache. This is meant to generate an entry in
 # the access log and the error log.
 curl http://localhost:80/forbidden.html
+

integration_test/third_party_apps_data/applications/apache/metadata.yaml

@@ -51,11 +51,6 @@ minimum_supported_agent_version:
   metrics: 2.7.0
   logging: 2.4.0
 supported_operating_systems: linux
-# TODO: Fix errors and enable tests on all platforms.
-platforms_to_skip:
-  - centos-7 # apache_error: expected values for field jsonPayload.tid but got nil
-  - sles-12 # touch: cannot touch '/var/www/html/forbidden.html': No such file or directory
-  - sles-15 # touch: cannot touch '/var/www/html/forbidden.html': No such file or directory
 supported_app_version: ["2.4"]
 expected_metrics:
   - type: workload.googleapis.com/apache.current_connections
@@ -64,13 +59,18 @@ expected_metrics:
     monitored_resource: gce_instance
     labels:
       server_name: .*
-    representative: true
+    unavailable_on:
+      # https://github.com/GoogleCloudPlatform/ops-agent/issues/1173
+      - centos-7
+      - sles-12
+      - sles-15
   - type: workload.googleapis.com/apache.requests
     value_type: INT64
     kind: CUMULATIVE
     monitored_resource: gce_instance
     labels:
       server_name: .*
+    representative: true
   - type: workload.googleapis.com/apache.scoreboard
     value_type: INT64
     kind: GAUGE
@@ -166,6 +166,13 @@ expected_logs:
       - name: jsonPayload.tid
         type: string
         description: Thread ID
+        unavailable_on:
+          # Unavailable for the same reasons as current_connections,
+          # described in:
+          # https://github.com/GoogleCloudPlatform/ops-agent/issues/1173
+          - centos-7
+          - sles-12
+          - sles-15
       - name: severity
         type: string
         description: ''

integration_test/third_party_apps_test.go

@@ -334,7 +334,7 @@ func verifyLog(actualLog *cloudlogging.Entry, expectedLog *metadata.ExpectedLog)
 		_, fileOk := expectedFields["sourceLocation.file"]
 		_, lineOk := expectedFields["sourceLocation.line"]
 		if fileOk || lineOk {
-			multiErr = multierr.Append(multiErr, fmt.Errorf("excpected sourceLocation.file and sourceLocation.line but got nil\n"))
+			multiErr = multierr.Append(multiErr, fmt.Errorf("expected sourceLocation.file and sourceLocation.line but got nil\n"))
 		}
 	} else {
 		if err := verifyLogField("sourceLocation.file", actualLog.SourceLocation.File, expectedFields); err != nil {
@@ -407,16 +407,40 @@ func verifyLog(actualLog *cloudlogging.Entry, expectedLog *metadata.ExpectedLog)
 	return nil
 }
 
+// stripUnavailableFields removes the fields that are listed as unavailable_on
+// the current platform.
+func stripUnavailableFields(fields []*metadata.LogFields, platform string) []*metadata.LogFields {
+	var result []*metadata.LogFields
+	for _, field := range fields {
+		if !metadata.SliceContains(field.UnavailableOn, platform) {
+			result = append(result, field)
+		}
+	}
+	return result
+}
+
 func runLoggingTestCases(ctx context.Context, logger *logging.DirectoryLogger, vm *gce.VM, logs []*metadata.ExpectedLog) error {
 	// Wait for each entry in LogEntries concurrently. This is especially helpful
 	// when	the assertions fail: we don't want to wait for each one to time out
 	// back-to-back.
 	var err error
 	c := make(chan error, len(logs))
 	for _, entry := range logs {
-		entry := entry // https://golang.org/doc/faq#closures_and_goroutines
+		// https://golang.org/doc/faq#closures_and_goroutines
+		// Plus we need to dereference the pointer to make a copy of the
+		// underlying struct.
+		entry := *entry
 		go func() {
-			// Construct query using non-optional fields.
+			// Strip out the fields that are not available on this platform.
+			// We do this here so that:
+			// 1. the field is never mentioned in the query we send, and
+			// 2. verifyLogField treats it as any other unexpected field, which
+			//    means it will fail the test ("expected no value for field").
+			//    This could result in annoying test failures if the app suddenly
+			//    begins reporting a log field on a certain platform.
+			entry.Fields = stripUnavailableFields(entry.Fields, vm.Platform)
+
+			// Construct query using remaining fields with a nonempty regex.
 			query := constructQuery(entry.LogName, entry.Fields)
 
 			// Query logging backend for log matching the query.
@@ -427,7 +451,7 @@ func runLoggingTestCases(ctx context.Context, logger *logging.DirectoryLogger, v
 			}
 
 			// Verify the log is what was expected.
-			err = verifyLog(actualLog, entry)
+			err = verifyLog(actualLog, &entry)
 			if err != nil {
 				c <- err
 				return
@@ -475,6 +499,10 @@ func runMetricsTestCases(ctx context.Context, logger *logging.DirectoryLogger, v
 			logger.ToMainLog().Printf("Skipping optional or representative metric %s", metric.Type)
 			continue
 		}
+		if metadata.SliceContains(metric.UnavailableOn, vm.Platform) {
+			logger.ToMainLog().Printf("Skipping metric %s due to unavailable_on", metric.Type)
+			continue
+		}
 		requiredMetrics = append(requiredMetrics, metric)
 	}
 	c := make(chan error, len(requiredMetrics))
@@ -579,9 +607,9 @@ func runSingleTest(ctx context.Context, logger *logging.DirectoryLogger, vm *gce
 
 	if metadata.ExpectedLogs != nil {
 		logger.ToMainLog().Println("found expectedLogs, running logging test cases...")
-		// TODO(b/239240173): bad bad bad, remove this horrible hack once we fix Aerospike on SLES
+		// TODO(b/254325217): bad bad bad, remove this horrible hack once we fix Aerospike on SLES
 		if app == AerospikeApp && folder == "sles" {
-			logger.ToMainLog().Printf("skipping aerospike logging tests (b/239240173)")
+			logger.ToMainLog().Printf("skipping aerospike logging tests (b/254325217)")
 		} else if err = runLoggingTestCases(ctx, logger, vm, metadata.ExpectedLogs); err != nil {
 			return nonRetryable, err
 		}