fix(analysis): Adding field in YAML to provide region for Sigv4 signing.

[lewinkedrs]

Checklist:

Fixes #2458

• [ b] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO

This provides a region field in the analysis template prometheus_type. The region is then passed into the Sigv4 signer in order to succesfully sign the request as per the sigv4 doc

[github-actions]

Go Published Test Results

1 988 tests   1 988 ✔️  2m 36s ⏱️
   118 suites         0 💤
       1 files           0 ❌

Results for commit f1b5a57.

♻️ This comment has been updated with latest results.

[github-actions]

E2E Tests Published Test Results

    4 files      4 suites   3h 20m 52s ⏱️
  96 tests   84 ✔️   5 💤   7 ❌
394 runs  364 ✔️ 20 💤 10 ❌

For more details on these failures, see this check.

Results for commit f1b5a57.

♻️ This comment has been updated with latest results.

[zachaller]

@lewinkedrs You will need to run make codegen and also sign the DCO

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (9690329) 81.68% compared to head (f1b5a57) 81.67%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2794      +/-   ##
==========================================
- Coverage   81.68%   81.67%   -0.02%     
==========================================
  Files         133      133              
  Lines       20178    20188      +10     
==========================================
+ Hits        16483    16489       +6     
- Misses       2843     2847       +4     
  Partials      852      852              

Impacted Files	Coverage Δ
metricproviders/prometheus/prometheus.go	86.61% <0.00%> (-2.82%)	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lewinkedrs]

@lewinkedrs You will need to run make codegen and also sign the DCO

Hey @zachaller is there a contributors guide or somewhere that says what I need. I guess i need a go-proto library but i see there are a few different ones out there:

/bin/sh: go-to-protobuf: command not found

[zachaller]

@lewinkedrs this could help https://argo-rollouts.readthedocs.io/en/stable/CONTRIBUTING/

[lewinkedrs]

@lewinkedrs this could help https://argo-rollouts.readthedocs.io/en/stable/CONTRIBUTING/

Hey @zachaller thanks for the link, i was able to get started and get all the necessary packages. However, I am running into an error when running make codegen and I can't seem to figure it out.

2023/05/19 08:29:22 internal error: package "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" without types was imported from "github.com/argoproj/argo-rollouts/metric"
make: *** [gen-mocks] Error 1

I take it this is because I modified v1alpha1 as part of this PR. Was there somewhere else I was supposed to add the field for the sigv4 region? Have you seen this error before? Some I tried doing go clean -modcache and I even tried running this in a new sandbox docker container and I still get this error. Maybe there is something different I need to do in the import statement of argo-rollouts/metric ?

[zachaller]

@lewinkedrs can you try moving the repo outside of your GOPATH directory if that is where you have it placed?

[lewinkedrs]

@lewinkedrs can you try moving the repo outside of your GOPATH directory if that is where you have it placed?

He @zachaller I have tried this both in and out of my $GOPATH and still get the same error. I also tried to just reinstall go and still get it. Here is my output from go env in case it is relevant:

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/lewinke/Library/Caches/go-build"
GOENV="/Users/lewinke/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/lewinke/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/lewinke/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.4/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.4/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
AR="ar"
CC="cc"
CXX="c++"
CGO_ENABLED="1"
GOMOD="/Users/lewinke/go/argo-rollouts/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nk/x_h5l5zj5b160p7lxj1t55900000gr/T/go-build4017038431=/tmp/go-build -gno-record-gcc-switches -fno-common"

[zachaller]

I ran codegen but I am wondering if it makes more sense to just document setting an ENV var? I am not sure how much I care for AWS specific configuration under generic prometheus config.

fyi, I think the env var you set is AWS_DEFAULT_REGION or AWS_REGION

[lewinkedrs]

I ran codegen but I am wondering if it makes more sense to just document setting an ENV var? I am not sure how much I care for AWS specific configuration under generic prometheus config.

fyi, I think the env var you set is AWS_DEFAULT_REGION or AWS_REGION

While this does technically work, IMO it is a very nice convenience factor to just pass in the appropriate region in the yaml spec for the analysis, just because IRSA does not provide the region as an ENV var. Perhaps there is also the case where you want to do run an analysis against AMP that lives in a different region. For example, have an analysis template that points at us-east-2 AMP and one at us-west-2 AMP. There are a lot of other open source tools that provide similar helper functionalities to interact with Sigv4 and prometheus such as grafana and open telemetry.

