Ads.Cert Call Sign support #2241

VeronikaSolovei9 · 2022-05-06T07:12:17Z

Call Sign support for Prebid Server. For more info refer to adscertsigner.md

config/bidderinfo.go

config/config.go

config/experiment.go

exchange/bidder.go

experiment/adscert/signer.go

go.mod

experiment/adscert/signer.go

# Conflicts: # exchange/exchange.go

VeronikaSolovei9 · 2022-05-25T06:07:54Z

What kind of unit tests can be added here?

VeronikaSolovei9 · 2022-05-26T02:19:05Z

Discussed unit tests offline. Will add them in next commit

config/bidderinfo.go

config/config.go

exchange/bidder.go

exchange/bidder_test.go

experiment/adscert/README.md

experiment/adscert/signer.go

SyntaxNode · 2022-06-02T03:10:15Z

config/bidderinfo.go

+	Experiment              BidderInfoExperiment `yaml:"experiment"`
+}
+
+// BidderInfoExperiment specified non-production ready features.


Nitpick: specified -> specifies

SyntaxNode · 2022-06-02T03:12:09Z

config/experiment.go

+	//Url - address of grpc server
+	Url string `mapstructure:"url"`
+	//SigningTimeoutMs specifies how long this client will wait for signing to finish before abandoning
+	SigningTimeoutMs int `mapstructure:"signing_timeout"`


I like that you included the ms unit in the name. Could you please carry that over into the map structure name too so the unit is available in the config?

SyntaxNode · 2022-06-02T03:12:40Z

docs/developers/adscertsigner.md

@@ -0,0 +1,90 @@
+##Ads Cert


I recommend moving this up to just /docs instead of /docs/developers. Make it easier to discover. :)

SyntaxNode · 2022-06-02T03:13:13Z

endpoints/openrtb2/auction_benchmark_test.go

