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

Extend valid resources for project-scoped role policy #207

Merged
merged 10 commits into from
Oct 13, 2022

Conversation

onematchfox
Copy link
Collaborator

Enables the ability to create project-role policies for the resources clusters, applications, exec and logs.

Notes:

  • Bumping the argo-cd go dependency required dropping support for ksonnet as per the 2.4 release notes. Not sure quite how you want to handle this from the perspective of this provider given that it could definitely be seen as a breaking change. Let me know if anything is needed from my side.
  • Replacement for Update ArgoCD dependency to v2.4.x #203 which went stale.
  • I have upgraded the default Kubernetes and ArgoCD versions that are installed as part of the tests (see commit messages for more details) - happy to drop these commits if you have any issues with them.

Support for `ksonnet` was dropped in ArgoCD 2.4. As such, this has been removed from the `argocd_application` resource in this provider as well.
`1.19` is long past EOL so we should upgrade the default cluster version. I did consider `1.22` but as this is approaching EOL in 1 month (https://kubernetes.io/releases/), I figured we may as well jump straight to `1.23`.
Aligns with the minimum version run by the GitHub Actions.
Ensures that role policy can be provisioned for clusters, repositories, exec and logs.
Resolves following build error resulting from dependency updates:

```
../../../go/pkg/mod/github.com/argoproj/argo-cd/[email protected]/pkg/apiclient/apiclient.go:53:2: //go:build comment without // +build comment
```
@onematchfox
Copy link
Collaborator Author

onematchfox commented Sep 29, 2022

@oboukili is it just me or are the tests a bit flaky? v2.1.10 passed on the previous attempt but failed on the latest attempt (for seemingly unrelated reasons).

Will leave this PR as is for now. Let me know if there's anything I need to do about the tests (in addition to any review comments of course 😄)

@lchastel
Copy link

lchastel commented Oct 5, 2022

I need features of 2.4 version of ArgoCD. (rbac from projects).

Could you please fix the issue and publish the new version ?

PS : I am wondering if support of 2.1 is still needed ? (it not supported anymore according to documentation)

@onematchfox
Copy link
Collaborator Author

Could you please fix the issue and publish the new version ?

I'm afraid there's not much I can do here - I'm waiting on a review. I'm sure @oboukili will get to this when he has the time.

PS : I am wondering if support of 2.1 is still needed ? (it not supported anymore according to documentation)

🤔 this is a fair point - even by the providers own compatibility promise versions 2.1 and 2.2 are unsupported. So, I could drop the tests for these versions. But, I would rather wait for a maintainer's thoughts on the matter than make that call myself.

fi

echo "\n--- Install ArgoCD ${ARGOCD_VERSION:-v1.8.7}\n"
curl https://raw.githubusercontent.com/argoproj/argo-cd/${ARGOCD_VERSION:-v1.8.7}/manifests/install.yaml > manifests/install/argocd.yml &&
echo "\n--- Install ArgoCD ${ARGOCD_VERSION:-v2.1.10}\n"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop support for 2.1.x, as per the compatibility promise, and let's raise this to the latest 2.3.x.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

StateUpgraders: []schema.StateUpgrader{
{
Type: resourceArgoCDApplicationV0().CoreConfigSchema().ImpliedType(),
Upgrade: resourceArgoCDApplicationStateUpgradeV0,
Version: 0,
},
{
Type: resourceArgoCDApplicationV1().CoreConfigSchema().ImpliedType(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a schemaUpgrader test? If something goes wrong, we'll have to release hotfixes under pressure (already experienced that ;) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch :) Added as requested.

@oboukili
Copy link
Collaborator

oboukili commented Oct 6, 2022

Hi there,
Apologies for the slow response time, indeed everybody is pretty busy these days. @onematchfox since you're already a long time contributor, would you mind if I added you as a repository maintainer?
Cheers and thanks,

@onematchfox
Copy link
Collaborator Author

Hi there, Apologies for the slow response time, indeed everybody is pretty busy these days.

Thanks so much for the review. Have now completed the changes as requested.

@onematchfox since you're already a long time contributor, would you mind if I added you as a repository maintainer? Cheers and thanks,

I'm by no means an expert in Terraform providers 😄 but yes sure, I'd be more than happy to help out wherever possible if it would lighten your load.

argocd/utils.go Outdated
@@ -97,9 +97,17 @@ func validatePolicy(project string, role string, policy string) error {
return fmt.Errorf("invalid policy rule '%s': policy subject must be: '%s', not '%s'", policy, expectedSubject, subject)
}
// resource
// See https://github.com/muma378/argo-cd/blob/6fd4c6f44acc06f934061ce7848d72454366345b/pkg/apis/application/v1alpha1/types.go#L1555-L1561
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a comment to refactor this using a data structure rbac.ResourceApplications data structure maintained upstream, did you consider that instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't... Glossed over it because of the related comment for validActions at https://github.com/argoproj/argo-cd/blob/c99669e088b5f25c8ce8faff6df25797a8beb5ba/pkg/apis/application/v1alpha1/types.go#L1527

But now that you mention it, this module won't have any "import cycle" issues. So I have gone ahead and adjusted to reference rbacpolicy instead of hard-coding the strings (handled actions as well while I was at it)

scripts/testacc_prepare_env.sh Outdated Show resolved Hide resolved
scripts/testacc_prepare_env.sh Outdated Show resolved Hide resolved
@oboukili
Copy link
Collaborator

I'm by no means an expert in Terraform providers 😄 but yes sure, I'd be more than happy to help out wherever possible if it would lighten your load.

No worries, I'll set this up. Cheers!

@onematchfox
Copy link
Collaborator Author

onematchfox commented Oct 13, 2022

@oboukili Changes now made. Acceptance Tests on v2.4.12 failed initially for seemingly unrelated reasons so I have relaunched them.

Edit: and all passing now. 🎉

@oboukili
Copy link
Collaborator

Awesome, thanks :)
Since there is a major breaking change, we are going to increment the major version counter.

@oboukili oboukili merged commit 2dbefb5 into argoproj-labs:master Oct 13, 2022
@onematchfox onematchfox deleted the deps/argo-cd-2.4.12 branch October 13, 2022 17:22
@VladoPortos
Copy link

Guys how do I fix this?

Error decoding "argocd_application.gitlab" from previous state: unsupported attribute "ksonnet"

Just remove it from state file ?

@onematchfox
Copy link
Collaborator Author

Were/are you using ksonnet?

Just remove it from state file ?

Yes, that should do the trick.

@VladoPortos
Copy link

Worked, thanks @onematchfox

@oboukili
Copy link
Collaborator

Good point, we should have shipped a state migrator with the latest major release!

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

Successfully merging this pull request may close these issues.

4 participants