Merge pull request #1109 from circleci/clean-up-httpclient

Remove GET with Body support now RT-724 is complete

Author: SemanticallyNull

httpclient/httpclient.go

@@ -174,11 +174,6 @@ type Request struct {
 	query          url.Values
 	rawquery       string
 
-	// We want to prevent HTTP GETs with body or rawBody due to incompatibilities with CloudFront WAF which API Infra
-	// are introducing. In order to facilitate a migration for runner we need to be able to override this. This can be
-	// removed once RT-724 is completed.
-	allowGETWithBody bool
-
 	propagation bool
 	flatten     string
 
@@ -218,14 +213,6 @@ func RouteParams(routeParams ...interface{}) func(*Request) {
 	}
 }
 
-// AllowGETWithBody will allow the client to send a GET request with a body, which we error on by default. We should
-// remove this once RT-724 is completed.
-func AllowGETWithBody() func(*Request) {
-	return func(r *Request) {
-		r.allowGETWithBody = true
-	}
-}
-
 // Decoder adds a response body decoder to some http status code
 // Note this will modify the original Request.
 //
@@ -793,7 +780,6 @@ func doneRetrying(err error) error {
 func (r Request) validate() error {
 	// We do not allow GET requests with Body as they are not supported by CloudFront WAF, which requests are routed
 	// through. - https://circleci.slack.com/archives/C03M4P0Q4GH/p1659566842825159
-	// This can be overridden with httpclient.AllowGETWithBody() if required for legacy or third-party compatibility
 	if !r.validateGetWithBody() {
 		return errors.New("cannot have GET request with body or raw body")
 	}
@@ -814,7 +800,7 @@ func (r Request) validateOnlyRawBodyOrBody() bool {
 }
 
 func (r Request) validateGetWithBody() bool {
-	if !r.allowGETWithBody && (r.method == "GET" && r.hasBody()) {
+	if r.method == "GET" && r.hasBody() {
 		return false
 	}
 

releases/release/handler.go

@@ -30,7 +30,19 @@ func Handler(cfg HandlerConfig) func(c *gin.Context) {
 		ctx := c.Request.Context()
 		var req Requirements
 
-		bindAndValidate(c, &req)
+		if err := c.BindQuery(&req); err != nil {
+			c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
+				"message": fmt.Sprintf("bad request: %s", err),
+			})
+			return
+		}
+
+		if err := req.Validate(); err != nil {
+			c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
+				"message": fmt.Sprintf("bad request: %s", err),
+			})
+			return
+		}
 
 		// if list is nil, client should never proceed to download
 		if cfg.List == nil {
@@ -70,27 +82,3 @@ func Handler(cfg HandlerConfig) func(c *gin.Context) {
 		}
 	}
 }
-
-func bindAndValidate(c *gin.Context, req *Requirements) {
-	// To support a migration path from JSON body to query params we check the content length header. This can be
-	// removed when RT-724 is done
-	var err error
-	if c.Request.ContentLength != 0 {
-		err = c.BindJSON(&req)
-	} else {
-		err = c.BindQuery(&req)
-	}
-	if err != nil {
-		c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
-			"message": fmt.Sprintf("bad request: %s", err),
-		})
-		return
-	}
-
-	if err = req.Validate(); err != nil {
-		c.AbortWithStatusJSON(http.StatusBadRequest, gin.H{
-			"message": fmt.Sprintf("bad request: %s", err),
-		})
-		return
-	}
-}

releases/release/handler_test.go

@@ -19,81 +19,6 @@ import (
 	"github.com/circleci/ex/testing/testcontext"
 )
 
-func TestHandler_WithBody(t *testing.T) {
-	ctx := testcontext.Background()
-
-	t.Run("Test success", func(t *testing.T) {
-		fix := startAPI(ctx, t)
-
-		t.Run("Can get a release", func(t *testing.T) {
-			agent, err := fix.DownloadWithBody(ctx, release.Requirements{
-				Platform: "linux",
-				Arch:     "amd64",
-			})
-
-			assert.Assert(t, err)
-			assert.Check(t, cmp.DeepEqual(agent, &release.Release{
-				URL:      fix.S3URL + "/1.1.1-abcdef01/linux/amd64/circleci-agent",
-				Checksum: "4a62f09b64873a20386cdbfaca87cc10d8352fab014ef0018f1abcce08a3d027",
-				Version:  "1.1.1-abcdef01",
-			}))
-		})
-	})
-
-	t.Run("Test for unknown arch", func(t *testing.T) {
-		fix := startAPI(ctx, t)
-
-		t.Run("Release not found", func(t *testing.T) {
-			_, err := fix.DownloadWithBody(ctx, release.Requirements{
-				Platform: "linux",
-				Arch:     "enemy",
-			})
-			assert.Check(t, httpclient.HasStatusCode(err, http.StatusNotFound))
-			assert.Check(t, cmp.ErrorContains(err,
-				`404 (Not Found) (1 attempts): no download found for version="1.1.1-abcdef01" os="linux" arch="enemy"`,
-			))
-		})
-	})
-
-	t.Run("Test invalid requests", func(t *testing.T) {
-		fix := startAPI(ctx, t)
-
-		t.Run("No platform", func(t *testing.T) {
-			_, err := fix.DownloadWithBody(ctx, release.Requirements{
-				Arch: "enemy",
-			})
-			assert.Check(t, httpclient.HasStatusCode(err, http.StatusBadRequest))
-			assert.Check(t, cmp.ErrorContains(err,
-				`400 (Bad Request) (1 attempts): bad request: platform is required`,
-			))
-		})
-		t.Run("No arch", func(t *testing.T) {
-			_, err := fix.DownloadWithBody(ctx, release.Requirements{
-				Platform: "linux",
-			})
-			assert.Check(t, httpclient.HasStatusCode(err, http.StatusBadRequest))
-			assert.Check(t, cmp.ErrorContains(err,
-				`400 (Bad Request) (1 attempts): bad request: arch is required`,
-			))
-		})
-	})
-
-	t.Run("Test no downloads", func(t *testing.T) {
-		fix := startAPIWithDownloads(ctx, t, false)
-
-		t.Run("Should give 410", func(t *testing.T) {
-			_, err := fix.DownloadWithBody(ctx, release.Requirements{
-				Platform: "linux",
-				Arch:     "amd64",
-			})
-			assert.Check(t, httpclient.HasStatusCode(err, http.StatusGone))
-			assert.Check(t, cmp.ErrorContains(err,
-				`410 (Gone) (1 attempts): no more downloads possible`,
-			))
-		})
-	})
-}
-
 func TestHandler_WithQuery(t *testing.T) {
 	ctx := testcontext.Background()
 
@@ -169,10 +94,6 @@ func TestHandler_WithQuery(t *testing.T) {
 	})
 }
 
-func (f *fixture) DownloadWithBody(ctx context.Context, requirements release.Requirements) (*release.Release, error) {
-	return f.download(ctx, httpclient.Body(requirements), httpclient.AllowGETWithBody())
-}
-
 func (f *fixture) DownloadWithQuery(ctx context.Context, requirements release.Requirements) (*release.Release, error) {
 	return f.download(ctx, httpclient.QueryParams(map[string]string{
 		"arch":    requirements.Arch,