atc: behaviour: add ArchivePipeline endpoint #5346
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| package pipelineserver | ||
|
|
||
| import ( | ||
| "net/http" | ||
|
|
||
| "github.com/concourse/concourse/atc/db" | ||
| ) | ||
|
|
||
| func (s *Server) ArchivePipeline(pipelineDB db.Pipeline) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if !s.enableArchivePipeline { | ||
| http.Error(w, "endpoint is not enabled", http.StatusForbidden) | ||
| return | ||
| } | ||
| s.logger.Debug("archive-pipeline") | ||
|
There was a problem hiding this comment. oh was this intentionally left here? if one wants to get a sense of "this endpoint was hit", one could leverage the audit logs There was a problem hiding this comment. Given that I find myself reading other people's logs (more on that below), I feel motivated to make the system more verbose by default, just in case. This line is actually enforced by a unit test, because I don't want to rely on other people to turn on auditing if they're then going to be needing my advice on the state of their cluster. There was a problem hiding this comment. yeah, I do get the feeling :/
although this does rely on the fact that they turned if the same was true for audit (assuming the pain-point is quickly turning the system into a more verbose mode), would you think this log message in this particular endpoint would not be needed anymore? |
||
| err := pipelineDB.Archive() | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| s.logger.Error("archive-pipeline", err) | ||
| } | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package pipelineserver_test | ||
|
|
||
| import ( | ||
| "errors" | ||
| "io/ioutil" | ||
| "net/http" | ||
| "net/http/httptest" | ||
|
|
||
| "github.com/concourse/concourse/atc/api/pipelineserver" | ||
| "github.com/concourse/concourse/atc/api/pipelineserver/pipelineserverfakes" | ||
| "github.com/concourse/concourse/atc/db/dbfakes" | ||
| . "github.com/onsi/ginkgo" | ||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| //go:generate counterfeiter code.cloudfoundry.org/lager.Logger | ||
|
There was a problem hiding this comment. this caught my attention, it's something we seem to only be doing here $ ~/workspace/concourse/atc $ ag -Q "lager.Logger" | grep counter
api/pipelineserver/archive_test.go:16://go:generate counterfeiter code.cloudfoundry.org/lager.Loggerdo you think this is something we should adopt more? I'm curious about the rationale to testing the log statements here, and would challenge is a bit: if we were adding a (please don't take the above as criticism 😅 curious to know more about it) thanks! There was a problem hiding this comment. I've spent a lot of time inspecting logs from Concourse with no access to the cluster. Based on this, I'm shocked by how often I really wish Concourse was logging something and I find that it's not. In general I haven't yet felt like it's necessary to unit-test every log message, but there are times and places where logging feels like a really desirable feature with real business value and I want a test to verify that it will definitely be happening - in such cases I'd be pretty upset if the log stopped happening haha. |
||
|
|
||
| var _ = Describe("Archive Handler", func() { | ||
| var ( | ||
| fakeLogger *pipelineserverfakes.FakeLogger | ||
| server *pipelineserver.Server | ||
| dbPipeline *dbfakes.FakePipeline | ||
| handler http.Handler | ||
| recorder *httptest.ResponseRecorder | ||
| request *http.Request | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| fakeLogger = new(pipelineserverfakes.FakeLogger) | ||
| server = pipelineserver.NewServer( | ||
| fakeLogger, | ||
| new(dbfakes.FakeTeamFactory), | ||
| new(dbfakes.FakePipelineFactory), | ||
| "", | ||
| true, /* enableArchivePipeline */ | ||
| ) | ||
| dbPipeline = new(dbfakes.FakePipeline) | ||
| handler = server.ArchivePipeline(dbPipeline) | ||
| recorder = httptest.NewRecorder() | ||
| request = httptest.NewRequest("PUT", "http://example.com", nil) | ||
| }) | ||
|
|
||
| It("logs database errors", func() { | ||
| expectedError := errors.New("db error") | ||
| dbPipeline.ArchiveReturns(expectedError) | ||
|
|
||
| handler.ServeHTTP(recorder, request) | ||
|
|
||
| Expect(fakeLogger.ErrorCallCount()).To(Equal(1)) | ||
| action, actualError, _ := fakeLogger.ErrorArgsForCall(0) | ||
| Expect(action).To(Equal("archive-pipeline"), "wrong action name") | ||
| Expect(actualError).To(Equal(expectedError)) | ||
| }) | ||
|
|
||
| It("write a debug log on every request", func() { | ||
| handler.ServeHTTP(recorder, request) | ||
|
|
||
| Expect(fakeLogger.DebugCallCount()).To(Equal(1)) | ||
| action, _ := fakeLogger.DebugArgsForCall(0) | ||
| Expect(action).To(Equal("archive-pipeline"), "wrong action name") | ||
| }) | ||
|
|
||
| It("logs no errors if everything works", func() { | ||
| dbPipeline.ArchiveReturns(nil) | ||
|
|
||
| handler.ServeHTTP(recorder, request) | ||
|
|
||
| Expect(fakeLogger.ErrorCallCount()).To(Equal(0)) | ||
| }) | ||
|
|
||
| Context("when the endpoint is not enabled", func() { | ||
| BeforeEach(func() { | ||
| server = pipelineserver.NewServer( | ||
| fakeLogger, | ||
| new(dbfakes.FakeTeamFactory), | ||
| new(dbfakes.FakePipelineFactory), | ||
| "", | ||
| false, /* enableArchivePipeline */ | ||
| ) | ||
| handler = server.ArchivePipeline(dbPipeline) | ||
| }) | ||
|
|
||
| It("responds with status Forbidden", func() { | ||
| handler.ServeHTTP(recorder, request) | ||
|
|
||
| Expect(recorder.Code).To(Equal(http.StatusForbidden)) | ||
| body, _ := ioutil.ReadAll(recorder.Body) | ||
| Expect(body).To(Equal([]byte("endpoint is not enabled\n"))) | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same suite there are quite a bit of tests that also validate what happens when an unauthorized users tries to reach the corresponding endpoint
e.g.
concourse/atc/api/pipelines_test.go
Lines 409 to 417 in 004e924
wdyt of adding tests for this one too? it seems to me that the big purpose of this more "end-to-end" approach of testing at the
apilevel would be to catch these things 🤔 please correct me if i'm wrong! 😁There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we tried to drive out this feature by pretty rigorously practicing TDD. While pairing with @aoldershaw and @YoussB on different occasions, I recall weighing the decision to add these kinds of tests, and they felt redundant since we were already adding the table test entries for accessor itself. Indeed I think if we followed the boilerplate of the other tests, the newly-added tests would never actually go red. However, leaving them out is really only safe if we trust the other components we are integrating with and we have a solid contract with them. For example, I think there is a component with a name like 'api auth wrappa' that is conceptually 'responsible' for the behaviours you are describing, but we don't have a unit test anywhere saying "this code delegates to the api auth wrappa", and maybe we should? After all, if we ended up switching away from using the api auth wrappa, there would be nothing ensuring this "sensible default" behaviour.