@@ -88,6 +88,7 @@ func BenchmarkOpenrtbEndpoint(b *testing.B) {
 		func(ctx context.Context, id uint16) (vendorlist.VendorList, error) { return nil, nil },
 		currency.NewRateConverter(&http.Client{}, "", time.Duration(0)),
 		empty_fetcher.EmptyFetcher{},
+		nil,


Consider using a NilSigner here instead to document what the argument is for.

Yes, replaced in all occurrences

SyntaxNode · 2022-06-02T03:14:03Z

exchange/bidder_test.go

@@ -704,8 +704,8 @@ func TestMultiCurrencies(t *testing.T) {
 			1,
 			currencyConverter.Rates(),
 			&adapters.ExtraRequestInfo{},
-			true,
-			true,
+			nil,


Same for all of these tests. A NilSigner would be self-documenting and easier for the reader to understand.

Yes, replaced in all occurrences

experiment/adscert/signer.go

SyntaxNode · 2022-06-02T03:23:08Z

experiment/adscert/signer.go

+	opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
+	conn, err := grpc.Dial(remoteSignerConfig.Url, opts...)
+	if err != nil {
+		fmt.Errorf("failed to dial remote signer: %v", err)


Looks like you're missing a return statement here. Consider adding a unit test. I don't know if that's feasible for the dialer code though. Maybe Google or StackOverflow know of some tricks to test something like this?

Added return statement.

Unit tests with fake input data fail with null pointer exception that I cannot "catch"

SyntaxNode · 2022-06-02T03:24:24Z

experiment/adscert/signer.go

+	if err != nil {
+		fmt.Errorf("failed to dial remote signer: %v", err)
+	}
+	//defer conn.Close() -- where this should be?


Ideally, this should be triggered when the app is gracefully shutting down. You might be able to just get away without it though, since the connections are expected to remain open for the life of the application.

Right, this should live and die with application. I removed this line :/

SyntaxNode · 2022-06-02T03:24:51Z

experiment/adscert/signer.go

+
+func (ns *NilSigner) Sign(destinationURL string, body []byte) (string, error) {
+	return "", nil
+}


Do you think it would be a good idea to separate the in process signer, remote signer, and nil signer to their own files?

Separated In-Process and Remote signers to separate files. NilSigner is now in the same file with interface. Should I move it too?

SyntaxNode · 2022-06-02T03:26:07Z

experiment/adscert/signer_test.go

+			assert.EqualError(t, err, "Test error", "incorrect error")
+		} else {
+			if test.operationStatusOk {
+				assert.NoError(t, err, "error should not be returned")


IMHO there is no need to include an error message like "error should not be returned" since that's pretty obvious in the test results. I usually include the name/description of the test case and what i'm asserting to help with debugging, since those will not automatically appear in the test results.

# Conflicts: # config/config.go # exchange/exchange.go

SyntaxNode · 2022-07-11T20:42:43Z

config/experiment.go

+
+// AdsCertInProcess configures data to sign requests using ads certs library in core PBS logic
+type AdsCertInProcess struct {
+	//Origin is ads.cert hostname for the originating party


Nitpick: Please add a space before the comment (ie // Origin) for the field comments for AdsCertInProcess and AdsCertRemote.

SyntaxNode · 2022-07-11T20:43:30Z

exchange/bidder_test.go

+		httpRequest: &adapters.RequestData{
+			Method:  "POST",
+			Uri:     server.URL,
+			Body:    []byte("{\"key\":\"val\"}"),


Nitpick: You can use the backtick string notation here too.

SyntaxNode · 2022-07-11T20:44:48Z

exchange/exchange_test.go

+		Adapters: make(map[string]config.Adapter, 1),
+	}
+	cfg.Adapters["appnexus"] = config.Adapter{
+		Endpoint: "test.com", //Note the '&' character in there


Is this a left over copy/paste? I don't see an & character.

Copy paste, yes, sorry, fixed.

SyntaxNode · 2022-07-11T20:45:40Z

exchange/exchange_test.go

@@ -4073,6 +4074,132 @@ func TestAuctionDebugEnabled(t *testing.T) {

 }

+func TestExperimentConfigs(t *testing.T) {
+	//This test just pass experiment ads cert configs to HoldAuction


Consider choosing a more descriptive test name and removing the comment. Looks like the test assets and error, but is that a real error from trying to determine if test.com has a call sign registered?

Renamed to TestPassExperimentConfigsToHoldAuction. This test runs HoldAuction with MockSigner and only tests correct data was passed to the signer.

SyntaxNode · 2022-07-11T20:53:22Z

experiment/adscert/inprocesssigner.go

+}
+
+func validateInProcessSignerConfig(inProcessSignerConfig config.AdsCertInProcess) error {
+	_, err := url.ParseRequestURI(inProcessSignerConfig.Origin)


I believe all config validation logic is currently in config.fo, so it moving it there would keep it central

I understand about knowing which side to configure. I didn't focus on that before. Perhaps we need to change Enabled to Mode (or some such, whatever language we use elsewhere) which can be something like "off", "inprocess" or "remote". Then we know exactly the host's intent on which one to validate, and also perhaps that the other choice has not been configured.

SyntaxNode · 2022-07-11T20:54:43Z

experiment/adscert/remotesigner.go

+		return nil, err
+	}
+	// Establish the gRPC connection that the client will use to connect to the
+	// signatory server.  Authenticated connections are not implemented at this time


Eeek! This now reads "Authenticated connections are not implemented at this time" .. but the problem is this PR is literally implementing "ads.cert authenticated connections". Sorry for misguiding you here. How about:

Secure connections are not implemented at this time.

SyntaxNode · 2022-07-11T20:55:12Z

experiment/adscert/signer.go

+	errBothSignersSpecified = errors.New("both inprocess and remote signers are specified. Please use just one signer")
+)
+
+//Signer represents interface to access request Ads Cert signing functionality


Nitpick: Add space at the start of the comment.

….mode

VeronikaSolovei9 · 2022-07-13T05:30:18Z

Modified config from experiment.adscert.enabled to experiment.adscert.mode

mansinahar · 2022-07-13T16:54:16Z

config/config.go

+	ErrInvalidRemoteSignerURL                   = errors.New("invalid url for remote signer")
+	ErrInvalidRemoteSignerSigningTimeout        = errors.New("invalid signing timeout for remote signer")
+
+	AdCertsSignerModeOff       = "off"


Wouldn't it make more sense to have these defined as constants?

Also, I'd recommend moving all these values to experiment.go since they are AdsCert specific and all the other AdsCert specific structs live there

Moved to experiment.go. Extracted to constants.

mansinahar · 2022-07-13T17:01:52Z

config/config.go

@@ -139,6 +156,41 @@ type AuctionTimeouts struct {
 	Max uint64 `mapstructure:"max"`
 }

+func (cfg *Experiment) validate(errs []error) []error {


We should also move this validate method over to experiment.go since the Experiment struct is defined there. I'd group the struct and the methods on that struct in the same file.

Agreed, moved

mansinahar · 2022-07-15T17:26:02Z

config/config.go

+	if cfg.AdCerts.Mode == AdCertsSignerModeInprocess {
+		_, err := url.ParseRequestURI(cfg.AdCerts.InProcess.Origin)
+		if err != nil {
+			errs = append(errs, ErrInProcessSignerInvalidURL)


I'd recommend appending the actual values that are set in the config to the error messages so that it's clear from the error message why that value is invalid. You'll see similar behavior in other config validate functions as well.

This is how it was before and modified due to this discussion: #2241 (comment)

appending the actual values - I read it like appending actual error strings to array. Modified

mansinahar · 2022-07-15T17:31:50Z

exchange/bidder.go

@@ -164,6 +172,13 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest Bidde
 			if reqInfo.GlobalPrivacyControlHeader == "1" {
 				reqData[i].Headers.Add("Sec-GPC", reqInfo.GlobalPrivacyControlHeader)
 			}
+			if bidRequestMetadata.addCallSignHeader {
+				signatureMessage, err := adCertSigner.Sign(reqData[i].Uri, reqData[i].Body)
+				if err == nil && len(signatureMessage) > 0 {


Separate PR sounds good to me 👍 @VeronikaSolovei9 would you mind just modifying the comment to add a warning message in debug mode as well in addition to the metrics please? Just so that we don't forget about it

mansinahar · 2022-07-15T17:40:48Z

Looking pretty close to me other than the very few minor comments above

mansinahar

LGTM except for the above minor comment regarding the test

mansinahar · 2022-07-27T13:02:53Z

config/experiment_test.go

@@ -98,15 +101,15 @@ func TestExperimentValidate(t *testing.T) {
 				AdCerts: ExperimentAdsCert{Mode: AdCertsSignerModeInprocess, InProcess: AdsCertInProcess{Origin: "http://test.com", PrivateKey: "pk", DNSCheckIntervalInSeconds: -10, DNSRenewalIntervalInSeconds: 10}},
 			},
 			expectErrors:   true,
-			expectedErrors: []error{ErrInProcessSignerInvalidDNSCheckInterval},
+			expectedErrors: []error{fmt.Errorf("invalid dns check interval for inprocess signer: -10")},


fmt.Errorf should be used when you have a formatted error string i.e. when you're using %s, %d, etc for which the values are going to filled in run time. Here for expectedErrors you already have a constant string that you're expecting so you can just use errors.New.

Same for L112, L123 and L124

Agree, I missed it accidentally. Fixed.

vsolovei added 3 commits May 6, 2022 00:11

Initial preparation for Ads.Cert Call Sign support

1312664

First initial working version

1722b81

Added request option and bidder config to enable or disable ads certs

384bb22

mansinahar requested a review from SyntaxNode May 23, 2022 17:33

mansinahar assigned SyntaxNode May 23, 2022

SyntaxNode reviewed May 23, 2022

View reviewed changes

vsolovei added 5 commits May 23, 2022 21:30

Code clean up and README file added

a94d723

Code refactoring and clean up

1547a0b

Unit tests fix

7f91db7

Merge branch 'master' into ads_cert

3e6f780

# Conflicts: # exchange/exchange.go

Merge fixes

82715a9

VeronikaSolovei9 marked this pull request as ready for review May 25, 2022 06:03

mansinahar requested a review from guscarreon May 25, 2022 17:16

mansinahar assigned guscarreon May 25, 2022

vsolovei added 2 commits May 25, 2022 22:42

Code refactoring and clean up

9c28dd6

Added more go docs for newly defines structures

0bc246d

SyntaxNode reviewed May 26, 2022

View reviewed changes

vsolovei added 8 commits May 26, 2022 00:54

Added unit tests

aea4f71

Added unit tests

9bffd99

Added unit tests

b8baa68

Minor clean up

ac47bd5

Added remote signer.

c78b16d

Removed optional code

540c9f8

Added tests for NewAdCertsSigner

26dac9b

Unit tests for remote signer

de1d8fa

SyntaxNode reviewed Jun 2, 2022

View reviewed changes

vsolovei added 2 commits June 2, 2022 14:13

CR fixes part I

4b9e937

CR fixes part II

7b6ffe9

vsolovei added 2 commits July 11, 2022 12:54

Merge branch 'master' into ads_cert

8709b76

# Conflicts: # config/config.go # exchange/exchange.go

Merge conflict fix

72cae9b

SyntaxNode reviewed Jul 11, 2022

View reviewed changes

vsolovei added 3 commits July 11, 2022 16:08

Moved validation to config.go

7486973

Minor fixes

a1177a9

Modified config from experiment.adscert.enabled to experiment.adscert…

d3d50bb

….mode

mansinahar reviewed Jul 15, 2022

View reviewed changes

vsolovei added 3 commits July 18, 2022 16:45

Merge branch 'master' into ads_cert

a92f723

Code review fixes

999e6da

Code review fixes

d0a9966

SyntaxNode previously approved these changes Jul 19, 2022

View reviewed changes

Code review fixes

b9fc196

VeronikaSolovei9 dismissed SyntaxNode’s stale review via b9fc196 July 20, 2022 06:34

SyntaxNode previously approved these changes Jul 20, 2022

View reviewed changes

mansinahar reviewed Jul 27, 2022

View reviewed changes

Code cleanup

5b81715

VeronikaSolovei9 dismissed SyntaxNode’s stale review via 5b81715 July 27, 2022 18:04

Code cleanup

c3c0fc2

mansinahar approved these changes Jul 27, 2022

View reviewed changes

SyntaxNode approved these changes Jul 27, 2022

View reviewed changes

SyntaxNode merged commit 9bfc1ad into master Jul 27, 2022

pm-nilesh-chate pushed a commit to pm-nilesh-chate/prebid-server that referenced this pull request Aug 9, 2022

Ads.Cert Call Sign support (prebid#2241)

b9a8e7b

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Ads.Cert Call Sign support (prebid#2241)

7457d7b

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Ads.Cert Call Sign support (prebid#2241)

aa34e32

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Ads.Cert Call Sign support (prebid#2241)

8a5710a

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

Ads.Cert Call Sign support (prebid#2241)

fe3f1e6

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Ads.Cert Call Sign support (prebid#2241)

ffa7e3f

SyntaxNode deleted the ads_cert branch March 13, 2023 13:55

Ads.Cert Call Sign support #2241

Ads.Cert Call Sign support #2241

Conversation

VeronikaSolovei9 commented May 6, 2022 • edited Loading

VeronikaSolovei9 commented May 25, 2022

VeronikaSolovei9 commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar commented Jul 15, 2022

mansinahar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented May 6, 2022 •

edited

Loading

VeronikaSolovei9 commented May 26, 2022 •

edited

Loading

VeronikaSolovei9 Jul 20, 2022 •

edited

Loading