-
Notifications
You must be signed in to change notification settings - Fork 7
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
Create ACL bindings for kafka #36
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just have a couple of comments / questions
rhoas/kafkas/resource_kafka.go
Outdated
|
||
// required for api, the user id, service account id or * works | ||
// when appended to User: | ||
principal = "User:" + principal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any validation to check if the user passes the principal in as User:blah
or user:blah
or user: blah
, you get my point, in the TF script?
in those cases wouldn't the principal come out as User:User:blah
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just need to set a standard for these inputs and consider anything outside of that not valid. If we define that the user must enter the client_id or user id in the principal field then starting it with User: is automatically not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my preference here would be to have a little validation around the prepending of User:
here but that is just my own opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would normally agree but... then doing what I am in this pr not impossible but a mess, I am using a service account client id as the principle but I would need to prepend it with User:, this afaik is possible but a pain in terraform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now i have realized doing this changing of values can cause issues with terraform syncing after create, i avoided it in this pr in a hacky way but something to think about for the future.
aa91127
to
d320ec3
Compare
@@ -265,7 +265,13 @@ func kafkaCreate(ctx context.Context, d *schema.ResourceData, m interface{}) dia | |||
|
|||
func createACLForKafka(ctx context.Context, api rhoasAPI.Clients, d *schema.ResourceData, kafka *kafkamgmtclient.KafkaRequest) error { | |||
|
|||
acl, ok := d.Get("acl").([]interface{}) | |||
aclInput := d.Get("acl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
Great work @jackdelahunt
Looks good. Sorry for late review |
This pr adds the functionality of creating ACL bindings for any kafka instance, it adds more options within the kafka resource to achieve this. It also adds a acl resource to use with the need for the kafka resource to be created at the same time.
Verify
Terraform config (main.tf)
Commands
make install
terraform init
terraform apply
Expected Results