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

Include name argument in service account cmd #53

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

smarlowucf
Copy link
Contributor

And use explicit set string option in helm install to handle the account id number as a string.

@smarlowucf smarlowucf requested review from bear454 and apozsuse April 19, 2024 18:52
@smarlowucf
Copy link
Contributor Author

There are two paths for the account id issue. This one uses --set-string to ensure the int is treated as a string. The other option is to set the account id as an int in the helm chart. This is still TBD.

Copy link

@apozsuse apozsuse left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@smarlowucf
Copy link
Contributor Author

Sounds like we will make the change in schema https://github.com/neuvector/neuvector-helm/blob/master/charts/core/values.schema.json#L188 so we can drop the --set-string change. Marking do not merge for now.

@smarlowucf
Copy link
Contributor Author

@bear454 dropped the set string change since change will happen in upstream chart. This is ready for review and merge.

@@ -50,6 +50,7 @@ eksctl create iamserviceaccount \
--namespace neuvector --cluster $CLUSTER_NAME \
--role-name $ROLE_NAME --role-only \
--attach-policy-arn 'arn:aws:iam::aws:policy/AWSMarketplaceMeteringFullAccess' \
--name csp \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the late review, can you explain how this attribute is used?

Copy link
Contributor Author

@smarlowucf smarlowucf May 6, 2024

Choose a reason for hiding this comment

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

It's the name of the ServiceAccount in K8s that gets attached to the container role. https://github.com/neuvector/neuvector-helm/blob/master/charts/core/values.yaml#L51

@bear454 bear454 self-requested a review May 6, 2024 23:56
@smarlowucf smarlowucf merged commit 4a9cd23 into main May 7, 2024
@smarlowucf smarlowucf deleted the nv-updates branch May 7, 2024 18:38
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.

3 participants