implement controllers for AWS IRSA #7735
Conversation
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Signed-off-by: nithyatsu <nithyasu@microsoft.com>
Signed-off-by: nithyatsu <nithyasu@microsoft.com>
Signed-off-by: nithyatsu <nithyasu@microsoft.com>
Signed-off-by: nithyatsu <nithyasu@microsoft.com>
nithyatsu
force-pushed
the
irsa_model_controller2
branch
from
cd8eb33 to
32053b1
Compare
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Signed-off-by: nithyatsu <nithyasu@microsoft.com>
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Unit Tests3 264 tests +2 3 258 ✅ +2 4m 1s ⏱️ -8s Results for commit bbd1bca. ± Comparison against base commit 0782511. This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7735 +/- ##
==========================================
+ Coverage 61.00% 61.26% +0.25%
==========================================
Files 520 520
Lines 27010 27441 +431
==========================================
+ Hits 16478 16811 +333
- Misses 9080 9166 +86
- Partials 1452 1464 +12 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Nithya Subramanian <98416062+nithyatsu@users.noreply.github.com>
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
nithyatsu
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
| } | ||
| case ucp_datamodel.AWSIRSACredentialKind: | ||
| if credentials.IRSACredential == nil || credentials.IRSACredential.RoleARN == "" { | ||
| logger.Info("AWS IRSACredential is not registered, skipping credentials configuration.") |
There was a problem hiding this comment.
Should IRSACredential be one word or can it be split?
There was a problem hiding this comment.
@ytimocin I am not sure what is meant by split. Could you please elaborate a bit.
| AccessKeyCredential: &ucp_datamodel.AWSAccessKeyCredentialProperties{ | ||
| AccessKeyID: "testAccessKey", | ||
| SecretAccessKey: "testSecretKey", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Should we have IRSACredential unit tests?
There was a problem hiding this comment.
Yes, I going to add them as part of Terraform RP changes. I had to add minimal changes in the PR so that it would compile with new data model. But I have not yet implemented Terraform support.
| }, | ||
| } | ||
| case nil: | ||
| return nil, &v1.ErrModelConversion{PropertyName: "$.properties.storage", ValidValue: "not nil"} |
There was a problem hiding this comment.
Instead of not nil should we have a proper error message?
There was a problem hiding this comment.
I think we have this since storage is a structure like below:
"storage":{
"kind":"Internal"
}
We seem to return the "not nil" error message for all structures, for example
case *InternalCredentialStorageProperties:
if c.Kind == nil {
return nil, &v1.ErrModelConversion{PropertyName: "$.properties", ValidValue: "not nil"}
}
There was a problem hiding this comment.
Can you reformat the JSON files?
| Kind: ucp_datamodel.AWSAccessKeyCredentialKind, | ||
| AccessKeyCredential: &ucp_datamodel.AWSAccessKeyCredentialProperties{ | ||
| AccessKeyID: "fakeid", | ||
| SecretAccessKey: "fakesecretkey", | ||
| }, |
There was a problem hiding this comment.
Should we have IRSA unit tests here?
There was a problem hiding this comment.
The IRSA UT are coming as part of UCP PR for supporting IRSA. (similar to terraform)
pkg/ucp/frontend/controller/credentials/aws/createorupdateawscredential_test.go
Show resolved
Hide resolved
There was a problem hiding this comment.
Why did we delete these files? Are we going to replace them?
There was a problem hiding this comment.
yes, earlier we had just one kind so this file actually contained the accesskey credential example. I deleted it ( renamed to accesskey) and added the irsa example.json, as part of tsp PR (https://github.com/radius-project/radius/pull/7708/files) . I had missed deleting these files though.
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
|
continuing the PR in #7739 |
nithyatsu
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Description
Add convertor and controllers for the new IRSA credential type
Type of change
Partially Fixes: #7618