-
Notifications
You must be signed in to change notification settings - Fork 376
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
Adds Initial Infra Manager #178
Conversation
|
||
// A nil infra proxy ir means the proxy infra should be deleted, but metadata is | ||
// required to know the ns/name of the resources to delete. Add support for deleting | ||
// the infra when https://github.com/envoyproxy/gateway/issues/173 is resolved. |
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.
imho the metadata should only be used for informational features such as labelling and status and not for CRUD of k8s resources.
one way to handle deletes could be for the infra manager to reconcile all k8s resources owned by this infra manager (tied to a single GW Class), and delete those resources that are not part of the Translate o/p
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.
IMO infra IR metadata should include a deletionTimestamp
that is set by the Resource Translator when the associated Gateway is deleted. An infra IR with deletionTimestamp
tells the Infra Manager to delete the associated infra.
xref kube meta: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#metadata
one way to handle deletes could be for the infra manager to reconcile all k8s resources owned by this infra manager (tied to a single GW Class), and delete those resources that are not part of the Translate o/p
Yes, this is a possible solution but IMO not as efficient as it requires a List() and then Delete().
// Manager provides the scaffolding for managing infrastructure. | ||
type Manager struct { | ||
Kubernetes *kubernetes.Infra | ||
} |
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.
Do we expect there to multiple things here? Before I got to this file, I was assuming Manager
was going to be an interface that kubernetes.Infra
implements.
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, the struct will contain a field for every supported provider. I created #182 to track the need to move this to an interface.
Codecov Report
@@ Coverage Diff @@
## main #178 +/- ##
==========================================
- Coverage 61.74% 60.16% -1.59%
==========================================
Files 20 23 +3
Lines 1814 1993 +179
==========================================
+ Hits 1120 1199 +79
- Misses 632 722 +90
- Partials 62 72 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. |
// NewInfra returns a new Infra. | ||
func NewInfra(cli client.Client) *Infra { | ||
return &Infra{ | ||
mu: sync.Mutex{}, |
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.
nit: this line is unnecessary
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
Signed-off-by: danehans <[email protected]>
Signed-off-by: danehans <[email protected]>
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.
👍
Adds initial Infra Manager implementation.
Requires: #183
xref: #41
Signed-off-by: danehans [email protected]