[zachaller]

I ran codegen but I am wondering if it makes more sense to just document setting an ENV var? I am not sure how much I care for AWS specific configuration under generic prometheus config.
fyi, I think the env var you set is AWS_DEFAULT_REGION or AWS_REGION

While this does technically work, IMO it is a very nice convenience factor to just pass in the appropriate region in the yaml spec for the analysis, just because IRSA does not provide the region as an ENV var. Perhaps there is also the case where you want to do run an analysis against AMP that lives in a different region. For example, have an analysis template that points at us-east-2 AMP and one at us-west-2 AMP. There are a lot of other open source tools that provide similar helper functionalities to interact with Sigv4 and prometheus such as grafana and open telemetry.

Ahh yea the multi region per controller use case would not work well with env good callout. I am trying to think if we want to add say an aws: section with region underneath it in case there is any other settings that might want to be set, do you know of any other configs that users might want configurable?

[lewinkedrs]

I ran codegen but I am wondering if it makes more sense to just document setting an ENV var? I am not sure how much I care for AWS specific configuration under generic prometheus config.
fyi, I think the env var you set is AWS_DEFAULT_REGION or AWS_REGION

While this does technically work, IMO it is a very nice convenience factor to just pass in the appropriate region in the yaml spec for the analysis, just because IRSA does not provide the region as an ENV var. Perhaps there is also the case where you want to do run an analysis against AMP that lives in a different region. For example, have an analysis template that points at us-east-2 AMP and one at us-west-2 AMP. There are a lot of other open source tools that provide similar helper functionalities to interact with Sigv4 and prometheus such as grafana and open telemetry.

Ahh yea the multi region per controller use case would not work well with env good callout. I am trying to think if we want to add say an aws: section with region underneath it in case there is any other settings that might want to be set, do you know of any other configs that users might want configurable?

We could add the full SigV4 config the same way it is done in Prometheus. The spec would look something like this:

# Optionally configures AWS's Signature Verification 4 signing process to
# sign requests. Cannot be set at the same time as basic_auth, authorization, or oauth2.
# To use the default credentials from the AWS SDK, use `sigv4: {}`.
sigv4:
  # The AWS region. If blank, the region from the default credentials chain
  # is used.
  [ region: [<string>](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#string) ]

  # The AWS API keys. If blank, the environment variables `AWS_ACCESS_KEY_ID`
  # and `AWS_SECRET_ACCESS_KEY` are used.
  [ access_key: [<string>](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#string) ]
  [ secret_key: [<secret>](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#secret) ]

  # Named AWS profile used to authenticate.
  [ profile: [<string>](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#string) ]

  # AWS Role ARN, an alternative to using AWS API keys.
  [ role_arn: [<string>](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#string) ]

I think in the case of argo though the rest of the fields (access key, secret key, role arn) would almost certainly be coming from the service account.

[zachaller]

Yea if you could update pr to add a top level aws section then something like below, this will just allow us to be a little forward thinking in being able to add more config at a later date if needed/requested

aws:
  sigv4:
    region:

[lewinkedrs]

@zachaller I updated this to look as follows:

provider:
  prometheus:
    address: https://aps-workspaces.$REGION.amazonaws.com/workspaces/$WORKSPACEID
    query: |
      sum(irate(
        istio_requests_total{reporter="source",destination_service=~"{{args.service-name}}",response_code!~"5.*"}[5m]
      )) /
      sum(irate(
        istio_requests_total{reporter="source",destination_service=~"{{args.service-name}}"}[5m]
      ))
    sigv4:
      region: $REGION
      profile: $PROFILE
      roleArn: $ROLEARN

I was thinking that top level aws struct could be confusing because the CloudWatch metric provider does not require this type of SigV4 signing. The reason being that it uses the CloudWatch GO SDK which automatically takes care of this for you. In prometheus it is using the Prometheus api, which we then have to sign manually with the Sigv4 signer.

I am still having issues running make codegen and getting all tests to pass. So any help there would be greatly appreciated.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[danil-smirnov]

Are you going to have this merged folks?