Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow analysis arguments to get valueFrom Rollout status #1242

Closed
jessesuen opened this issue Jun 2, 2021 · 4 comments
Closed

Allow analysis arguments to get valueFrom Rollout status #1242

jessesuen opened this issue Jun 2, 2021 · 4 comments
Labels
enhancement New feature or request
Milestone

Comments

@jessesuen
Copy link
Member

Summary

Currently, we can pass arguments to AnalysisTemplates from metadata labels and annotations, e.g.

  strategy:
    canary:
      analysis:
        templates:
        - templateName: args-example
        args:
        - name: region
          valueFrom:
            fieldRef:
              fieldPath: metadata.labels['region']

We should also allow passing arguments to AnalysisTemplates whose value is derived from fields in the rollout status.

Use Cases

As part of #1241, we will be surfacing ALB information under the rollout status. For example:

status:
  alb:
    loadBalancer:
      name: k8s-devdeplo-testdele-b9061572ad
      arn: arn:aws:elasticloadbalancing:us-west-2:1234567891011:loadbalancer/app/k8s-devdeplo-testdele-abcdef1234/3a47c96b75571d0e
    canaryTargetGroup:
      name: k8s-devdeplo-testdele-ab72c5b132
      arn: arn:aws:elasticloadbalancing:us-west-2:1234567891011:targetgroup/k8s-devdeplo-testdele-ab72c5b132/df2957821c1214fc
    stableTargetGroup:
      name: k8s-devdeplo-testdele-8832d6519a
      arn: arn:aws:elasticloadbalancing:us-west-2:1234567891011:targetgroup/k8s-devdeplo-testdele-8832d6519a/df2957821c1214fc 

Once that is available, it would then be useful to pass that information to an AnalysisTemplate, so that argo rollouts could perform CloudWatch queries against the canary targetgroup vs. stable targetgroup, like so:

      analysis:
        templates:
        - templateName: cloud-watch-error-rate
        args:
        - name: env
          valueFrom:
            fieldRef:
              fieldPath: status.alb.canaryTargetGroup.name

Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.

@jessesuen jessesuen added the enhancement New feature or request label Jun 2, 2021
@jessesuen jessesuen added this to the v1.1 milestone Jun 2, 2021
@jessesuen jessesuen removed this from the v1.1 milestone Jul 1, 2021
@harikrongali harikrongali added this to the v1.2 milestone Nov 2, 2021
@jessesuen
Copy link
Member Author

To make this feature more useful, instead of single field paths, we should support JSON string objects and enhance analysis to allow dot notation referencing when the supplied argument is a json string.

@noam-codefresh
Copy link
Contributor

noam-codefresh commented Nov 4, 2021

I am not sure I understand this last comment - do you mean something more complex than the example above? Or just having the ability to reference any field in the status, by using dot notation like status.alb.canaryTargetGroup.name?

I was thinking about using https://github.com/stretchr/objx to extract the value from the rollout instance. Is there any library/way you recommend to do it?

@noam-codefresh
Copy link
Contributor

created PR - #1629

alexmt pushed a commit that referenced this issue Jan 12, 2022
…tus (#1242) (#1629)

* use objx to read value from Rollout manifest

Signed-off-by: Noam Gal <[email protected]>

* handle `[]` annotation correcly in BuildArgumentsForRolloutAnalysisRun

Signed-off-by: Noam Gal <[email protected]>

* validate valueFrom correctly

Signed-off-by: Noam Gal <[email protected]>

* use jsonpath instead of objx
return err if path is inavlid in runtime (don't check in validation time)

Signed-off-by: Noam Gal <[email protected]>

* parse path in code, instead of using jsonPath

Signed-off-by: Noam Gal <[email protected]>

* fixed test

Signed-off-by: Noam Gal <[email protected]>

* updated documentation

Signed-off-by: Noam Gal <[email protected]>

* added tests for coverage

Signed-off-by: Noam Gal <[email protected]>

* fixed lint

Signed-off-by: Noam Gal <[email protected]>

* added another test case

Signed-off-by: Noam Gal <[email protected]>

* fixed case when path ends with "]"

Signed-off-by: Noam Gal <[email protected]>

* removed objx dependency

Signed-off-by: Noam Gal <[email protected]>
@jessesuen
Copy link
Member Author

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants