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

Kubernetes Client: Use the correct service account name #25750

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

Sgitario
Copy link
Contributor

When not setting any name, the extension will use the current resource name which is not a correct logic (as there could have additional resources from users).

Fix #25688

When not setting any name, the extension will use the current resource name which is not a correct logic (as there could have additional resources from users). 

Fix quarkusio#25688
@geoand geoand requested a review from iocanel May 24, 2022 09:35

/**
* Workaround for: https://github.com/dekorateio/dekorate/issues/987
* Once the issue is fixed in upstream, and we bump the Dekorate version, we should delete this decorator.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reported issue on Dekorate side: dekorateio/dekorate#987
I've also provided the fix there.

@Sgitario
Copy link
Contributor Author

/cc @geoand, @iocanel, @edeandrea

@edeandrea
Copy link
Contributor

edeandrea commented May 24, 2022

@Sgitario Is there a config option to tell the extension not to generate a role binding/service account? In my case I'm providing the RoleBindings myself via the user-provided yamls & I don't necessarily want to create new service accounts.

@Sgitario
Copy link
Contributor Author

@Sgitario Is there a config option to tell the extension not to generate a role binding/service account? In my case I'm providing the RoleBindings myself via the user-provided yamls & I don't necessarily want to create new service accounts.

At the moment, there is no config option to disable this.
The rbac manifests generation is triggered by the kubernetes-client extension (see

roleBindingProducer.produce(new KubernetesRoleBindingBuildItem("view", true));
).
Therefore, we could easily add a new property to not do so if disabled.
Would this make sense to you? cc @edeandrea @iocanel ?

@edeandrea
Copy link
Contributor

Yeah I think that would make sense, at least for my use case. The default could be to do what is being done today, but if disabled, don't generate a role binding or service account.

@Sgitario
Copy link
Contributor Author

Sgitario commented May 24, 2022

Yeah I think that would make sense, at least for my use case. The default could be to do what is being done today, but if disabled, don't generate a role binding or service account.

Okis. I will create a separate pull request to add a new property that disables the rbac generation.
PR: #25756

@edeandrea
Copy link
Contributor

Thanks @Sgitario !

@geoand
Copy link
Contributor

geoand commented Jun 2, 2022

@iocanel can you take a look at this please?

Thanks

@edeandrea
Copy link
Contributor

Hi any progress on this one?

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM

@geoand geoand merged commit 31932db into quarkusio:main Jun 27, 2022
@quarkus-bot quarkus-bot bot added this to the 2.11 - main milestone Jun 27, 2022
@Sgitario Sgitario deleted the fix_wrong_service_account branch June 27, 2022 12:33
@iocanel
Copy link
Contributor

iocanel commented Jun 27, 2022

While I can see how this pull request fixes: #25688, I feel that the issue description indicates that when existing resources are used some things are performed possibly out of the expected order. This is something that we may want to track, as it's likely we'll encounter that in the future.

@edeandrea
Copy link
Contributor

edeandrea commented Jul 1, 2022

Hey @Sgitario I just tried this out for the first time and I don't think the behavior is correct. When I generate I can see the ServiceAccount is created, but if I have other existing Deployments or DeploymentConfigs within my src/main/kubernetes files it is applying the service account to ALL of the resources rather than JUST the resource its supposed to.

For example, looking at https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-fights/src/main/kubernetes/openshift.yml you'll notice there are 3 Deployments listed (apicurio, fights-kafka, and fights-db).

When I generate openshift.yml with the updated kubernetes-client extension, the resulting target/kubernetes/openshift.yml the rest-fights ServiceAccount, but that service account is applied to ALL of the other Deployments. It should only be applied to the DeploymentConfig for rest-fights.

I would think this is a problem still. I need to use quarkus.kubernetes-client.generate-rbac=false to suppress this.

Sgitario added a commit to Sgitario/quarkus that referenced this pull request Jul 5, 2022
@Sgitario
Copy link
Contributor Author

Sgitario commented Jul 5, 2022

Hey @Sgitario I just tried this out for the first time and I don't think the behavior is correct. When I generate I can see the ServiceAccount is created, but if I have other existing Deployments or DeploymentConfigs within my src/main/kubernetes files it is applying the service account to ALL of the resources rather than JUST the resource its supposed to.

For example, looking at https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-fights/src/main/kubernetes/openshift.yml you'll notice there are 3 Deployments listed (apicurio, fights-kafka, and fights-db).

When I generate openshift.yml with the updated kubernetes-client extension, the resulting target/kubernetes/openshift.yml the rest-fights ServiceAccount, but that service account is applied to ALL of the other Deployments. It should only be applied to the DeploymentConfig for rest-fights.

I would think this is a problem still. I need to use quarkus.kubernetes-client.generate-rbac=false to suppress this.

I've changed this behavior in this pull request: #26558
Let's continue the discussion there.

gsmet pushed a commit to gsmet/quarkus that referenced this pull request Jul 5, 2022
This change is related to this comment: quarkusio#25750 (comment)

(cherry picked from commit 2521138)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubernetes-client and kubernetes extension combinations don't generate the correct things
5 participants