The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Feature #2744

Provide an option to send back diff of txt files in reports

Added by Yimin Li over 4 years ago. Updated about 1 year ago.

Status:ClosedStart date:10/22/2009
Priority:NormalDue date:
Assignee:Nick Lewis% Done:

0%

Category:reports
Target version:2.7.8
Affected Puppet version:0.24.8 Branch:
Keywords: customer

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

When a txt file get replaced, only md5 of the new file is sent back to server. If there is an option that can be enabled to send back diff between new file and old file, then you can know what get changes in the reports/mails.


Related issues

Related to Puppet - Feature #7638: Suppress file show_diff permanently Accepted 05/23/2011
Duplicated by Puppet - Feature #10496: I'd like to have show_diff output in the report. Duplicate 11/02/2011

History

#1 Updated by Luke Kanies over 4 years ago

  • Status changed from Unreviewed to Accepted
  • Target version set to 2.6.0

It’d be great if we could roll this into the event work we’re doing for this release. I think a general-purpose “extra metadata” capability would be a good thing to have in the events in general.

#2 Updated by James Turnbull about 4 years ago

  • Target version changed from 2.6.0 to 2.7.x

#3 Updated by Nigel Kersten almost 3 years ago

  • Status changed from Accepted to Needs More Information
  • Target version deleted (2.7.x)

Does the filebucket API support for showing diffs help you here? We may also have implemented the show_diff option for catalog runs since you initially filed this.

#4 Updated by Nigel Kersten almost 3 years ago

  • Status changed from Needs More Information to Accepted
  • Target version set to 2.7.x

We’re going to modify the show_diff setting so it doesn’t just print the diff, but does it as a notice so that the diff ends up in reports, partly because it’s more consistent, but also because a paying customer has identified this as an important feature.

#5 Updated by Nigel Kersten almost 3 years ago

  • Target version changed from 2.7.x to 3.x

Woops. assigned to wrong version.

#6 Updated by Nigel Kersten over 2 years ago

This is the diff fwiw.

diff --git a/lib/puppet/util/diff.rb b/lib/puppet/util/diff.rb
index f37ea08..1b022bc 100644
--- a/lib/puppet/util/diff.rb
+++ b/lib/puppet/util/diff.rb
@@ -64,7 +64,7 @@ module Puppet::Util::Diff
tempfile.open
tempfile.print string
tempfile.close
- print diff(path, tempfile.path)
+ notice diff(path, tempfile.path)
tempfile.delete
end
end

#7 Updated by Daniel Pittman over 2 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

There were a handful more changes, to ensure that all our diff printing went to the right destination, but the heart of this was right: https://github.com/puppetlabs/puppet/pull/104

Now, the logs will all get a copy of the diff, which includes syslog when configured – but, generally, showing diffs is not enabled, so this shouldn’t result in substantial log spam, etc, for people.

#8 Updated by James Turnbull over 2 years ago

  • Category set to reports
  • Assignee set to Jason McKerr

Daniel has written the code – we need someone to review it.

#9 Updated by James Turnbull over 2 years ago

  • Target version changed from 3.x to 2.7.x

#10 Updated by Jason McKerr over 2 years ago

  • Assignee changed from Jason McKerr to Jacob Helwig

#11 Updated by Nick Lewis over 2 years ago

I’m concerned about the security implications of this. This diff could potentially contain sensitive information which shouldn’t necessarily be published indiscriminately.

At the very least, making this change in a point release worries me, as I would prefer a change with such a significant security impact be more conspicuous.

#12 Updated by Daniel Pittman over 2 years ago

  • Assignee changed from Jacob Helwig to James Turnbull

Nick Lewis wrote:

I’m concerned about the security implications of this. This diff could potentially contain sensitive information which shouldn’t necessarily be published indiscriminately.

Concretely, that matters because the diff … ? Can you fill in the blank, so I don’t answer my own guess about what your concern is?

At the very least, making this change in a point release worries me, as I would prefer a change with such a significant security impact be more conspicuous.

That seems a reasonable concern, but we are somewhat committed by commercial customers – who have demanded this feature for the 2.7 release. Nigel and James are the people to make the final call about where this lands, and how that plays with our promises.

#13 Updated by Nick Lewis over 2 years ago

Daniel Pittman wrote:

I’m concerned about the security implications of this. This diff could potentially contain sensitive information which shouldn’t necessarily be published indiscriminately.

Concretely, that matters because the diff … ? Can you fill in the blank, so I don’t answer my own guess about what your concern is?

Because we may be uploading secure information into a less-secure system (such as Dashboard, which may be shared amongst teams, and have a lighter security policy given its present lack of precisely this sort of information).

At the very least, making this change in a point release worries me, as I would prefer a change with such a significant security impact be more conspicuous.

That seems a reasonable concern, but we are somewhat committed by commercial customers – who have demanded this feature for the 2.7 release. Nigel and James are the people to make the final call about where this lands, and how that plays with our promises.

I won’t complain much if this goes in; I just want to make sure that the decision is/was made with consideration of these potential issues.

#14 Updated by Jason McKerr over 2 years ago

  • Assignee changed from James Turnbull to Nick Lewis

Nigel has already approved the ramifications of this. Copying Nick F, since we’ll need documentation of the change. Also copying Dom for QA. Nick L, please go ahead and merge if the code is acceptable.

#15 Updated by Daniel Pittman over 2 years ago

Nick Lewis wrote:

Daniel Pittman wrote:

I’m concerned about the security implications of this. This diff could potentially contain sensitive information which shouldn’t necessarily be published indiscriminately.

Concretely, that matters because the diff … ? Can you fill in the blank, so I don’t answer my own guess about what your concern is?

Because we may be uploading secure information into a less-secure system (such as Dashboard, which may be shared amongst teams, and have a lighter security policy given its present lack of precisely this sort of information).

nod In the concrete case I don’t think it really moves the security dial in a way that the file bucket / inventory service tools do, but I agree that it is a concern.

Nigel is comfortable that we can go ahead and put this in; Nick Lewis, would you be happy to merge the change?

Nick Fagerlund, I added you because there might be some documentation changes needed to match this.

Thanks.

#16 Updated by Nick Lewis over 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

Rebased against 2.7.x and merged in commit:845825a535e9d6bc76af0491a0734cdebd509d56.

#17 Updated by Nick Lewis over 2 years ago

  • Status changed from Merged - Pending Release to In Topic Branch Pending Review

I’ve opened pull request 237 to cover the additional requirement that noop should no longer implicitly enable show_diff in light of this behavior change.

#18 Updated by Jacob Helwig over 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version changed from 2.7.x to 2.7.8

This has been merged into 2.7.x in commit:eab8b7bb2ea64f7cfa824ec49ae27f2bd5309864

(#2744) Don't automatically enable show_diff in noop mode

As of 845825a, file diffs are now logged, rather than printed to
console. Because log messages may be stored and more broadly readable,
we no longer implicitly set show_diff in noop mode.

#19 Updated by Matthaus Owens over 2 years ago

  • Status changed from Merged - Pending Release to Closed

Released in 2.7.8rc1

#20 Updated by Charlie Sharpsteen about 1 year ago

  • Keywords set to customer

Also available in: Atom PDF