-
Notifications
You must be signed in to change notification settings - Fork 1
Add YurtIngress operator implementation #32
Conversation
@zzguang: GitHub didn't allow me to assign the following users: your_reviewer. Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1eb5122
to
cb667d0
Compare
/assign @rambohe-ch, @kadisi |
service: | ||
name: yurtapp-webhook-service | ||
namespace: kube-system | ||
path: /validate-apps-openyurt-io-v1alpha1-yurtingress |
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.
lack of "mutate-apps-openyurt-io-v1alpha1-yurtingress" ?
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.
only validating webhook is implemented, no mutating webhook.
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.
mutate-apps-openyurt-io-v1alpha1-yurtingress
shoud auto generated by pkg/yurtappmanager/webhook/yurtingress/mutating/webhooks.go:
/ +kubebuilder:webhook:path=/mutate-apps-openyurt-io-v1alpha1-yurtingress,mutating=true,failurePolicy=fail,groups=apps.openyurt.io,resources=yurtingresses,verbs=create;update,versions=v1alpha1,name=myurtingress.kb.io
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.
Yes, it is auto generated, and the generated codes are kept as well. But if we don't need to modify the request from user, why not leave this mutating webhook disabled? It might reduce the communication with webhook server as well.
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.
ok, will add mutating webhook to keep consistent with other components. :-)
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.
Yes, it is auto generated, and the generated codes are kept as well. But if we don't need to modify the request from user, why not leave this mutating webhook disabled? It might reduce the communication with webhook server as well.
yes, if we don't need to modify the request from user , we need let this mutating webhook disabled 。 So we need to comment out this line / +kubebuilder:webhook:path=/mutate-apps-openyurt-io-v1alpha1-yurtingress,mutating=true,failurePolicy=fail,groups=apps.openyurt.io,resources=yurtingresses,verbs=create;update,versions=v1alpha1,name=myurtingress.kb.io
in pkg/yurtappmanager/webhook/yurtingress/mutating/webhooks.go
file. Otherwise, the mutate configuration will be generated in manager.yaml each time make generate-manifests
is executed .
isIngressCRChanged = true | ||
ownerRef := prepareDeploymentOwnerReferences(instance) | ||
if currentPoolNames == nil { | ||
yurtapputil.CreateNginxIngressCommonResource(r.Client) |
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.
need to deal error
} | ||
err = client.Create(context.Background(), ns) | ||
if err != nil { | ||
return fmt.Errorf("fail to create the namespace/%s: %v", ns.Name, err) |
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.
need to check whether is exist
} | ||
for _, pool := range addedPools { | ||
replicas := instance.Spec.Replicas | ||
yurtapputil.CreateNginxIngressSpecificResource(r.Client, pool, replicas, ownerRef) |
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.
need deal with error
isIngressCRChanged = true | ||
for _, pool := range removedPools { | ||
if desiredPoolNames == nil { | ||
yurtapputil.DeleteNginxIngressSpecificResource(r.Client, pool, true) |
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.
need to deal with error
if desiredPoolNames == nil { | ||
yurtapputil.DeleteNginxIngressSpecificResource(r.Client, pool, true) | ||
} else { | ||
yurtapputil.DeleteNginxIngressSpecificResource(r.Client, pool, false) |
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.
deal with error
} | ||
} | ||
if desiredPoolNames == nil { | ||
yurtapputil.DeleteNginxIngressCommonResource(r.Client) |
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.
error
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.
well done , just some nit
cb667d0
to
39f784d
Compare
Thanks for @kadisi 's valuable suggestions, please review the updated version. |
YurtIngress operator is used to control and manage ingress controller in NodePools, user can enable/disable ingress feature of multi NodePools through the singleton YurtIngress CR, the init version only contains some basic functions such as which NodePools to enable ingress, ingress controller replicas for every pool, and it can be extended to support other functions according to user requirements in future. Signed-off-by: zhenggu1 <zhengguang.zhang@intel.com>
Signed-off-by: Linda Yu <linda.yu@intel.com>
39f784d
to
ff57388
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kadisi, zzguang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@zzguang @LindaYu17 @gnunu Thank you for your contributions。 |
THANKS! |
Signed-off-by: zhenggu1 <zhengguang.zhang@intel.com>
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
other Note