[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5801: URL Sig Keys in Postgres and DELETE methods

[GitBox <gi...@apache.org>]

rawlinp commented on a change in pull request #5801:
URL: https://github.com/apache/trafficcontrol/pull/5801#discussion_r624110807



##########
File path: CHANGELOG.md
##########
@@ -37,6 +37,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Add a Federation to the Ansible Dataset Loader
 - Added asynchronous status to ACME certificate generation.
 - Added headers to Traffic Portal, Traffic Ops, and Traffic Monitor to opt out of tracking users via Google FLoC.
+- `DELETE` request method for `deliveryservices/xmlId/{name}/urlkeys` and `deliveryservices/{id}/urlkeys`.
+- Added URL Sig Keys support for Postgres Traffic Vault.

Review comment:
       I already added a line in the changelog saying that postgres support for traffic vault has been added, so we don't really need to include these for every PR (the `DELETE` one should stay though)

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/url_sig_keys.go
##########
@@ -0,0 +1,73 @@
+package postgres
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"database/sql"
+	"encoding/json"
+	"errors"
+
+	"github.com/apache/trafficcontrol/lib/go-tc"
+
+	"github.com/jmoiron/sqlx"
+)
+
+func getURLSigKeys(xmlID string, tvTx *sqlx.Tx) (tc.URLSigKeys, bool, error) {
+	var jsonUrlKeys string
+	if err := tvTx.QueryRow("SELECT data FROM url_sig_key WHERE deliveryservice = $1", xmlID).Scan(&jsonUrlKeys); err != nil {
+		if err == sql.ErrNoRows {
+			return tc.URLSigKeys{}, true, nil
+		}
+		return tc.URLSigKeys{}, false, err
+	}
+
+	urlSignKey := tc.URLSigKeys{}
+	err := json.Unmarshal([]byte(jsonUrlKeys), &urlSignKey)
+	if err != nil {
+		return tc.URLSigKeys{}, false, errors.New("unmarshalling keys: " + err.Error())
+	}
+
+	return urlSignKey, true, nil
+}
+
+func putURLSigKeys(xmlID string, tvTx *sqlx.Tx, keys tc.URLSigKeys) error {
+	keyJSON, err := json.Marshal(&keys)
+	if err != nil {
+		return errors.New("marshalling keys: " + err.Error())
+	}
+
+	res, err := tvTx.Exec("INSERT INTO url_sig_key (deliveryservice, data) VALUES ($1, $2)", xmlID, keyJSON)

Review comment:
       This query needs to be idempotent, so we may need to `DELETE FROM url_sig_key WHERE deliveryservice = $1` before running the `INSERT`. The way this is currently, the request will fail if someone tries to generate URL sig keys more than once on the same DS without deleting them first.

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go
##########
@@ -133,11 +133,32 @@ func (p *Postgres) DeleteDNSSECKeys(cdnName string, tx *sql.Tx, ctx context.Cont
 }
 
 func (p *Postgres) GetURLSigKeys(xmlID string, tx *sql.Tx, ctx context.Context) (tc.URLSigKeys, bool, error) {
-	return tc.URLSigKeys{}, false, notImplementedErr
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return tc.URLSigKeys{}, false, err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+	return getURLSigKeys(xmlID, tvTx)
 }
 
 func (p *Postgres) PutURLSigKeys(xmlID string, keys tc.URLSigKeys, tx *sql.Tx, ctx context.Context) error {
-	return notImplementedErr
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+
+	return putURLSigKeys(xmlID, tvTx, keys)
+}
+
+func (p *Postgres) DeleteURLSigKeys(xmlID string, tx *sql.Tx, ctx context.Context) error {
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+
+	return deleteURLSigKeys(xmlID, tvTx)

Review comment:
       ditto prior comment

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/url_sig_keys.go
##########
@@ -0,0 +1,73 @@
+package postgres
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"database/sql"
+	"encoding/json"
+	"errors"
+
+	"github.com/apache/trafficcontrol/lib/go-tc"
+
+	"github.com/jmoiron/sqlx"
+)
+
+func getURLSigKeys(xmlID string, tvTx *sqlx.Tx) (tc.URLSigKeys, bool, error) {
+	var jsonUrlKeys string
+	if err := tvTx.QueryRow("SELECT data FROM url_sig_key WHERE deliveryservice = $1", xmlID).Scan(&jsonUrlKeys); err != nil {
+		if err == sql.ErrNoRows {
+			return tc.URLSigKeys{}, true, nil
+		}
+		return tc.URLSigKeys{}, false, err

Review comment:
       Before this return is where we should call `checkErrWithContext` in case the context was cancelled

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/url_sig_keys.go
##########
@@ -0,0 +1,73 @@
+package postgres
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"database/sql"
+	"encoding/json"
+	"errors"
+
+	"github.com/apache/trafficcontrol/lib/go-tc"
+
+	"github.com/jmoiron/sqlx"
+)
+
+func getURLSigKeys(xmlID string, tvTx *sqlx.Tx) (tc.URLSigKeys, bool, error) {
+	var jsonUrlKeys string
+	if err := tvTx.QueryRow("SELECT data FROM url_sig_key WHERE deliveryservice = $1", xmlID).Scan(&jsonUrlKeys); err != nil {
+		if err == sql.ErrNoRows {
+			return tc.URLSigKeys{}, true, nil

Review comment:
       I think this should return `false` because the row didn't exist and was hence "not found"

##########
File path: traffic_ops/v4-client/deliveryservice.go
##########
@@ -329,6 +335,25 @@ func (to *Session) GetDeliveryServiceURLSigKeys(dsName string, header http.Heade
 	return data.Response, reqInf, nil
 }
 
+// CreateDeliveryServiceURLSigKeys creates new URL-signing keys used by the Delivery Service
+// identified by the XMLID 'dsName'
+func (to *Session) CreateDeliveryServiceURLSigKeys(dsName string, header http.Header) (tc.Alerts, toclientlib.ReqInf, error) {
+	var alerts tc.Alerts
+	reqInf, err := to.post(fmt.Sprintf(APIDeliveryServicesURLSigKeysGenerate, dsName), nil, header, &alerts)
+	if err != nil {

Review comment:
       This `err != nil` check is redundant, we can just return `err` regardless

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/url_sig_keys.go
##########
@@ -0,0 +1,73 @@
+package postgres
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"database/sql"
+	"encoding/json"
+	"errors"
+
+	"github.com/apache/trafficcontrol/lib/go-tc"
+
+	"github.com/jmoiron/sqlx"
+)
+
+func getURLSigKeys(xmlID string, tvTx *sqlx.Tx) (tc.URLSigKeys, bool, error) {
+	var jsonUrlKeys string
+	if err := tvTx.QueryRow("SELECT data FROM url_sig_key WHERE deliveryservice = $1", xmlID).Scan(&jsonUrlKeys); err != nil {
+		if err == sql.ErrNoRows {
+			return tc.URLSigKeys{}, true, nil
+		}
+		return tc.URLSigKeys{}, false, err
+	}
+
+	urlSignKey := tc.URLSigKeys{}
+	err := json.Unmarshal([]byte(jsonUrlKeys), &urlSignKey)
+	if err != nil {
+		return tc.URLSigKeys{}, false, errors.New("unmarshalling keys: " + err.Error())
+	}
+
+	return urlSignKey, true, nil
+}
+
+func putURLSigKeys(xmlID string, tvTx *sqlx.Tx, keys tc.URLSigKeys) error {
+	keyJSON, err := json.Marshal(&keys)
+	if err != nil {
+		return errors.New("marshalling keys: " + err.Error())
+	}
+
+	res, err := tvTx.Exec("INSERT INTO url_sig_key (deliveryservice, data) VALUES ($1, $2)", xmlID, keyJSON)
+	if err != nil {
+		return err

Review comment:
       Before this return is where we should call `checkErrWithContext` in case the context was cancelled

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go
##########
@@ -133,11 +133,32 @@ func (p *Postgres) DeleteDNSSECKeys(cdnName string, tx *sql.Tx, ctx context.Cont
 }
 
 func (p *Postgres) GetURLSigKeys(xmlID string, tx *sql.Tx, ctx context.Context) (tc.URLSigKeys, bool, error) {
-	return tc.URLSigKeys{}, false, notImplementedErr
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return tc.URLSigKeys{}, false, err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+	return getURLSigKeys(xmlID, tvTx)
 }
 
 func (p *Postgres) PutURLSigKeys(xmlID string, keys tc.URLSigKeys, tx *sql.Tx, ctx context.Context) error {
-	return notImplementedErr
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+
+	return putURLSigKeys(xmlID, tvTx, keys)

Review comment:
       ditto prior comment

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/url_sig_keys.go
##########
@@ -0,0 +1,73 @@
+package postgres
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"database/sql"
+	"encoding/json"
+	"errors"
+
+	"github.com/apache/trafficcontrol/lib/go-tc"
+
+	"github.com/jmoiron/sqlx"
+)
+
+func getURLSigKeys(xmlID string, tvTx *sqlx.Tx) (tc.URLSigKeys, bool, error) {
+	var jsonUrlKeys string
+	if err := tvTx.QueryRow("SELECT data FROM url_sig_key WHERE deliveryservice = $1", xmlID).Scan(&jsonUrlKeys); err != nil {
+		if err == sql.ErrNoRows {
+			return tc.URLSigKeys{}, true, nil
+		}
+		return tc.URLSigKeys{}, false, err
+	}
+
+	urlSignKey := tc.URLSigKeys{}
+	err := json.Unmarshal([]byte(jsonUrlKeys), &urlSignKey)
+	if err != nil {
+		return tc.URLSigKeys{}, false, errors.New("unmarshalling keys: " + err.Error())
+	}
+
+	return urlSignKey, true, nil
+}
+
+func putURLSigKeys(xmlID string, tvTx *sqlx.Tx, keys tc.URLSigKeys) error {
+	keyJSON, err := json.Marshal(&keys)
+	if err != nil {
+		return errors.New("marshalling keys: " + err.Error())
+	}
+
+	res, err := tvTx.Exec("INSERT INTO url_sig_key (deliveryservice, data) VALUES ($1, $2)", xmlID, keyJSON)
+	if err != nil {
+		return err
+	}
+	if rowsAffected, err := res.RowsAffected(); err != nil {
+		return err
+	} else if rowsAffected == 0 {
+		return errors.New("URL Sign Keys: no keys were inserted")
+	}
+	return nil
+}
+
+func deleteURLSigKeys(xmlID string, tvTx *sqlx.Tx) error {
+	if _, err := tvTx.Exec("DELETE FROM url_sig_key WHERE deliveryservice = $1", xmlID); err != nil {
+		return err

Review comment:
       Before this return is where we should call `checkErrWithContext` in case the context was cancelled

##########
File path: traffic_ops/traffic_ops_golang/trafficvault/backends/postgres/postgres.go
##########
@@ -133,11 +133,32 @@ func (p *Postgres) DeleteDNSSECKeys(cdnName string, tx *sql.Tx, ctx context.Cont
 }
 
 func (p *Postgres) GetURLSigKeys(xmlID string, tx *sql.Tx, ctx context.Context) (tc.URLSigKeys, bool, error) {
-	return tc.URLSigKeys{}, false, notImplementedErr
+	tvTx, dbCtx, cancelFunc, err := p.beginTransaction(ctx)
+	if err != nil {
+		return tc.URLSigKeys{}, false, err
+	}
+	defer p.commitTransaction(tvTx, dbCtx, cancelFunc)
+	return getURLSigKeys(xmlID, tvTx)

Review comment:
       I think we may want to pass `ctx` into these function as well in order to use `checkErrWithContext` after attempting the DB query. That function is basically there to give us better context in the error message in case the client closed the connection or the query timed out -- both of which _cancel_ the context, cancelling the DB query.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org